After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748367 - [review] NetworkManager lacks support for 802.11w (management frame protection) [bg/wifi-pmf-bgo748367-pt2]
[review] NetworkManager lacks support for 802.11w (management frame protectio...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: Wi-Fi
1.2.x
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks: nm-review
 
 
Reported: 2015-04-23 13:59 UTC by bordjukov
Modified: 2017-04-28 08:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support 802.11w (3.61 KB, patch)
2017-01-05 15:19 UTC, Stijn Tintel
none Details | Review
Revised patch against git master (4.60 KB, patch)
2017-03-15 01:53 UTC, Michael Cronenworth
none Details | Review

Description bordjukov 2015-04-23 13:59:57 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.
Comment 1 Stijn Tintel 2016-12-28 06:07:40 UTC
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
Comment 2 Beniamino Galvani 2017-01-02 16:20:03 UTC
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
Comment 4 Stijn Tintel 2017-01-05 15:19:53 UTC
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.
Comment 5 Thomas Haller 2017-01-05 17:19:03 UTC
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!!
Comment 6 Stijn Tintel 2017-01-05 18:14:50 UTC
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.
Comment 7 Stijn Tintel 2017-01-30 18:47:12 UTC
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.
Comment 8 Michael Cronenworth 2017-03-11 06:09:34 UTC
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?
Comment 9 Stijn Tintel 2017-03-11 06:22:29 UTC
(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.
Comment 10 Michael Cronenworth 2017-03-15 01:53:48 UTC
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.
Comment 11 Beniamino Galvani 2017-03-19 09:27:32 UTC
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.
Comment 12 Thomas Haller 2017-03-22 13:47:15 UTC
bg/wifi-pmf-bgo748367 lgtm
Comment 13 Michael Cronenworth 2017-04-14 16:44:03 UTC
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?
Comment 14 Dan Williams 2017-04-14 16:49:23 UTC
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.
Comment 15 Beniamino Galvani 2017-04-15 08:48:32 UTC
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.
Comment 16 Beniamino Galvani 2017-04-20 10:01:41 UTC
(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?
Comment 17 Thomas Haller 2017-04-20 12:01:15 UTC
minor fixup pushed.

rest lgtm
Comment 18 Dan Williams 2017-04-26 17:23:31 UTC
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?
Comment 19 Michael Cronenworth 2017-04-26 20:34:26 UTC
Even though I'm outside of NM development, yes, it should be in the security settings. PMF is only possible with authentication.
Comment 20 Michael Cronenworth 2017-04-26 20:36:59 UTC
(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.
Comment 21 Beniamino Galvani 2017-04-27 15:08:52 UTC
(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.
Comment 22 Thomas Haller 2017-04-27 19:20:50 UTC
> 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().
Comment 23 Beniamino Galvani 2017-04-28 08:08:27 UTC
(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