GNOME Bugzilla – Bug 777225
[review] nm-connection-editor silently clears WPA Enterprise subject-match properties, reintroducing MITM risk [bg/unsupported-properties-bgo777225]
Last modified: 2017-02-02 14:28:02 UTC
As noted in https://blog.wirelessmoves.com/2016/02/eduroam-wifi-with-a-certificate-and-cool-roaming-features.html, if a WPA Enterprise connection is edited with nm-connection-editor, the subject-match, altsubject-matches, and domain-suffix-match properties (see bug 341323) are silently lost, reintroducing the possibility of MITM attacks on the connection and theft of the user's password (depending on the user authentication method). Thus, IMO, the original security problem cannot be considered to have been reliably solved from the point of view of an end user who uses the connection editor. I reproduced this behavior using the latest master of network-manager-applet (6f0941e) against NetworkManager-1.4.2-2.fc25 on my Fedora 25 system. I believe the offending code is here: https://git.gnome.org/browse/network-manager-applet/tree/src/wireless-security/wireless-security.c?id=6f0941ef26ae1ddf1403c5728b5840603b0d165e#n550 I don't know what the most reasonable solution is. Silently preserve properties that aren't shown in the UI or add a warning to the dialog box that they will be lost? Steps to reproduce: 1. In nm-connection-editor, create a Wi-Fi connection named "test" with "Wi-Fi Security" -> "Security" set to "WPA & WPA2 Enterprise", "Authentication" set to "Tunneled TLS", and enough parameters filled in to pass validation. 2. nmcli connection modify test +802-1x.altsubject-matches DNS:radius.example.com 3. In nm-connection-editor, open the "test" connection and save it without making any changes. 4. nmcli connection show test Actual result: 802-1x.altsubject-matches is blank and a MITM attacker with a certificate for his own domain name from the same CA can intercept the TLS connection. Expected result: 802-1x.altsubject-matches is still set or the user is warned at some point.
Created attachment 344029 [details] [review] [PATCH] libnm-core: return NULL from _nm_utils_slist_to_strv for empty lists Pushed some commits to branch bg/unsupported-properties-bgo777225 to show a warning in the wifi page when there are properties that will be cleared on save. The attached patch is needed in the daemon.
Beniamino, thanks for working on this! I would like to test the proposed fix. I don't see the bg/unsupported-properties-bgo777225 branch at https://cgit.freedesktop.org/NetworkManager/NetworkManager/refs/heads. Should I be looking somewhere else?
Hi, the nm-applet/editor code is in a different repository: https://git.gnome.org/browse/network-manager-applet/log/?h=bg/unsupported-properties-bgo777225
> the nm-applet/editor code is in a different repository Oh, right. Thanks. The fix seems to work for me.
(In reply to Beniamino Galvani from comment #1) > Created attachment 344029 [details] [review] [review] > [PATCH] libnm-core: return NULL from _nm_utils_slist_to_strv for empty lists > The attached patch is needed in the daemon. makes sense #define g_strv_contains _nm_g_strv_contains should use g_strv_contains, not the wrapper implementation. I think nm_connection_editor_check_unsupported_properties() leaks "GValue value". Consider nm_auto_unset_gvalue otherwise, lgtm
(In reply to Thomas Haller from comment #5) > should use g_strv_contains, not the wrapper implementation. > I think nm_connection_editor_check_unsupported_properties() leaks "GValue > value". Consider nm_auto_unset_gvalue Fixed and repushed with an additional commit.
lgtm
Ping... Is the patch going to be merged?
Merged to master: https://git.gnome.org/browse/network-manager-applet/commit/?id=2e3d3351a7208aba55954b2ad25b83843f4c6af7 https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=660bb1a48f78cafb280e72c00c1bf0ea818f1538
Woohoo! Once this gets released, nm-connection-editor users will be able to have secure WPA Enterprise connections after 10 years! Thanks.