GNOME Bugzilla – Bug 765059
802-1x phase1 setting, disabling tlsv1.1 and tlsv1.2
Last modified: 2017-02-20 13:13:39 UTC
I have been having some trouble connecting to an eduroam network after a wpa_supplicant update. This lead me to bisect wpa_supplicant and the offending commit is [35efa2479ff19c3f13e69dc50d2708ce79a99beb] OpenSSL: Allow TLS v1.1 and v1.2 to be negotiated by default. The commit message contains: 'Use SSLv23_method() to enable TLS version negotiation for any version equal to or newer than 1.0. If the old behavior is needed as a workaround for some broken authentication servers, it can be configured with phase1="tls_disable_tlsv1_1=1 tls_disable_tlsv1_2=1"'. Sure enough adding that to a wpa_supplicant configuration file allows me to connect without problems. I have been looking through networkmanager's documentation [1] but I haven't been able to find an way to add this in the connection configuration file. If possible it would be nice to be able to configure this. Having to add this option manually in the configuration file is enough, as it is easier to add this option (at the cost of security I suppose) than convince the IT department to stop using broken authentication servers. [1] https://developer.gnome.org/NetworkManager/stable/ref-settings.html
Created attachment 343770 [details] [review] Add property to disable tls>1.0 negotiation This patch adds option phase1-tls-disable to NetworkManager profile that allow user to disable tls>1.0 negotiation.
Quoting from IRC: <dcbw> IIRC, I think I was advocating for a bitfield of "allowed TLS versions" or something like that, rather than a blanket disable for > 1.0 <dcbw> for two reasons: 1) at some point TLS 1.2 will be deprecated because somebody will figure out how to hack it, and 2) people may wish to disable 1.0 and 1.1 for security reasons and only use 1.2, regardless of whether the authentication server still supports 1.0/1 <thaller> so the bits would mean whether to allow a certain version or forbid? <dcbw> probably to forbid, since a default of 0x0 (eg, forbid none) is easier than a default of 0xffffffff (allow all) to handle in the D-Bus API <dcbw> plus we want to default to allowing all anyway <dcbw> hey, if it's a int64 (not uint) then we can keep -1 be "global default" and still have 32 bits :) <dcbw> or just make it a comma-separated string, but that's more work <thaller> for NMSettingWiredWakeOnLan we use int32, but still encode there "default" somehow: https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/libnm-core/nm-setting-wired.h#n43 <dcbw> oh true, I guess for a bitfield 0x0 can be "global default" and 0x1 can be "everything"
Created attachment 345079 [details] [review] Adding NMSetting8021xTLSDisable bitfield Currently I'm in the process of adding a bitfield. But encounter this error when building. Any help on solving this is appreciated GISCAN libnm/NM-1.0.gir tmp-introspectr7_xujzl/home/x/Projects/NetworkManager/build/tmp-introspectr7_xujzl/NM-1.0.o:(.data.rel.GI_GET_TYPE_FUNCS_+0xf0): undefined reference to `nm_setting_802_1x_tls_disable_get_type' collect2: error: ld returned 1 exit status libnm-core/nm-setting-bond.c:932: Warning: NM: GObject-Introspection specific GTK-Doc tag "Type" has been deprecated, please use annotations on the identifier instead: * Type: GHashTable(utf8,utf8) ^ libnm-core/nm-setting-vpn.c:882: Warning: NM: GObject-Introspection specific GTK-Doc tag "Type" has been deprecated, please use annotations on the identifier instead: * Type: GHashTable(utf8,utf8) ^ libnm-core/nm-setting-vpn.c:909: Warning: NM: GObject-Introspection specific GTK-Doc tag "Type" has been deprecated, please use annotations on the identifier instead: * Type: GHashTable(utf8,utf8) ^ libnm-core/nm-setting-wired.c:1370: Warning: NM: GObject-Introspection specific GTK-Doc tag "Type" has been deprecated, please use annotations on the identifier instead: * Type: GHashTable(utf8,utf8) ^ libnm/nm-dhcp-config.c:161: Warning: NM: GObject-Introspection specific GTK-Doc tag "Type" has been deprecated, please use annotations on the identifier instead: * Type: GLib.HashTable(utf8,utf8) ^ libnm/nm-secret-agent-old.c:898: Warning: NM: GObject-Introspection specific GTK-Doc tag "Virtual" has been deprecated, please use annotations on the identifier instead: * Virtual: get_secrets ^ libnm/nm-secret-agent-old.c:938: Warning: NM: GObject-Introspection specific GTK-Doc tag "Virtual" has been deprecated, please use annotations on the identifier instead: * Virtual: save_secrets ^ libnm/nm-secret-agent-old.c:967: Warning: NM: GObject-Introspection specific GTK-Doc tag "Virtual" has been deprecated, please use annotations on the identifier instead: * Virtual: delete_secrets ^ linking of temporary binary failed: Command '['/bin/sh', './libtool', '--mode=link', '--tag=CC', '--silent', 'gcc', '-o', '/home/x/Projects/NetworkManager/build/tmp-introspectr7_xujzl/NM-1.0', '-export-dynamic', '-Wall', '-std=gnu99', '-Werror', '-Wshadow', '-Wmissing-declarations', '-Wmissing-prototypes', '-Wdeclaration-after-statement', '-Wfloat-equal', '-Wno-unused-parameter', '-Wno-sign-compare', '-Wstrict-prototypes', '-Wno-unused-but-set-variable', '-Wundef', '-Wimplicit-function-declaration', '-Wpointer-arith', '-Winit-self', '-Wmissing-include-dirs', '-Wno-pragmas', '-g', '-O2', '-Warray-bounds', '-Wunused-value', '-fno-strict-aliasing', '-fdata-sections', '-ffunction-sections', '-Wl,--gc-sections', '-Wno-error', 'tmp-introspectr7_xujzl/home/x/Projects/NetworkManager/build/tmp-introspectr7_xujzl/NM-1.0.o', '-L.', 'libnm/libnm.la', '-Wl,--export-dynamic', '-lgmodule-2.0', '-pthread', '-lgio-2.0', '-lgudev-1.0', '-lgobject-2.0', '-lglib-2.0']' returned non-zero exit status 1. make: *** [/usr/share/gobject-introspection-1.0/Makefile.introspection:156: libnm/NM-1.0.gir] Error 1
(In reply to alaviss0+gnome from comment #3) > Created attachment 345079 [details] [review] [review] > Adding NMSetting8021xTLSDisable bitfield > > Currently I'm in the process of adding a bitfield. But encounter this error > when building. Any help on solving this is appreciated > > GISCAN libnm/NM-1.0.gir > tmp-introspectr7_xujzl/home/x/Projects/NetworkManager/build/tmp- > introspectr7_xujzl/NM-1.0.o:(.data.rel.GI_GET_TYPE_FUNCS_+0xf0): undefined > reference to `nm_setting_802_1x_tls_disable_get_type' The build system automatically generates GEnums for every enum in header files, and a nm_setting_*_get_type() function to retrieve the enum GType. You should add the function to libnm/libnm.ver to export the function in libnm.
(In reply to Beniamino Galvani from comment #4) > (In reply to alaviss0+gnome from comment #3) > > Created attachment 345079 [details] [review] [review] > > Adding NMSetting8021xTLSDisable bitfield > > > > Currently I'm in the process of adding a bitfield. But encounter this error > > when building. Any help on solving this is appreciated > > > > GISCAN libnm/NM-1.0.gir > > tmp-introspectr7_xujzl/home/x/Projects/NetworkManager/build/tmp- > > introspectr7_xujzl/NM-1.0.o:(.data.rel.GI_GET_TYPE_FUNCS_+0xf0): undefined > > reference to `nm_setting_802_1x_tls_disable_get_type' > > The build system automatically generates GEnums for every enum in header > files, and a nm_setting_*_get_type() function to retrieve the enum GType. > You should add the function to libnm/libnm.ver to export the function in > libnm. Thanks for your help.
Created attachment 345170 [details] [review] Another take on adding phase1-tls-disable This patch allows user to disable specific TLS version on phase 1 authentication.
Review of attachment 345170 [details] [review]: Overall a great patch, follows (almost) all the peculiar style. Could you squash the 3rd commit into the first? Basically, just add the new flags and documentation in one step. Maybe rename NMSetting8021xTLSDisable to NMSetting8021xTlsDisable, because acronyms with >=3 characters we do in CamelCase (and it's public API, so it matters more). Should add a NMSetting8021x::verify() function to check that the DEFAULT flag is never used with any of the other flags. Something like https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/libnm-core/nm-setting-wired.c#n731 ::: src/supplicant/nm-supplicant-config.c @@ +940,3 @@ + if (phase1->len) + g_string_append_c (phase1, ' '); + g_string_append_printf(phase1, "tls_disable_tlsv1_0=%d", space between function name and "("
Created attachment 345235 [details] [review] Second version of the patch to add phase1-tls-disable I have squashed docs into the first commit, renamed NMSetting8021xTlsDisable, fixed some styling errors.
looks good to me
> libnm-core/8021x: add phase1-tls-disable configuration item + NM_SETTING_802_1X_TLS_DISABLE_10 = (1 << 1), + NM_SETTING_802_1X_TLS_DISABLE_11 = (1 << 2), + NM_SETTING_802_1X_TLS_DISABLE_12 = (1 << 3), Maybe, add a underscore in the version (no strong opinion about that): + NM_SETTING_802_1X_TLS_DISABLE_1_0 = (1 << 1), + NM_SETTING_802_1X_TLS_DISABLE_1_1 = (1 << 2), + NM_SETTING_802_1X_TLS_DISABLE_1_2 = (1 << 3), + if (!nm_utils_is_power_of_two (priv->phase1_tls_disable)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("TLS disable mode 'default' is an exclusive flag")); Wouldn't this forbid multiple flags as: (NM_SETTING_802_1X_TLS_DISABLE_1_0 | NM_SETTING_802_1X_TLS_DISABLE_1_1) ? The rest looks good! We should also add nmcli and ifcfg-rh support, but this can be done later.
Created attachment 345319 [details] [review] Version 3 on phase1-tls-disable I've fixed up the verify function. It's my bad for mistaking power-of-two with divisible-by-two. Also, I added an underscore between major and minor versions of TLS.
Created attachment 345320 [details] [review] Version 4 My fault, forgot to commit the changes to supplicant.
lgtm now. I only wonder whether we shouldn't make a more generic flags attribute. Currently it's named "NMSetting8021xTlsDisable", hence the name indicates that it is only to *disable* *tls* modes. For example, to support "tls_allow_md5=1", would we call the flag "NM_SETTING_802_1X_TLS_DISABLE_ALLOW_MD5"? Maybe it should be "NMSetting8021xTlsOptFlags" with NM_SETTING_802_1X_TLS_OPT_FLAG_DISABLE_1_* or maybe even not talking about "Tls" in particular. Maybe just "NMSetting8021xAuthFlags" with NM_SETTING_802_1X_AUTH_FLAG_TLS_DISABLE_1_*
Yes, there may well be other things we'd like ot allow users to disable in the future. Like SHA1 which isn't close behind MD5 in being broken. So my vote would be for AUTH_FLAGS_* instead of a TLS-specific set of flags.
Assuming we would go the AUTH_FLAGS_* route. Although now it will only have tls_disable_tlsv1.* implemented. Should phase1-tls-disable now be phase1-auth-opts?
(In reply to Dan Williams from comment #14) > Yes, there may well be other things we'd like ot allow users to disable in > the future. Like SHA1 which isn't close behind MD5 in being broken. > > So my vote would be for AUTH_FLAGS_* instead of a TLS-specific set of flags. Good idea. (In reply to alaviss0+gnome from comment #15) > Assuming we would go the AUTH_FLAGS_* route. Although now it will only have > tls_disable_tlsv1.* implemented. Should phase1-tls-disable now be > phase1-auth-opts? Yes, or phase1-auth-flags.
Created attachment 346075 [details] [review] Version 5: replace phase1-tls-disable with phase1-auth-flags
(In reply to alaviss0+gnome from comment #17) > Created attachment 346075 [details] [review] [review] > Version 5: replace phase1-tls-disable with phase1-auth-flags thanks for v5. It looks pretty good to me now. Rethinking it again :), I think the flags should have a different semantic. To show what I mean, I did a branch. The first 2 patches are your v5 from comment 17, with one follow-up. What do you think? https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=th%2F8021x-auth-flags-tls-disable-bgo765059
(In reply to Thomas Haller from comment #18) > (In reply to alaviss0+gnome from comment #17) > > Created attachment 346075 [details] [review] [review] [review] > > Version 5: replace phase1-tls-disable with phase1-auth-flags > > thanks for v5. It looks pretty good to me now. > > Rethinking it again :), I think the flags should have a different semantic. > To show what I mean, I did a branch. The first 2 patches are your v5 from > comment 17, with one follow-up. > > What do you think? > > https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > ?id=th%2F8021x-auth-flags-tls-disable-bgo765059 I think this looks pretty good to me.
> libnm/wifi: rework NMSetting8021xAuthFlags to selectively choose TLS version The wpa_supplicant API supports to enable/disable each TLS version individually, or leave it at the default. As explained below in the commit message, basically this means that the only meaningful value for the disable options is 1, since all TLS versions are enabled by default. Even the wpa_supplicant.conf documentation seem to imply that only disable=1 makes sense for TLS: # tls_disable_session_ticket=1 - disable TLS Session Ticket extension # tls_disable_session_ticket=0 - allow TLS Session Ticket extension to be used # [...] # tls_disable_tlsv1_0=1 - disable use of TLSv1.0 # tls_disable_tlsv1_1=1 - disable use of TLSv1.1 (a workaround for AAA servers # that have issues interoperating with updated TLS version) # tls_disable_tlsv1_2=1 - disable use of TLSv1.2 (a workaround for AAA servers # that have issues interoperating with updated TLS version) # [...] In my opinion the TLS_1_*_ENABLE flags are not useful at the moment and can be dropped; the meaning of the AUTH_FLAGS_TLS_1_x_DISABLE flag should be: if set, explicitly disable TLS v1.x, otherwise use the default (keep it enabled).
(In reply to Beniamino Galvani from comment #20) > In my opinion the TLS_1_*_ENABLE flags are not useful at the moment > and can be dropped; the meaning of the AUTH_FLAGS_TLS_1_x_DISABLE flag > should be: if set, explicitly disable TLS v1.x, otherwise use the > default (keep it enabled). makes sense. So v7: th/8021x-auth-flags-tls-disable-bgo765059
(In reply to Thomas Haller from comment #21) > (In reply to Beniamino Galvani from comment #20) > > In my opinion the TLS_1_*_ENABLE flags are not useful at the moment > > and can be dropped; the meaning of the AUTH_FLAGS_TLS_1_x_DISABLE flag > > should be: if set, explicitly disable TLS v1.x, otherwise use the > > default (keep it enabled). > > makes sense. So v7: th/8021x-auth-flags-tls-disable-bgo765059 LGTM - pushed other 2 commits
(In reply to Beniamino Galvani from comment #22) > > LGTM - pushed other 2 commits Why do you use g_strdelimit()? Rest looks good
(In reply to Thomas Haller from comment #23) > (In reply to Beniamino Galvani from comment #22) > > > > LGTM - pushed other 2 commits > > Why do you use g_strdelimit()? In the writer, nm_utils_enum_to_str() returns a comma-separated string and the g_strdelimit() is used to replace commas to spaces. In the reader the g_strdelimit() is actually not necessary (pushed a fixup to remove it).
(In reply to Beniamino Galvani from comment #24) > (In reply to Thomas Haller from comment #23) > > (In reply to Beniamino Galvani from comment #22) > > > > > > LGTM - pushed other 2 commits > > > > Why do you use g_strdelimit()? > > In the writer, nm_utils_enum_to_str() returns a comma-separated string and > the g_strdelimit() is used to replace commas to spaces. In the reader the > g_strdelimit() is actually not necessary (pushed a fixup to remove it). well, sure. But why?? :) If you want to use spaces, add an option to nm_utils_enum_to_str() to join values by spaces (*). And if nm_utils_enum_to_str() returns ',', why do you want to use spaces? (*) since nm_utils_enum_to_str() is public API, that actually means to add an internal function ~nm_utils_enum_to_str_full()~.
(In reply to Thomas Haller from comment #25) > If you want to use spaces, add an option to nm_utils_enum_to_str() to join > values by spaces (*). since nm_utils_enum_to_str() is public API, that > actually means to add an internal function ~nm_utils_enum_to_str_full()~. Ok, it just seemed simpler to use the existing function and replace the output. How about the new commits? ("libnm-core: add _nm_utils_enum_to_str_full()" needs to be moved before of course) > And if nm_utils_enum_to_str() returns ',', why do you want to use spaces? Mainly for consistency with other ifcfg variables: $ man nm-settings-ifcfg-rh | grep comma.separated -c 0 $ man nm-settings-ifcfg-rh | grep space.separated -c 4
merged now: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a184c25cb952403441235c9bfbb0915a3f11fab4 Thanks.