From dcd032055ca9997db748139b058b5ea35696233a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Fri, 23 Apr 2021 16:00:52 +0200 Subject: [PATCH 1/5] supplicant/config: Add a comment mentioning global pmf config value It looks a bit weird on the first glance that we do nothing when NM_SETTING_WIRELESS_SECURITY_PMF_OPTIONAL is used. The reason for this is that we already intialize the global option "pmf" of wpa_supplicant to "1" (optional), so add a brief comment about that here. --- src/core/supplicant/nm-supplicant-config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/supplicant/nm-supplicant-config.c b/src/core/supplicant/nm-supplicant-config.c index c216159ca0..844f2a93c9 100644 --- a/src/core/supplicant/nm-supplicant-config.c +++ b/src/core/supplicant/nm-supplicant-config.c @@ -1007,6 +1007,9 @@ nm_supplicant_config_add_setting_wireless_security(NMSupplicantConfig * error)) return FALSE; + /* We set the supplicants global "pmf" config value to "1" (optional), + * so no need to set it network-specific again if PMF_OPTIONAL is set. + */ if (set_pmf && NM_IN_SET(pmf, NM_SETTING_WIRELESS_SECURITY_PMF_DISABLE, From 29c7debf40748791e82e1f20bac95be3add3332e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Fri, 23 Apr 2021 16:08:04 +0200 Subject: [PATCH 2/5] supplicant/config: Remove superfluous check to disable PMF We only set the "ieee80211w" option in the wpa_supplicant config in case we're using WPA (see the if-block underneath), otherwise the value of "pmf" is completely ignored. That means the override here (in case WPA isn't used) isn't getting applied anyway, so just remove it. --- src/core/supplicant/nm-supplicant-config.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/core/supplicant/nm-supplicant-config.c b/src/core/supplicant/nm-supplicant-config.c index 844f2a93c9..2af04bbd55 100644 --- a/src/core/supplicant/nm-supplicant-config.c +++ b/src/core/supplicant/nm-supplicant-config.c @@ -954,10 +954,6 @@ nm_supplicant_config_add_setting_wireless_security(NMSupplicantConfig * } } - /* Don't try to enable PMF on non-WPA/SAE/OWE networks */ - if (!NM_IN_STRSET(key_mgmt, "wpa-eap", "wpa-eap-suite-b-192", "wpa-psk", "sae", "owe")) - pmf = NM_SETTING_WIRELESS_SECURITY_PMF_DISABLE; - /* Check if we actually support PMF */ set_pmf = TRUE; if (!_get_capability(priv, NM_SUPPL_CAP_TYPE_PMF)) { From 97a49430bb41d4ccbe4b8a5134748838d30b4a51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Fri, 23 Apr 2021 16:23:03 +0200 Subject: [PATCH 3/5] libnm-core: Correct an error message wpa-eap-suite-b-192 is also valid here, so mention it in the error message. --- src/libnm-core-impl/nm-setting-wireless-security.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wireless-security.c b/src/libnm-core-impl/nm-setting-wireless-security.c index 6104aea575..e5e6b979f4 100644 --- a/src/libnm-core-impl/nm-setting-wireless-security.c +++ b/src/libnm-core-impl/nm-setting-wireless-security.c @@ -1104,13 +1104,13 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) "wpa-psk", "sae", "owe")) { - g_set_error( - error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' can only be used with 'wpa-eap', 'wpa-eap-suite-b-192', 'wpa-psk' or 'sae' key " - "management "), - priv->pmf == NM_SETTING_WIRELESS_SECURITY_PMF_OPTIONAL ? "optional" : "required"); + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' can only be used with 'owe', 'wpa-psk', 'sae', 'wpa-eap' " + "or 'wpa-eap-suite-b-192' key management"), + priv->pmf == NM_SETTING_WIRELESS_SECURITY_PMF_OPTIONAL ? "optional" + : "required"); g_prefix_error(error, "%s.%s: ", NM_SETTING_WIRELESS_SECURITY_SETTING_NAME, From aab56adeeace5b8b905519d43e600565a2b45fe0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Fri, 23 Apr 2021 16:33:20 +0200 Subject: [PATCH 4/5] libnm-core: Correctly check for "sae" or "none" when wifi mesh is used A small bug sneaked into commit 3ef3733c8139 ('wireless-security: ensure Mesh networks can't use anything but SAE') during review: Instead of allowing only "sae" or "none" as key-mgmt, we now disallow "sae" and "none", but allow anything else. This is obviously not what was intended, so fix the check. Also move the valid_key_mgmt check back up to where it was before that commit, it seems we want to apply that check in all cases. --- .../nm-setting-wireless-security.c | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wireless-security.c b/src/libnm-core-impl/nm-setting-wireless-security.c index e5e6b979f4..47a9ca3286 100644 --- a/src/libnm-core-impl/nm-setting-wireless-security.c +++ b/src/libnm-core-impl/nm-setting-wireless-security.c @@ -904,33 +904,32 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } - if (g_strcmp0(wifi_mode, NM_SETTING_WIRELESS_MODE_MESH) == 0) { - if ((strcmp(priv->key_mgmt, "none") == 0) || (strcmp(priv->key_mgmt, "sae") == 0)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for '%s' mode connections"), - priv->key_mgmt, - NM_SETTING_WIRELESS_MODE_MESH); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_WIRELESS_SECURITY_SETTING_NAME, - NM_SETTING_WIRELESS_SECURITY_KEY_MGMT); - return FALSE; - } - } else { - if (!g_strv_contains(valid_key_mgmt, priv->key_mgmt)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid value for the property"), - priv->key_mgmt); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_WIRELESS_SECURITY_SETTING_NAME, - NM_SETTING_WIRELESS_SECURITY_KEY_MGMT); - return FALSE; - } + if (!g_strv_contains(valid_key_mgmt, priv->key_mgmt)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for the property"), + priv->key_mgmt); + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_WIRELESS_SECURITY_SETTING_NAME, + NM_SETTING_WIRELESS_SECURITY_KEY_MGMT); + return FALSE; + } + + if (NM_IN_STRSET(wifi_mode, NM_SETTING_WIRELESS_MODE_MESH) + && !NM_IN_STRSET(priv->key_mgmt, "none", "sae")) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid value for '%s' mode connections"), + priv->key_mgmt, + NM_SETTING_WIRELESS_MODE_MESH); + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_WIRELESS_SECURITY_SETTING_NAME, + NM_SETTING_WIRELESS_SECURITY_KEY_MGMT); + return FALSE; } if (priv->auth_alg && !strcmp(priv->auth_alg, "leap")) { From b876e76518b319a31d87abea7f9ba60bdd366359 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Fri, 23 Apr 2021 17:10:43 +0200 Subject: [PATCH 5/5] supplicant/config: Make sure PMF gets enabled with wpa-eap-suite-b-192 wpa-eap-suite-b-192 key-mgmt method uses special values for "pairwise" and "group" ciphers, we can also handle that a few lines underneath where those are set to make this a bit easier to read. We currently set the supplicants PMF config (ieee80211w) inside an if block that tries to detect whether WPA is used. That if-block doesn't include the "wpa-eap-suite-b-192" case because we want special "pairwise" and "group" handling for wpa-eap-suite-b-192. This means we're currently missing to enable PMF in the "wpa-eap-suite-b-192" case, even though it's set to REQUIRED. Fix it by moving the "pairwise" and "group" special-casing down a bit so we can include "wpa-eap-suite-b-192" in the "Only WPA-specific things when using WPA" check, that will make sure ieee80211w gets set in the wpa-eap-suite-b-192 case. --- src/core/supplicant/nm-supplicant-config.c | 57 ++++++++++--------- .../supplicant/tests/test-supplicant-config.c | 3 +- 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/src/core/supplicant/nm-supplicant-config.c b/src/core/supplicant/nm-supplicant-config.c index 2af04bbd55..bb6cb6c44a 100644 --- a/src/core/supplicant/nm-supplicant-config.c +++ b/src/core/supplicant/nm-supplicant-config.c @@ -883,9 +883,6 @@ nm_supplicant_config_add_setting_wireless_security(NMSupplicantConfig * g_string_append(key_mgmt_conf, " ft-sae"); } else if (nm_streq(key_mgmt, "wpa-eap-suite-b-192")) { pmf = NM_SETTING_WIRELESS_SECURITY_PMF_REQUIRED; - if (!nm_supplicant_config_add_option(self, "pairwise", "GCMP-256", -1, NULL, error) - || !nm_supplicant_config_add_option(self, "group", "GCMP-256", -1, NULL, error)) - return FALSE; } if (!add_string_val(self, key_mgmt_conf->str, "key_mgmt", TRUE, NULL, error)) @@ -968,7 +965,7 @@ nm_supplicant_config_add_setting_wireless_security(NMSupplicantConfig * } /* Only WPA-specific things when using WPA */ - if (NM_IN_STRSET(key_mgmt, "wpa-psk", "wpa-eap", "sae", "owe")) { + if (NM_IN_STRSET(key_mgmt, "owe", "wpa-psk", "sae", "wpa-eap", "wpa-eap-suite-b-192")) { if (!ADD_STRING_LIST_VAL(self, setting, wireless_security, @@ -980,28 +977,36 @@ nm_supplicant_config_add_setting_wireless_security(NMSupplicantConfig * NULL, error)) return FALSE; - if (!ADD_STRING_LIST_VAL(self, - setting, - wireless_security, - pairwise, - pairwise, - "pairwise", - ' ', - TRUE, - NULL, - error)) - return FALSE; - if (!ADD_STRING_LIST_VAL(self, - setting, - wireless_security, - group, - groups, - "group", - ' ', - TRUE, - NULL, - error)) - return FALSE; + + if (nm_streq(key_mgmt, "wpa-eap-suite-b-192")) { + if (!nm_supplicant_config_add_option(self, "pairwise", "GCMP-256", -1, NULL, error)) + return FALSE; + if (!nm_supplicant_config_add_option(self, "group", "GCMP-256", -1, NULL, error)) + return FALSE; + } else { + if (!ADD_STRING_LIST_VAL(self, + setting, + wireless_security, + pairwise, + pairwise, + "pairwise", + ' ', + TRUE, + NULL, + error)) + return FALSE; + if (!ADD_STRING_LIST_VAL(self, + setting, + wireless_security, + group, + groups, + "group", + ' ', + TRUE, + NULL, + error)) + return FALSE; + } /* We set the supplicants global "pmf" config value to "1" (optional), * so no need to set it network-specific again if PMF_OPTIONAL is set. diff --git a/src/core/supplicant/tests/test-supplicant-config.c b/src/core/supplicant/tests/test-supplicant-config.c index 2c2d9478e2..3525f99962 100644 --- a/src/core/supplicant/tests/test-supplicant-config.c +++ b/src/core/supplicant/tests/test-supplicant-config.c @@ -815,9 +815,10 @@ test_wifi_eap_suite_b_generation(void) NMTST_EXPECT_NM_INFO("Config: added 'scan_ssid' value '1'*"); NMTST_EXPECT_NM_INFO("Config: added 'bssid' value '11:22:33:44:55:66'*"); NMTST_EXPECT_NM_INFO("Config: added 'freq_list' value *"); + NMTST_EXPECT_NM_INFO("Config: added 'key_mgmt' value 'WPA-EAP-SUITE-B-192'"); NMTST_EXPECT_NM_INFO("Config: added 'pairwise' value 'GCMP-256'"); NMTST_EXPECT_NM_INFO("Config: added 'group' value 'GCMP-256'"); - NMTST_EXPECT_NM_INFO("Config: added 'key_mgmt' value 'WPA-EAP-SUITE-B-192'"); + NMTST_EXPECT_NM_INFO("Config: added 'ieee80211w' value '2'"); NMTST_EXPECT_NM_INFO("Config: added 'eap' value 'TLS'"); NMTST_EXPECT_NM_INFO("Config: added 'fragment_size' value '1086'"); NMTST_EXPECT_NM_INFO("Config: added 'ca_cert' value '*/test-ca-cert.pem'");