[PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Nicolas Cavallari]

When drop_unencrypted is enabled and MFP is disabled, non-robust
management frames for not-yet associated STA are dropped  before
being transmitted.

This isn't visible as many management frames sent from  the kernel
have TX_INTFL_DONT_ENCRYPT set and management frames injected from
monitor vifs have TX_CTL_INJECTED set, so are never dropped in the
tx path. But mgmt frames sent from userspace via NL80211_CMD_FRAME
do not have any of these flags, so are dropped.

This patch make it accept non-robust management frames,
but drops non-management frames and robust management frames as usual.

Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>

---

Found this while trying to manage auth  frames in userspace
on a IBSS RSN network.
I'm  a bit surprised that drop_unencrypted is disabled both
in AP and managed mode, but only enabled in IBSS RSN, which
does not support MFP, yet.

patch based on linux-wireless 565e1fc.

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4990f4f..97682e0 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -545,6 +545,8 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 	else if (tx->sdata->drop_unencrypted &&
 		 (tx->skb->protocol != tx->sdata->control_port_protocol) &&
 		 !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
+		 (!ieee80211_is_mgmt(hdr->frame_control) ||
+		  ieee80211_is_robust_mgmt_frame(hdr)) &&
 		 (!ieee80211_is_robust_mgmt_frame(hdr) ||
 		  (ieee80211_is_action(hdr->frame_control) &&
 		   tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))) {
-- 
1.7.10.4

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Johannes Berg]

On Wed, 2012-07-04 at 11:13 +0200, Nicolas Cavallari wrote:
> When drop_unencrypted is enabled and MFP is disabled, non-robust
> management frames for not-yet associated STA are dropped  before
> being transmitted.
> 
> This isn't visible as many management frames sent from  the kernel
> have TX_INTFL_DONT_ENCRYPT set and management frames injected from
> monitor vifs have TX_CTL_INJECTED set, so are never dropped in the
> tx path. But mgmt frames sent from userspace via NL80211_CMD_FRAME
> do not have any of these flags, so are dropped.
> 
> This patch make it accept non-robust management frames,
> but drops non-management frames and robust management frames as usual.
> 
> Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>
> 
> ---
> 
> Found this while trying to manage auth  frames in userspace
> on a IBSS RSN network.
> I'm  a bit surprised that drop_unencrypted is disabled both
> in AP and managed mode, but only enabled in IBSS RSN, which
> does not support MFP, yet.

Hmmm. Yes, all this seems a bit messy. Can we clean this up first?

Maybe something like this?
http://p.sipsolutions.net/d68a99dd3d9b1ab1.txt

The whole drop_unencrypted seems strange to start with though...

johannes

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Nicolas Cavallari]

On 04/07/2012 11:35, Johannes Berg wrote:
> On Wed, 2012-07-04 at 11:13 +0200, Nicolas Cavallari wrote:
>> Found this while trying to manage auth  frames in userspace
>> on a IBSS RSN network.
>> I'm  a bit surprised that drop_unencrypted is disabled both
>> in AP and managed mode, but only enabled in IBSS RSN, which
>> does not support MFP, yet.
> 
> Hmmm. Yes, all this seems a bit messy. Can we clean this up first?

The whole function is a bit messy. some clean ups would be nice
but it is too easy to break corner cases.

> 
> Maybe something like this?
> http://p.sipsolutions.net/d68a99dd3d9b1ab1.txt

No problem for me. I didn't want that patch to too intrusive at first.

Do you want to apply it yourself ? or should i include it in a series ?

> The whole drop_unencrypted seems strange to start with though...

what to you find strange ? that it is not used in a ESS, or how it works
? or both ?

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Johannes Berg]

On Wed, 2012-07-04 at 12:03 +0200, Nicolas Cavallari wrote:

> > Hmmm. Yes, all this seems a bit messy. Can we clean this up first?
> 
> The whole function is a bit messy. some clean ups would be nice
> but it is too easy to break corner cases.

Yeah ...

> > Maybe something like this?
> > http://p.sipsolutions.net/d68a99dd3d9b1ab1.txt
> 
> No problem for me. I didn't want that patch to too intrusive at first.

