GNOME Bugzilla – Bug 734081
[review] dcbw/wifi-random-mac-bgo734081: randomize MAC address when scanning wifi
Last modified: 2015-11-19 15:07:10 UTC
We should randomize the interface's MAC address when scanning wifi networks just like iOS 8 does, to ensure user safety and privacy.
[sidenote] that was also discussed here shortly: https://mail.gnome.org/archives/networkmanager-list/2014-June/msg00025.html https://mail.gnome.org/archives/networkmanager-list/2014-June/msg00030.html
Also done in Android: http://arstechnica.com/gadgets/2015/10/android-6-0-marshmallow-thoroughly-reviewed/5/#h1
wpa_supplicant has support for this in v2.3 (?) and later via a config option. There are a couple settings for this: 1) don't randomize 2) randomize only while scanning, but use normal MAC for actual associations (most compatible) 3) randomize while scanning and while associating (most private/secure, less compatible) Essentially, NM's 802-11-wireless setting could grow an property for these (defaulting to #1/don't randomize) and then users and UIs could set the option as appropriate. The question is what to do if the installed supplicant doesn't support the option; I would argue that it would be very bad to silently connect if the user requested MAC randomizing but it wasn't supported. I think NM should fail with a useful DeviceStateReason in that case.
Turns out we need supplicant patches to expose the right properties, which have been sent to the wpa_supplicant mailing lists.
See dcbw/wifi-random-mac-bgo734081 for the NM side of the implementation.
dcbw/wifi-random-mac-bgo734081 will always randomize the MAC address when scanning. It will also randomize the MAC address for associations if the connection sets the 802-11-wireless.mac-address-randomization to "always", though the default is left to not randomize for associations. This is most compatible. The default "never" for associations can be overridden by NM defaults, or can be set by the GUI when creating a new connection.
>> libnm: add Wi-Fi MAC address randomization property The name: NMSettingWirelessMacAddressRandomization nm_setting_wireless_mac_address_randomization_get_type is quite long. We also have NMVlanFlags, instead of NMSettingVlanFlags. Maybe this should be NMWirelessMacAddressRandomization... and maybe abbreviate something: "MacAddress" vs. "Mac" "Address" vs. "Addr" "Randomization" vs. "Rand". Do you actually expect the enum to get more values in the future? We could use a generic type like NMBoolean which has 3 values: Default, True, False. Otherwise, what is the "scan-only" option that is mentioned in the code-comment for ifcfg-rh files? >> wifi/supplicant: change ApSupport to FeatureSupport Could you mention "trivial" in the commit message, indicating that it's a rename only. Also, I would name it NMWifiSuppFeatureSupport (or something like that), because it's in a header file. I think all defines in a header file should have a NM-prefix. (except STRLEN() macro). I pushed a few fixups and new patches. I think, we should enable this feature by default (if supported). The question is, whether to upgrade existing connection to also enable it by default.
(In reply to Thomas Haller from comment #7) > >> libnm: add Wi-Fi MAC address randomization property > > The name: > NMSettingWirelessMacAddressRandomization > nm_setting_wireless_mac_address_randomization_get_type > is quite long. > We also have NMVlanFlags, instead of NMSettingVlanFlags. Maybe this should > be NMWirelessMacAddressRandomization... > and maybe abbreviate something: > "MacAddress" vs. "Mac" > "Address" vs. "Addr" > "Randomization" vs. "Rand". I realized after I'd done the libnm changes that we don't actually need separate settings for wifi at this time. So theoretically we could instead have NMMacRandomization instead of that and use it for 802-3-ethernet too. If we needed to add WiFi-specific options later, we could just add them there and validate in NMSettingWired. > Do you actually expect the enum to get more values in the future? We could > use a generic type like NMBoolean which has 3 values: Default, True, False. > Otherwise, what is the "scan-only" option that is mentioned in the > code-comment for ifcfg-rh files? That option is gone now, because it's always turned on and it was not connection-specific anyway. eg, you want to use a random MAC before any connection is started, so obviously it cannot be a connection-specific option :) I realized that halfway through and corrected it but apparently not everywhere. > >> wifi/supplicant: change ApSupport to FeatureSupport > > Could you mention "trivial" in the commit message, indicating that it's a > rename only. Sure. > Also, I would name it NMWifiSuppFeatureSupport (or something like that), > because it's in a header file. I think all defines in a header file should > have a NM-prefix. (except STRLEN() macro). Sure. > I pushed a few fixups and new patches. Saw those, they look good thanks! > I think, we should enable this feature by default (if supported). > The question is, whether to upgrade existing connection to also enable it by > default. I would actually keep it disabled by default for *associations* (which is how the branch currently works). That's most compatible. We cannot enable it for existing connections (and shouldn't for new ones either) because it will then break any connection where the AP uses MAC address ACLs, which is somewhat common. It can also break roaming cases in Enterprise networks where the MAC can be part of how the network tracks authentication between access points. Even so, having the scanning always randomized is a big win since it won't disclose your MAC address when you're out-and-about unless you actually associate to something. Note that Android 6.0 (according to Ars Technica that Bastien linked to) appears to only randomize on scans too, at least by default: "When Android does a background Wi-Fi and Bluetooth scan, (for finding open networks or Eddystone Bluetooth beacons) the scan spoofs the MAC address and identifies as a randomized value."
*** Bug 720064 has been marked as a duplicate of this bug. ***
(In reply to Dan Williams from comment #8) > (In reply to Thomas Haller from comment #7) could you rebase the branch and squash the fixup commits (so that it's clear which part to keep and which to reject)?
Just rebased/repushed, fixed for comments, squashed, and added ifcfg-rh testcases. If we decide to keep the default as "never" then we can drop "ifcfg-rh: add svSetValueInt64() utility" and "wifi: enable mac-address-randomization by default".
One thing (A) is, whether old connections should switch over to use mac-randomization on upgrade (i.e. what makes keyfile/ifcfg reader if a property is missing). -- probably no. The other (B) is, whether "default" means: enable it. -- maybe yes. "DO NOT MERGE wifi: enable mac-address-randomization by default" does (B). But we do need the keyfile-related part from "DO NOT MERGE wifi: enable mac-address-randomization by default", because they does the opposite of (A): old keyfiles continue to explicitly set "NEVER". also, nmcli support is missing :) Also, back to the previous question: do we ever expect anything but "DEFAULT", "ALWAYS", "NEVER"? Can we not instead introduce a NMBoolean with values "Default|, "Yes", "No" which could be reused for other settings in the future as well?
(In reply to Thomas Haller from comment #12) > One thing (A) is, whether old connections should switch over to use > mac-randomization on upgrade (i.e. what makes keyfile/ifcfg reader if a > property is missing). > -- probably no. > > The other (B) is, whether "default" means: enable it. > -- maybe yes. I agree with these. > "DO NOT MERGE wifi: enable mac-address-randomization by default" does (B). Ah, ok. > But we do need the keyfile-related part from "DO NOT MERGE wifi: enable > mac-address-randomization by default", because they does the opposite of > (A): old keyfiles continue to explicitly set "NEVER". Ok. > also, nmcli support is missing :) Yeah, I hadn't gotten there before you saw the branch and started reviewing it :) You're fast. > Also, back to the previous question: do we ever expect anything but > "DEFAULT", "ALWAYS", "NEVER"? Can we not instead introduce a NMBoolean with > values "Default|, "Yes", "No" which could be reused for other settings in > the future as well? Yes, we might. The supplicant has an option to use a partially randomized MAC address that preserves the OUI of the permanent MAC address. There may be other options in the future too, and maybe we need something specific to Wired (if/when we support wired MAC randomization). So I left it open for these reasons.
updated for comments, added nmcli support, and repushed.
wpa_supplicant 2.6 will have the necessary support through these commits: f0ef68892e84cca695e772a3801fbb732458f1b5 tests: Add testcases for interface global properties e50c50d5a090a6a52af6d92ee3a3c9cc37743747 dbus: Expose interface globals via D-Bus properties 1aa0fb77ea5bcbb600673958029b3553a6c27220 dbus: Pass property description to getters/setters
> wifi: implement MAC address randomization + } else if ( mac_randomization != NM_SETTING_MAC_RANDOMIZATION_NEVER + && nm_supplicant_interface_get_mac_randomization_support (priv->sup_iface) != SUPPLICANT_FEATURE_YES) { + _LOGW (LOGD_DEVICE | LOGD_WIFI, "MAC address randomization is not supported"); + mac_randomization = NM_SETTING_MAC_RANDOMIZATION_NEVER; + } If the user requested MAC randomization but the supplicant doesn't support it, maybe we should consider failing the activation? > wifi: enable mac-address-randomization by default for new connections + mac_randomization_supported = nm_supplicant_interface_get_mac_randomization_support (priv->sup_iface) != SUPPLICANT_FEATURE_YES; s/!=/==/ ? Pushed some fixups. The rest LGTM.
I rebased the branch on master and added a few fixups. (previous version is at dcbw/wifi-random-mac-bgo734081-1) LGTM now. Question from comment 16 is still open.
(In reply to Beniamino Galvani from comment #16) > > wifi: implement MAC address randomization > > + } else if ( mac_randomization != NM_SETTING_MAC_RANDOMIZATION_NEVER > + && nm_supplicant_interface_get_mac_randomization_support > (priv->sup_iface) != SUPPLICANT_FEATURE_YES) { > + _LOGW (LOGD_DEVICE | LOGD_WIFI, "MAC address randomization > is not supported"); > + mac_randomization = NM_SETTING_MAC_RANDOMIZATION_NEVER; > + } > > If the user requested MAC randomization but the supplicant doesn't > support it, maybe we should consider failing the activation? It would have, but later. It was very unclear; fixed up now. All fixups squashed, rebased, and reposted.
LGTM
This branch was IMO ready. Merged. master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=59d243438397338b9dd41bd194f2bb08f9b5e509
What's the default value then?
(In reply to Bastien Nocera from comment #21) > What's the default value then? for scanning, randomization is always enabled (as wpa-supplicant supports it). for connecting: - if the new mac-address-randomization settings is explicitly set, that value is used. - When having the per-connection setting set to "default", the value can be specified in global configuration file (see `man NetworkManager.conf`). - when the value is still unspecified (in global configuration file), randomization is used as supported. Note, that * existing keyfiles/ifcfg-rh-files on disk have this flag missing, which means randomization is disabled for them unless they do: nmcli connection modify $CONNECTION \ 802-11-wireless.mac-address-randomization default * if you manually create a new keyfile/ifcfg-rh file the same applies. A missing setting means NEVER. * When making a D-Bus request to add/update a connection but omitting the mac-addr-rand-setting, DEFAULT is assumed. That means, old clients that are not aware of the new flag, always implicitly set it to DEFAULT. That is also true when modifying an existing connection, but it's not really avoidable (without having other downsides).
After discussion, I reverted "enabling-by-default". See commit http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=98e419496380758d8942606a96eaa199f5c7d10f (In reply to Thomas Haller from comment #22) > - when the value is still unspecified (in global configuration file), > randomization is used as supported. now it should read: ... randomization is disabled. Also opened bug 758301 for a follow-up feature
(In reply to Thomas Haller from comment #23) > After discussion, I reverted "enabling-by-default". > > See commit > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > ?id=98e419496380758d8942606a96eaa199f5c7d10f I really don't see an explanation here. It "[can be bad] for captive portals". Doesn't explain why, or how.
(In reply to Bastien Nocera from comment #24) > (In reply to Thomas Haller from comment #23) > > After discussion, I reverted "enabling-by-default". > > > > See commit > > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > > ?id=98e419496380758d8942606a96eaa199f5c7d10f > > I really don't see an explanation here. It "[can be bad] for captive > portals". Doesn't explain why, or how. But I didn't want to seal my half-knowledge about this in an eternal git-commit. I'd think that a captive portal associates the session with the MAC address, so that randomizing your MAC address requires you to re-authenticate on every connect. Also, certain Wi-Fi hotspots filter by MAC address. Thus randomizing by default is bad in those cases.
(In reply to Thomas Haller from comment #25) > (In reply to Bastien Nocera from comment #24) > > (In reply to Thomas Haller from comment #23) > > > After discussion, I reverted "enabling-by-default". > > > > > > See commit > > > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > > > ?id=98e419496380758d8942606a96eaa199f5c7d10f > > > > I really don't see an explanation here. It "[can be bad] for captive > > portals". Doesn't explain why, or how. > > But I didn't want to seal my half-knowledge about this in an eternal > git-commit. > > I'd think that a captive portal associates the session with the MAC address, > so that randomizing your MAC address requires you to re-authenticate on > every connect. Yes, basically that. (At $WORK, we still use a captive portal system for student Wi-Fi, and I've seen two different students rack up some ~200 sessions each because their phones kept using a different MAC address every time.)
(In reply to Thomas Haller from comment #25) > (In reply to Bastien Nocera from comment #24) > > (In reply to Thomas Haller from comment #23) > > > After discussion, I reverted "enabling-by-default". > > > > > > See commit > > > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > > > ?id=98e419496380758d8942606a96eaa199f5c7d10f > > > > I really don't see an explanation here. It "[can be bad] for captive > > portals". Doesn't explain why, or how. > > But I didn't want to seal my half-knowledge about this in an eternal > git-commit. > > I'd think that a captive portal associates the session with the MAC address, > so that randomizing your MAC address requires you to re-authenticate on > every connect. > > Also, certain Wi-Fi hotspots filter by MAC address. Thus randomizing by > default is bad in those cases. That's what I thought, but I'd really prefer justification to be in the commit messages (or in the bugzilla linked from the commit, if you prefer that), otherwise it's hard to know the justification... I guess that the worst offenders are blocked by default though, with the scanning using randomised addresses. Thanks.