| Message ID | 1341393221-5396-1-git-send-email-cavallar@lri.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)))) {
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.