From 020dcf5bc1f2d92d4212494bdc7ebb5b994ff8b8 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 1 Dec 2017 11:52:40 +0100 Subject: [PATCH 1/4] ifcfg-rh: persist the wep key type The wireless-security setting has a 'wep-key-type' property that is used to specify the WEP key type and is needed because some keys could be interpreted both as a passphrase or a hex/ascii key. The ifcfg-rh plugin currently stores the key type implicitly: if wep-key-type is 'passphrase' it uses the KEY_PASSPHRASE%d variable, if it's 'key' the KEY%d variable and when it's 'unknown' it uses either variables depending on the detected type (preferring 'key' in case both are compatible). This means that some connections will be read differently from how they were written, because once the KEY (or KEY_PASSPHRASE) is read there is no way to know whether the 'wep-key-type' property was 'key' (or 'passphrase') or 'unknown'. Fix this by persisting the key type explicitly in the file. The new variable is redundant in most cases because the variables used for keys also determine the key type. https://bugzilla.redhat.com/show_bug.cgi?id=1518177 (cherry picked from commit c6eb18ee05170727e55a25eb9897e36d65bc9f13) --- libnm-core/nm-setting-wireless-security.c | 5 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 97 +++++++++++-------- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 19 +++- .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 4 +- 4 files changed, 78 insertions(+), 47 deletions(-) diff --git a/libnm-core/nm-setting-wireless-security.c b/libnm-core/nm-setting-wireless-security.c index 91bcbe7a23..de77a4938d 100644 --- a/libnm-core/nm-setting-wireless-security.c +++ b/libnm-core/nm-setting-wireless-security.c @@ -1814,10 +1814,11 @@ nm_setting_wireless_security_class_init (NMSettingWirelessSecurityClass *setting **/ /* ---ifcfg-rh--- * property: wep-key-type - * variable: KEY or KEY_PASSPHRASE(+) + * variable: KEY or KEY_PASSPHRASE(+); KEY_TYPE(+) * description: KEY is used for "key" type (10 or 26 hexadecimal characters, * or 5 or 13 character string prefixed with "s:"). KEY_PASSPHRASE is used - * for WEP passphrases. + * for WEP passphrases. KEY_TYPE specifies the key type and can be either + * 'key' or 'passphrase'. KEY_TYPE is redundant and can be omitted. * example: KEY1=s:ahoj, KEY1=0a1c45bc02, KEY_PASSPHRASE1=mysupersecretkey * ---end--- */ diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 8ba04e474e..665839fbbf 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2336,9 +2336,8 @@ add_one_wep_key (shvarFile *ifcfg, NMSettingWirelessSecurity *s_wsec, GError **error) { - char *key = NULL; - char *value = NULL; - gboolean success = FALSE; + gs_free char *key = NULL; + gs_free char *value = NULL; g_return_val_if_fail (ifcfg != NULL, FALSE); g_return_val_if_fail (shvar_key != NULL, FALSE); @@ -2351,13 +2350,8 @@ add_one_wep_key (shvarFile *ifcfg, /* Validate keys */ if (passphrase) { - if (strlen (value) && strlen (value) < 64) { + if (strlen (value) && strlen (value) < 64) key = g_strdup (value); - g_object_set (G_OBJECT (s_wsec), - NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, - NM_WEP_KEY_TYPE_PASSPHRASE, - NULL); - } } else { if (strlen (value) == 10 || strlen (value) == 26) { /* Hexadecimal WEP key */ @@ -2367,7 +2361,7 @@ add_one_wep_key (shvarFile *ifcfg, if (!g_ascii_isxdigit (*p)) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Invalid hexadecimal WEP key."); - goto out; + return FALSE; } p++; } @@ -2381,7 +2375,7 @@ add_one_wep_key (shvarFile *ifcfg, if (!g_ascii_isprint ((int) (*p))) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Invalid ASCII WEP key."); - goto out; + return FALSE; } p++; } @@ -2396,47 +2390,47 @@ add_one_wep_key (shvarFile *ifcfg, } } - if (key) { - nm_setting_wireless_security_set_wep_key (s_wsec, key_idx, key); - g_free (key); - success = TRUE; - } else { + if (!key) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, "Invalid WEP key length."); + return FALSE; } -out: - g_free (value); - return success; + nm_setting_wireless_security_set_wep_key (s_wsec, key_idx, key); + + return TRUE; } static gboolean read_wep_keys (shvarFile *ifcfg, + NMWepKeyType key_type, guint8 def_idx, NMSettingWirelessSecurity *s_wsec, GError **error) { - /* Try hex/ascii keys first */ - if (!add_one_wep_key (ifcfg, "KEY1", 0, FALSE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY2", 1, FALSE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY3", 2, FALSE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY4", 3, FALSE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY", def_idx, FALSE, s_wsec, error)) - return FALSE; + if (key_type != NM_WEP_KEY_TYPE_PASSPHRASE) { + if (!add_one_wep_key (ifcfg, "KEY1", 0, FALSE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY2", 1, FALSE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY3", 2, FALSE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY4", 3, FALSE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY", def_idx, FALSE, s_wsec, error)) + return FALSE; + } - /* And then passphrases */ - if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE1", 0, TRUE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE2", 1, TRUE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE3", 2, TRUE, s_wsec, error)) - return FALSE; - if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE4", 3, TRUE, s_wsec, error)) - return FALSE; + if (key_type != NM_WEP_KEY_TYPE_KEY) { + if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE1", 0, TRUE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE2", 1, TRUE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE3", 2, TRUE, s_wsec, error)) + return FALSE; + if (!add_one_wep_key (ifcfg, "KEY_PASSPHRASE4", 3, TRUE, s_wsec, error)) + return FALSE; + } return TRUE; } @@ -2501,19 +2495,40 @@ make_wep_setting (shvarFile *ifcfg, /* Read keys in the ifcfg file if they are system-owned */ if (key_flags == NM_SETTING_SECRET_FLAG_NONE) { - if (!read_wep_keys (ifcfg, default_key_idx, s_wsec, error)) + NMWepKeyType key_type; + const char *v; + gs_free char *to_free = NULL; + + v = svGetValueStr (ifcfg, "KEY_TYPE", &to_free); + if (!v) + key_type = NM_WEP_KEY_TYPE_UNKNOWN; + else if (nm_streq (v, "key")) + key_type = NM_WEP_KEY_TYPE_KEY; + else if (nm_streq (v, "passphrase")) + key_type = NM_WEP_KEY_TYPE_PASSPHRASE; + else { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, + "Invalid KEY_TYPE value '%s'", v); + return FALSE; + } + + if (!read_wep_keys (ifcfg, key_type, default_key_idx, s_wsec, error)) return NULL; /* Try to get keys from the "shadow" key file */ keys_ifcfg = utils_get_keys_ifcfg (file, FALSE); if (keys_ifcfg) { - if (!read_wep_keys (keys_ifcfg, default_key_idx, s_wsec, error)) { + if (!read_wep_keys (keys_ifcfg, key_type, default_key_idx, s_wsec, error)) { svCloseFile (keys_ifcfg); return NULL; } svCloseFile (keys_ifcfg); g_assert (error == NULL || *error == NULL); } + + g_object_set (G_OBJECT (s_wsec), + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, key_type, + NULL); } value = svGetValueStr_cp (ifcfg, "SECURITYMODE"); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 8c3d14b7af..f614376fd5 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -662,12 +662,26 @@ write_wireless_security_setting (NMConnection *connection, /* And write the new ones out */ if (wep) { + NMWepKeyType key_type; + const char *key_type_str = NULL; + /* Default WEP TX key index */ svSetValueInt64 (ifcfg, "DEFAULTKEY", nm_setting_wireless_security_get_wep_tx_keyidx(s_wsec) + 1); - for (i = 0; i < 4; i++) { - NMWepKeyType key_type; + key_type = nm_setting_wireless_security_get_wep_key_type (s_wsec); + switch (key_type) { + case NM_WEP_KEY_TYPE_KEY: + key_type_str = "key"; + break; + case NM_WEP_KEY_TYPE_PASSPHRASE: + key_type_str = "passphrase"; + break; + case NM_WEP_KEY_TYPE_UNKNOWN: + break; + } + svSetValue (ifcfg, "KEY_TYPE", key_type_str); + for (i = 0; i < 4; i++) { key = nm_setting_wireless_security_get_wep_key (s_wsec, i); if (key) { gs_free char *ascii_key = NULL; @@ -678,7 +692,6 @@ write_wireless_security_setting (NMConnection *connection, * are some passphrases that are indistinguishable from WEP hex * keys. */ - key_type = nm_setting_wireless_security_get_wep_key_type (s_wsec); if (key_type == NM_WEP_KEY_TYPE_UNKNOWN) { if (nm_utils_wep_key_valid (key, NM_WEP_KEY_TYPE_KEY)) key_type = NM_WEP_KEY_TYPE_KEY; diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index 07b121b261..284886a7ae 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -2686,7 +2686,7 @@ test_read_wifi_wep_passphrase (void) g_assert (s_wsec); g_assert_cmpstr (nm_setting_wireless_security_get_key_mgmt (s_wsec), ==, "none"); g_assert_cmpint (nm_setting_wireless_security_get_wep_tx_keyidx (s_wsec), ==, 0); - g_assert_cmpint (nm_setting_wireless_security_get_wep_key_type (s_wsec), ==, NM_WEP_KEY_TYPE_PASSPHRASE); + g_assert_cmpint (nm_setting_wireless_security_get_wep_key_type (s_wsec), ==, NM_WEP_KEY_TYPE_UNKNOWN); g_assert_cmpstr (nm_setting_wireless_security_get_wep_key (s_wsec, 0), ==, "foobar222blahblah"); g_assert (!nm_setting_wireless_security_get_wep_key (s_wsec, 1)); g_assert (!nm_setting_wireless_security_get_wep_key (s_wsec, 2)); @@ -5758,6 +5758,7 @@ test_write_wifi_wep_40_ascii (void) g_object_set (s_wsec, NM_SETTING_WIRELESS_SECURITY_KEY_MGMT, "none", NM_SETTING_WIRELESS_SECURITY_WEP_TX_KEYIDX, 2, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, NM_WEP_KEY_TYPE_KEY, NM_SETTING_WIRELESS_SECURITY_AUTH_ALG, "shared", NULL); nm_setting_wireless_security_set_wep_key (s_wsec, 0, "lorem"); @@ -5845,6 +5846,7 @@ test_write_wifi_wep_104_ascii (void) g_object_set (s_wsec, NM_SETTING_WIRELESS_SECURITY_KEY_MGMT, "none", NM_SETTING_WIRELESS_SECURITY_WEP_TX_KEYIDX, 0, + NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, NM_WEP_KEY_TYPE_UNKNOWN, NM_SETTING_WIRELESS_SECURITY_AUTH_ALG, "open", NULL); nm_setting_wireless_security_set_wep_key (s_wsec, 0, "LoremIpsumSit"); From 74c2538bb39333c1aec9f7e32e3ab3db7dcf080c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Dec 2017 11:45:20 +0100 Subject: [PATCH 2/4] ifcfg-rh: use NM_STRCHAR_ANY() macro in add_one_wep_key() (cherry picked from commit da6394d5726012b963cf5610d7a9db0670f32d77) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 665839fbbf..d815e188f0 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2355,29 +2355,19 @@ add_one_wep_key (shvarFile *ifcfg, } else { if (strlen (value) == 10 || strlen (value) == 26) { /* Hexadecimal WEP key */ - char *p = value; - - while (*p) { - if (!g_ascii_isxdigit (*p)) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Invalid hexadecimal WEP key."); - return FALSE; - } - p++; + if (NM_STRCHAR_ANY (value, ch, !g_ascii_isxdigit (ch))) { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, + "Invalid hexadecimal WEP key."); + return FALSE; } key = g_strdup (value); } else if ( !strncmp (value, "s:", 2) && (strlen (value) == 7 || strlen (value) == 15)) { /* ASCII key */ - char *p = value + 2; - - while (*p) { - if (!g_ascii_isprint ((int) (*p))) { - g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, - "Invalid ASCII WEP key."); - return FALSE; - } - p++; + if (NM_STRCHAR_ANY (value + 2, ch, !g_ascii_isprint (ch))) { + g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, + "Invalid ASCII WEP key."); + return FALSE; } /* Remove 's:' prefix. From d94c0e747b17d6de1a6db756e4794884e87bb081 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Dec 2017 11:45:20 +0100 Subject: [PATCH 3/4] ifcfg-rh: use NM_IN_SET() macro in add_one_wep_key() Evaluate strlen() only once. (cherry picked from commit 5a857b39226472380fb42e4424e4f878e0e6f5c0) --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index d815e188f0..8004ae16d0 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2350,10 +2350,10 @@ add_one_wep_key (shvarFile *ifcfg, /* Validate keys */ if (passphrase) { - if (strlen (value) && strlen (value) < 64) + if (value[0] && strlen (value) < 64) key = g_strdup (value); } else { - if (strlen (value) == 10 || strlen (value) == 26) { + if (NM_IN_SET (strlen (value), 10, 26)) { /* Hexadecimal WEP key */ if (NM_STRCHAR_ANY (value, ch, !g_ascii_isxdigit (ch))) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, @@ -2362,7 +2362,7 @@ add_one_wep_key (shvarFile *ifcfg, } key = g_strdup (value); } else if ( !strncmp (value, "s:", 2) - && (strlen (value) == 7 || strlen (value) == 15)) { + && NM_IN_SET (strlen (value), 7, 15)) { /* ASCII key */ if (NM_STRCHAR_ANY (value + 2, ch, !g_ascii_isprint (ch))) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_INVALID_CONNECTION, From 860a4041efa1b9ee4cfe9b70a48884448cde3a87 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 Dec 2017 11:49:03 +0100 Subject: [PATCH 4/4] ifcfg-rh: avoid unnecessary string copies in add_one_wep_key() (cherry picked from commit c3d192b6a3993e09bec75c697a793bef0c8ab475) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 8004ae16d0..121d306e59 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2336,22 +2336,23 @@ add_one_wep_key (shvarFile *ifcfg, NMSettingWirelessSecurity *s_wsec, GError **error) { - gs_free char *key = NULL; - gs_free char *value = NULL; + gs_free char *value_free = NULL; + const char *value; + const char *key = NULL; g_return_val_if_fail (ifcfg != NULL, FALSE); g_return_val_if_fail (shvar_key != NULL, FALSE); g_return_val_if_fail (key_idx <= 3, FALSE); g_return_val_if_fail (s_wsec != NULL, FALSE); - value = svGetValueStr_cp (ifcfg, shvar_key); + value = svGetValueStr (ifcfg, shvar_key, &value_free); if (!value) return TRUE; /* Validate keys */ if (passphrase) { if (value[0] && strlen (value) < 64) - key = g_strdup (value); + key = value; } else { if (NM_IN_SET (strlen (value), 10, 26)) { /* Hexadecimal WEP key */ @@ -2360,7 +2361,7 @@ add_one_wep_key (shvarFile *ifcfg, "Invalid hexadecimal WEP key."); return FALSE; } - key = g_strdup (value); + key = value; } else if ( !strncmp (value, "s:", 2) && NM_IN_SET (strlen (value), 7, 15)) { /* ASCII key */ @@ -2376,7 +2377,7 @@ add_one_wep_key (shvarFile *ifcfg, * before passing to wpa_supplicant, this prevents two unnecessary conversions. And mainly, * ASCII WEP key doesn't change to HEX WEP key in UI, which could confuse users. */ - key = g_strdup (value + 2); + key = value + 2; } } @@ -2387,7 +2388,6 @@ add_one_wep_key (shvarFile *ifcfg, } nm_setting_wireless_security_set_wep_key (s_wsec, key_idx, key); - return TRUE; }