Can you review it? :)

> Do you want to apply it yourself ? or should i include it in a series ?

Then ... probably easiest for you to include it so you can work on top.
Here's a version with commit log:
http://p.sipsolutions.net/17ea0ebece544a59.txt

> > The whole drop_unencrypted seems strange to start with though...
> 
> what to you find strange ? that it is not used in a ESS, or how it works
> ? or both ?

The way it's used/works/isn't used.

johannes

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Nicolas Cavallari]

On 04/07/2012 12:12, Johannes Berg wrote:
> Can you review it? :)
> 

> +	else if (ieee80211_is_robust_mgmt_frame(hdr) &&
> +		 (!ieee80211_is_action(hdr->frame_control) ||
> +		  !tx->sta || !test_sta_flag(tx->sta, WLAN_STA_MFP)))
> +		tx->key = NULL;

I would have written that more like

is_robust_mgmt && !(is_action && tx->sta && test_sta_flag(mfp))

So it's more clear that we drop action frames to mfp stas, as per the spec.

> +	else {
> +
>  		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
>  		return TX_DROP;

is that blank line needed ?

>> Do you want to apply it yourself ? or should i include it in a series ?
> 
> Then ... probably easiest for you to include it so you can work on top.
> Here's a version with commit log:
> http://p.sipsolutions.net/17ea0ebece544a59.txt
> 

Ok. will send a series with this patch modified once i test it and my
new patch version.

>>> The whole drop_unencrypted seems strange to start with though...
>>
>> what to you find strange ? that it is not used in a ESS, or how it works
>> ? or both ?
> 
> The way it's used/works/isn't used.

>From what i understand, data frames to unauthorized stations are dropped
well before selecting the encryption key. as stations are authorized
after or at the same time their encryption key are set, it somewhat
works. But for MFP, i'm not sure. I think that drop_unencrypted is
mistaken for "drop_unencrypted_management" there.
But i'm not an MFP expert.

I'm not sure if we should just add a separate
drop_unencrypted_management, or just replace drop_unencrypted with
drop_unencrypted_management. But in a IBSS with RSN, if wpa_supplicant
isn't recent enough, stations are always authorized by default. so
drop_encrypted is required in this case.

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Johannes Berg]

On Wed, 2012-07-04 at 15:00 +0200, Nicolas Cavallari wrote:

> > +	else if (ieee80211_is_robust_mgmt_frame(hdr) &&
> > +		 (!ieee80211_is_action(hdr->frame_control) ||
> > +		  !tx->sta || !test_sta_flag(tx->sta, WLAN_STA_MFP)))
> > +		tx->key = NULL;
> 
> I would have written that more like
> 
> is_robust_mgmt && !(is_action && tx->sta && test_sta_flag(mfp))
> 
> So it's more clear that we drop action frames to mfp stas, as per the spec.

Yeah, makes sense. I was translating it directly.

> > +	else {
> > +
> >  		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
> >  		return TX_DROP;
> 
> is that blank line needed ?

Err, no :-)

> >> Do you want to apply it yourself ? or should i include it in a series ?
> > 
> > Then ... probably easiest for you to include it so you can work on top.
> > Here's a version with commit log:
> > http://p.sipsolutions.net/17ea0ebece544a59.txt
> > 
> 
> Ok. will send a series with this patch modified once i test it and my
> new patch version.

Great, thanks.

> >>> The whole drop_unencrypted seems strange to start with though...
> >>
> >> what to you find strange ? that it is not used in a ESS, or how it works
> >> ? or both ?
> > 
> > The way it's used/works/isn't used.
> 
> From what i understand, data frames to unauthorized stations are dropped
> well before selecting the encryption key. as stations are authorized
> after or at the same time their encryption key are set, it somewhat
> works. But for MFP, i'm not sure. I think that drop_unencrypted is
> mistaken for "drop_unencrypted_management" there.
> But i'm not an MFP expert.
> 
> I'm not sure if we should just add a separate
> drop_unencrypted_management, or just replace drop_unencrypted with
> drop_unencrypted_management. But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.

Ok ...

