GNOME Bugzilla – Bug 748367
[review] NetworkManager lacks support for 802.11w (management frame protection) [bg/wifi-pmf-bgo748367-pt2]
Last modified: 2017-04-28 08:08:27 UTC
IEEE 802.11w-2009 provides enhancements in the security of the management frames used with Wi-Fi networks. wpa-supplicant has had support for it for some time now. Currently there is no apparent way of enabling 802.11w for a Wi-Fi connection in NetworkManager. Due to this, management frame protection is not utilized when connecting to an AP that supports protected management frames and connecting to an AP that requires protected management frames is impossible.
An easy first step to support 802.11w could be to set the global parameter "pmf" to enabled: https://w1.fi/cgit/hostap/commit/?id=62d498033109e4fff00f6913bfd83f5d01ad2b14 This was done for Android in 2014: https://w1.fi/cgit/hostap/commit/?id=9ffd5129240d048fd55c9351dd1e94dade166d41
I think that enabling 802.11w by default as optional is the right thing to do, because it will provide additional security when the AP supports it. The wpa_supplicant documentation seem to imply that 'pmf=1' also requires 'key_mgmt=WPA-EAP WPA-EAP-SHA256' or 'key_mgmt=WPA-PSK WPA-PSK-SHA256' to be effective. Unfortunately WPA-*-SHA256 are accepted only when wpa_supplicant was compiled with CONFIG_IEEE80211W and I didn't find a way to discover whether wpa_supplicant supports it. Maybe we should: - add a new capability to wpa_supplicant [1] - when wpa_supplicant 802.11w support is detected: - set pmf=1 - add WPA-*-SHA256 key_mgmt options Unless someone has specific use cases to explain, I don't think that users should be able to choose whether to enable (optional or required) or disable PMF. [1] https://w1.fi/cgit/hostap/tree/wpa_supplicant/dbus/dbus_new_handlers.c#n975
http://lists.infradead.org/pipermail/hostap/2017-January/036953.html http://lists.infradead.org/pipermail/hostap/2017-January/036952.html
Created attachment 342966 [details] [review] Support 802.11w Quickly hacked up this patch to enable 802.11w support in NetworkManager. It requires a patched wpa_supplicant (see patches in previous comment). With this, I can connect to an AP that requires 11W, and when the AP has optional 11W support, it is also enabled.
could you please rebase the patch on current master, and generate it with `git format-patch`? Also the indention is not according to the style (see 2) in https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/CONTRIBUTING#n9) Thanks!!
Hi Thomas, as mentioned in the patch description I quickly hacked it up. I'm totally not familiar with the NM code, and I just added it here as an example. It's not intended to be included as-is. I might have a go at it later, but right now it's low prio for me.
Seems there might be a reason for the user wanting to disable 802.11w: https://forum.lede-project.org/t/hostapd-enable-sha256-based-algorithms/803/4 It seems to have a very bad impact on speed with some devices. My Intel AC 7265 client connected to a D-Link DAP-2695 (ath10k) doesn't show any impact, happily doing ~570Mbps average with burst up to 600Mbps.
I am using a Intel 7260 and Netgear X4S R7800 AP. No performance drop with MFP enabled. @Stijn, with your permission, and if Thomas is going to commit this patch, do you mind if I resubmit your patch with the necessary formatting changes?
(In reply to Michael Cronenworth from comment #8) > @Stijn, with your permission, and if Thomas is going to commit this patch, > do you mind if I resubmit your patch with the necessary formatting changes? Feel free, it's one of the reason I left it here even though it wasn't quite ready.
Created attachment 347971 [details] [review] Revised patch against git master Here is a patch rebased against master. I didn't see any formatting changes that were needed unless I missed them.
Hi Michael, thanks for the patch. I pushed it to branch bg/wifi-pmf-bgo748367, added a fixup and other commits to enable PMF only when we detect that the supplicant supports it and also to enable stronger authentication algorithms.
bg/wifi-pmf-bgo748367 lgtm
I pulled the 'bg/wifi-pmf-bgo748367' branch along with a clone of the latest git of wpa_supplicant and MFP reports enabled! Bandwidth and stability are not negatively impacted. Thanks! When is this feature going in the master branch?
Branch LGTM. We'll probably eventually want a boolean for force-enable MFP for a given network, so that you can ensure you only connect to a given SSID when MFP is enabled. But that can come later.
Merged the branch to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=03c2949a2c1b5d079bc697e177bfde49b3578fa8 Keeping this bug open for the missing part, i.e. per-connection configuration.
(In reply to Dan Williams from comment #14) > We'll probably eventually want a boolean for force-enable MFP for a given > network, so that you can ensure you only connect to a given SSID when MFP is > enabled. But that can come later. How about branch bg/wifi-pmf-bgo748367-pt2 ? I'm not sure if the new 'pmf' property is best placed in the wifi or wifi-sec setting, any opinions?
minor fixup pushed. rest lgtm
Looks generally good, just one thing... Wifi vs. Wifi-Sec: my feeling is it should go into wifi-sec; AFAICT PMF requires WPA/RSN support, and thus if you actually use PMF you'd have to have a wifi-sec setting already, right?
Even though I'm outside of NM development, yes, it should be in the security settings. PMF is only possible with authentication.
(In reply to Michael Cronenworth from comment #19) > PMF is only possible with authentication. Let me clarify: Only TKIP/AES frames are protected. Other frame types or unprotected frames are not covered by PMF.
(In reply to Dan Williams from comment #18) > Looks generally good, just one thing... > > Wifi vs. Wifi-Sec: my feeling is it should go into wifi-sec; AFAICT PMF > requires WPA/RSN support, and thus if you actually use PMF you'd have to > have a wifi-sec setting already, right? Yes. I moved the property to wifi-sec and added a check in verify() to prevent that PMF is enabled on non-WPA networks.
> libnm-core: add pmf property to wireless-security setting + if ( NM_IN_SET (priv->pmf, + NM_SETTING_WIRELESS_SECURITY_PMF_OPTIONAL, + NM_SETTING_WIRELESS_SECURITY_PMF_REQUIRED) + && !NM_IN_STRSET (priv->key_mgmt, "wpa-eap", "wpa-psk")) { let's have verify() reject all unknown enum values. > ifcfg-rh: support the wifi.pmf property + value = nm_utils_enum_to_str (nm_setting_wireless_security_pmf_get_type(), + nm_setting_wireless_security_get_pmf (s_wsec)); + svSetValueStr (ifcfg, "PMF", value); note how "lr/wps" branch (bgo#781336) uses a different separator for enums. It would be good to decide on a default behavior for how to handle enums -- and not re-implement it over and over again. This merely means to use a new function svSetValueEnum().
(In reply to Thomas Haller from comment #22) > > libnm-core: add pmf property to wireless-security setting > let's have verify() reject all unknown enum values. Ok. > > ifcfg-rh: support the wifi.pmf property > > + value = nm_utils_enum_to_str > (nm_setting_wireless_security_pmf_get_type(), > + nm_setting_wireless_security_get_pmf > (s_wsec)); > + svSetValueStr (ifcfg, "PMF", value); > > > note how "lr/wps" branch (bgo#781336) uses a different separator for enums. > It would be good to decide on a default behavior for how to handle enums -- > and not re-implement it over and over again. This merely means to use a new > function svSetValueEnum(). This enum is a GEnumValue and can only have a single value. Therefore, the resulting string doesn't contain any separators. Merged to master as: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a7fdf09848070990710529946fff782fc60c1461