I guess I'll have to let Jouni comment on this, right now I'm not much
less confused than before :-)

johannes

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Nicolas Cavallari]

On 04/07/2012 15:00, Nicolas Cavallari wrote:
But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.

drop_unencrypted, my bad.

[PATCH 1/2] mac80211: restructure key selection

[Nicolas Cavallari]

From: Johannes Berg <johannes.berg@intel.com>

The "no key" case in key selection that decides
whether to drop the frame or not is impossible
to understand, restructure the code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
[cavallar@lri.fr: removed blank line and restructured action frame clause]
Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>
---
 net/mac80211/tx.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4990f4f..cdd4ac6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -523,7 +523,7 @@ ieee80211_tx_h_check_control_port_protocol(struct ieee80211_tx_data *tx)
 static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 {
-	struct ieee80211_key *key = NULL;
+	struct ieee80211_key *key;
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
 
@@ -542,16 +542,20 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 	else if (!is_multicast_ether_addr(hdr->addr1) &&
 		 (key = rcu_dereference(tx->sdata->default_unicast_key)))
 		tx->key = key;
-	else if (tx->sdata->drop_unencrypted &&
-		 (tx->skb->protocol != tx->sdata->control_port_protocol) &&
-		 !(info->flags & IEEE80211_TX_CTL_INJECTED) &&
-		 (!ieee80211_is_robust_mgmt_frame(hdr) ||
-		  (ieee80211_is_action(hdr->frame_control) &&
-		   tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))) {
+	else if (info->flags & IEEE80211_TX_CTL_INJECTED)
+		tx->key = NULL;
+	else if (!tx->sdata->drop_unencrypted)
+		tx->key = NULL;
+	else if (tx->skb->protocol == tx->sdata->control_port_protocol)
+		tx->key = NULL;
+	else if (ieee80211_is_robust_mgmt_frame(hdr) &&
+		 !(ieee80211_is_action(hdr->frame_control) &&
+		  tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))
+		tx->key = NULL;
+	else {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TX_DROP;
-	} else
-		tx->key = NULL;
+	}
 
 	if (tx->key) {
 		bool skip_hw = false;
-- 
1.7.10.4

[PATCHv3 2/2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Nicolas Cavallari]

When drop_unencrypted is enabled and MFP is disabled,
non-robust management frames for not-yet associated STA are dropped.
This isn't visible as many management frames sent from the kernel
have TX_INTFL_DONT_ENCRYPT set and management frames injected
from a monitor vif have TX_CTL_INJECTED so aren't dropped.
But management frames sent from userspace via NL80211_CMD_FRAME
do not have this flag set, so are dropped.

This patch make it always accept non-robust management frames.

Signed-off-by: Nicolas Cavallari <cavallar@lri.fr>

---
changes :
v2->v3 : now depends on johannes's cleanup, even clearer code.
v1->v2 : thinier, clearer, more correct code.  migrated from RFC to PATCH.
---
 net/mac80211/tx.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index cdd4ac6..691aa6c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -552,6 +552,9 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 		 !(ieee80211_is_action(hdr->frame_control) &&
 		  tx->sta && test_sta_flag(tx->sta, WLAN_STA_MFP)))
 		tx->key = NULL;
+	else if (ieee80211_is_mgmt(hdr->frame_control) &&
+		 !ieee80211_is_robust_mgmt_frame(hdr))
+		tx->key = NULL;
 	else {
 		I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
 		return TX_DROP;
-- 
1.7.10.4

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Jouni Malinen]

On Wed, Jul 04, 2012 at 03:00:03PM +0200, Nicolas Cavallari wrote:
> From what i understand, data frames to unauthorized stations are dropped
> well before selecting the encryption key. as stations are authorized
> after or at the same time their encryption key are set, it somewhat
> works. But for MFP, i'm not sure. I think that drop_unencrypted is
> mistaken for "drop_unencrypted_management" there.
> But i'm not an MFP expert.

drop_unencrypted was originally (i.e., way before MFP) added as an extra
protection for some corner cases where keys may not have been set. In
theory, the PAE (authorized vs. unauthorized) should have covered those
cases, but there were some multi-SSID AP cases that were not obviously
clear. Consequently, it felt safer to add an extra protection for BSSes
that are known to use encryption for data frames.

As far as MFP is concerned, we have the WLAN_STA_MFP flag that should be
more reliable way of determining whether robust management frames have
to be protected.

> I'm not sure if we should just add a separate
> drop_unencrypted_management, or just replace drop_unencrypted with
> drop_unencrypted_management.

I hope that neither of those would be needed.

> But in a IBSS with RSN, if wpa_supplicant
> isn't recent enough, stations are always authorized by default. so
> drop_encrypted is required in this case.

For a BSS that uses RSN, we could maintain a new flag that indicates
that (non-nullfunc) Data frames are not to be transmitted or received
without protected. Though, this would be quite similar to
drop_unencrypted in practice.


As far as the new patch is concerned, it would look like this is
extending the fix in commit e0463f501fb945c1fde536d98eefc5ba156ff497.
The commit log for that change seems to claim that the goal was to avoid
dropping any management frames to a STA that does not use MFP, but the
change does not seem to do that.

As far as drop_unencrypted not being used in AP/managed mode is
concerned, that sounds like an additional bug.. This code is supposed to
drop Action frames from STA/AP before 4-way handshake. If we want to get
rid of drop_unencrypted, this function may need another condition to
drop the frame based on WLAN_STA_MFP flag. I have clearly assumed that
drop_unencrypted was set here (and maybe that was indeed the case in
early 2009 or maybe I did testing with WEXT at the time based on commit
0c7c10c7cc6bc890d23c8c62b81b4feccd92124b).

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2] mac80211: tx: do not drop non-robust mgmt to non-MFP stas.

[Johannes Berg]

On Wed, 2012-07-04 at 20:44 +0300, Jouni Malinen wrote:

> drop_unencrypted was originally (i.e., way before MFP) added as an extra
> protection for some corner cases where keys may not have been set. In
> theory, the PAE (authorized vs. unauthorized) should have covered those
> cases, but there were some multi-SSID AP cases that were not obviously
> clear. Consequently, it felt safer to add an extra protection for BSSes
> that are known to use encryption for data frames.

Hmm, ok.

> As far as MFP is concerned, we have the WLAN_STA_MFP flag that should be
> more reliable way of determining whether robust management frames have
> to be protected.

Right.

> > But in a IBSS with RSN, if wpa_supplicant
> > isn't recent enough, stations are always authorized by default. so
> > drop_encrypted is required in this case.
> 
> For a BSS that uses RSN, we could maintain a new flag that indicates
> that (non-nullfunc) Data frames are not to be transmitted or received
> without protected. Though, this would be quite similar to
> drop_unencrypted in practice.
> 
> 
> As far as the new patch is concerned, it would look like this is
> extending the fix in commit e0463f501fb945c1fde536d98eefc5ba156ff497.
> The commit log for that change seems to claim that the goal was to avoid
> dropping any management frames to a STA that does not use MFP, but the
> change does not seem to do that.

Yeah, it's a bit confusing, especially since the drop_unencrypted is in
there.

> As far as drop_unencrypted not being used in AP/managed mode is
> concerned, that sounds like an additional bug.. This code is supposed to
> drop Action frames from STA/AP before 4-way handshake. If we want to get
> rid of drop_unencrypted, this function may need another condition to
> drop the frame based on WLAN_STA_MFP flag. I have clearly assumed that
> drop_unencrypted was set here (and maybe that was indeed the case in
> early 2009 or maybe I did testing with WEXT at the time based on commit
> 0c7c10c7cc6bc890d23c8c62b81b4feccd92124b).

It looks a bit it got lost years ago in commit
f21293549f60f88c74fcb9944737f11048896dc4, but I can't tell you why. We
also never added nl80211 API for it. Did we just miss it?

I guess what we should do now is figure out what should be going on, do
we even need drop_unencrypted still or are we ok with only MFP?

johannes

Re: [PATCH 1/2] mac80211: restructure key selection

[Johannes Berg]

Applied both. I think we need to figure out more of the story with
drop_unencrypted and why we say we don't want to encrypt BA action
frames?!

johannes