From 5fe447d4a6f7acdc4a54d1720ddaa225b5d87031 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 09:17:19 +0200 Subject: shared: assert that nm_utils_buf_utf8safe_unescape() doesn't reallocate memory We want to use the function to unescape (compress) secrets. As such, we want to be sure that no secrets are leaked in memory due to growing the buffer with realloc. In fact, reallocation should never happen. Assert for that. As reallocation cannot happen, we could directly fill a buffer with API like nm_utils_strbuf_*(). But NMStrBuf has low overhead even in this case. --- shared/nm-glib-aux/nm-shared-utils.c | 15 +++++++++++++-- shared/nm-glib-aux/nm-shared-utils.h | 10 ++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index e64ef71378..56729a3b99 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -2480,7 +2480,7 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr return str; } - nm_str_buf_init (&strbuf, len, FALSE); + nm_str_buf_init (&strbuf, len + 1u, FALSE); nm_str_buf_append_len (&strbuf, str, s - str); str = s; @@ -2541,6 +2541,11 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr str = s; } + /* assert that no reallocation was necessary. For one, unescaping should + * never result in a longer string than the input. Also, when unescaping + * secrets, we want to ensure that we don't leak secrets in memory. */ + nm_assert (strbuf.allocated == len + 1u); + return (*to_free = nm_str_buf_finalize (&strbuf, out_len)); } @@ -2675,11 +2680,17 @@ nm_utils_buf_utf8safe_escape_bytes (GBytes *bytes, NMUtilsStrUtf8SafeFlags flags const char * nm_utils_str_utf8safe_unescape (const char *str, char **to_free) { + const char *res; gsize len; g_return_val_if_fail (to_free, NULL); - return nm_utils_buf_utf8safe_unescape (str, &len, (gpointer *) to_free); + res = nm_utils_buf_utf8safe_unescape (str, &len, (gpointer *) to_free); + + nm_assert ( (!res && len == 0) + || (strlen (res) <= len)); + + return res; } /** diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index fb9e3ac903..fde1499ab3 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1184,8 +1184,18 @@ GType nm_g_type_find_implementing_class_for_property (GType gtype, typedef enum { NM_UTILS_STR_UTF8_SAFE_FLAG_NONE = 0, + + /* This flag only has an effect during escaping. */ NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL = 0x0001, + + /* This flag only has an effect during escaping. */ NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII = 0x0002, + + /* This flag only has an effect during escaping to ensure we + * don't leak secrets in memory. Note that during unescape we + * know the maximum result size from the beginning, and no + * reallocation happens. Thus, unescape always avoids leaking + * secrets already. */ NM_UTILS_STR_UTF8_SAFE_FLAG_SECRET = 0x0004, } NMUtilsStrUtf8SafeFlags; -- cgit v1.2.1 From 0f22f77b1c1f871b0ae85f292e68324413283d06 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 09:52:16 +0200 Subject: shared: support stripping whitespace from nm_utils_buf_utf8safe_unescape() When parsing user input if is often convenient to allow stripping whitespace. Especially with escaped strings, the user could still escape the whitespace, if the space should be taken literally. Add support for that to nm_utils_buf_utf8safe_unescape(). Note that this is not the same as calling g_strstrip() before/after unescape. That is, because nm_utils_buf_utf8safe_unescape() correctly preserves escaped whitespace. If you call g_strstrip() before/after the unescape, you don't know whether the whitespace is escaped. --- libnm-core/nm-setting-ip-config.c | 2 +- libnm-core/nm-utils.c | 2 +- libnm-core/tests/test-general.c | 12 ++++++------ libnm/nm-libnm-utils.c | 2 +- shared/nm-glib-aux/nm-shared-utils.c | 37 ++++++++++++++++++++++++++++-------- shared/nm-glib-aux/nm-shared-utils.h | 14 +++++++++++--- src/nm-config.c | 2 +- 7 files changed, 50 insertions(+), 21 deletions(-) diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index 36f09b694b..09a3428362 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -2186,7 +2186,7 @@ nm_ip_routing_rule_get_xifname_bin (const NMIPRoutingRule *self, if (!xifname) return FALSE; - bin = nm_utils_buf_utf8safe_unescape (xifname, &len, &bin_to_free); + bin = nm_utils_buf_utf8safe_unescape (xifname, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &len, &bin_to_free); strncpy (out_xifname, bin, 16 /* IFNAMSIZ */); out_xifname[15] = '\0'; diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 7b69b4ef7a..f9bccea930 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4673,7 +4673,7 @@ nm_utils_is_valid_iface_name_utf8safe (const char *utf8safe_name) g_return_val_if_fail (utf8safe_name, FALSE); - bin = nm_utils_buf_utf8safe_unescape (utf8safe_name, &len, &bin_to_free); + bin = nm_utils_buf_utf8safe_unescape (utf8safe_name, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &len, &bin_to_free); if (bin_to_free) { /* some unescaping happened... */ diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 7da2479647..52f05dc306 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -7815,7 +7815,7 @@ _do_test_utils_str_utf8safe_unescape (const char *str, const char *expected, gsi gs_free gpointer buf_free_1 = NULL; gs_free char *str_free_1 = NULL; - s = nm_utils_buf_utf8safe_unescape (str, &l, &buf_free_1); + s = nm_utils_buf_utf8safe_unescape (str, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &l, &buf_free_1); g_assert_cmpint (expected_len, ==, l); g_assert_cmpstr (s, ==, expected); @@ -7839,7 +7839,7 @@ _do_test_utils_str_utf8safe_unescape (const char *str, const char *expected, gsi if ( expected && l == strlen (expected)) { /* there are no embeeded NULs. Check that nm_utils_str_utf8safe_unescape() yields the same result. */ - s = nm_utils_str_utf8safe_unescape (str, &str_free_1); + s = nm_utils_str_utf8safe_unescape (str, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &str_free_1); g_assert_cmpstr (s, ==, expected); if (strchr (str, '\\')) { g_assert (str_free_1 != str); @@ -7908,10 +7908,10 @@ _do_test_utils_str_utf8safe (const char *str, gsize str_len, const char *expecte g_assert (g_utf8_validate (str, -1, NULL)); } - g_assert (str == nm_utils_str_utf8safe_unescape (str_safe, &str_free_4)); + g_assert (str == nm_utils_str_utf8safe_unescape (str_safe, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &str_free_4)); g_assert (!str_free_4); - str_free_5 = nm_utils_str_utf8safe_unescape_cp (str_safe); + str_free_5 = nm_utils_str_utf8safe_unescape_cp (str_safe, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE); if (str) { g_assert (str_free_5 != str); g_assert_cmpstr (str_free_5, ==, str); @@ -7935,11 +7935,11 @@ _do_test_utils_str_utf8safe (const char *str, gsize str_len, const char *expecte str_free_6 = g_strcompress (str_safe); g_assert_cmpstr (str, ==, str_free_6); - str_free_7 = nm_utils_str_utf8safe_unescape_cp (str_safe); + str_free_7 = nm_utils_str_utf8safe_unescape_cp (str_safe, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE); g_assert (str_free_7 != str); g_assert_cmpstr (str_free_7, ==, str); - s = nm_utils_str_utf8safe_unescape (str_safe, &str_free_8); + s = nm_utils_str_utf8safe_unescape (str_safe, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE, &str_free_8); g_assert (str_free_8 != str); g_assert (s == str_free_8); g_assert_cmpstr (str_free_8, ==, str); diff --git a/libnm/nm-libnm-utils.c b/libnm/nm-libnm-utils.c index 7b4db11ef3..f8d1ff560c 100644 --- a/libnm/nm-libnm-utils.c +++ b/libnm/nm-libnm-utils.c @@ -139,7 +139,7 @@ _fixup_string (const char *desc, return NULL; /* restore original non-UTF-8-safe text. */ - desc_full = nm_utils_str_utf8safe_unescape_cp (desc); + desc_full = nm_utils_str_utf8safe_unescape_cp (desc, NM_UTILS_STR_UTF8_SAFE_FLAG_NONE); /* replace all invalid UTF-8 bytes with space. */ p = desc_full; diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 56729a3b99..96b09b16d1 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -2456,11 +2456,12 @@ _str_buf_append_c_escape_octal (NMStrBuf *strbuf, } gconstpointer -nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_free) +nm_utils_buf_utf8safe_unescape (const char *str, NMUtilsStrUtf8SafeFlags flags, gsize *out_len, gpointer *to_free) { + gboolean strip_spaces = NM_FLAGS_HAS (flags, NM_UTILS_STR_UTF8_SAFE_UNESCAPE_STRIP_SPACES); NMStrBuf strbuf; - gsize len; const char *s; + gsize len; g_return_val_if_fail (to_free, NULL); g_return_val_if_fail (out_len, NULL); @@ -2471,10 +2472,23 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr return NULL; } + if (strip_spaces) + str = nm_str_skip_leading_spaces (str); + len = strlen (str); s = memchr (str, '\\', len); if (!s) { + if ( strip_spaces + && len > 0 + && g_ascii_isspace (str[len - 1])) { + len--; + while ( len > 0 + && g_ascii_isspace (str[len - 1])) + len--; + *out_len = len; + return (*to_free = g_strndup (str, len)); + } *out_len = len; *to_free = NULL; return str; @@ -2494,7 +2508,7 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr ch = (++str)[0]; if (ch == '\0') { - // error. Trailing '\\' + /* error. Trailing '\\' */ break; } @@ -2533,7 +2547,14 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr s = strchr (str, '\\'); if (!s) { - nm_str_buf_append (&strbuf, str); + gsize l = strlen (str); + + if (strip_spaces) { + while ( l > 0 + && g_ascii_isspace (str[l - 1])) + l--; + } + nm_str_buf_append_len (&strbuf, str, l); break; } @@ -2678,14 +2699,14 @@ nm_utils_buf_utf8safe_escape_bytes (GBytes *bytes, NMUtilsStrUtf8SafeFlags flags /*****************************************************************************/ const char * -nm_utils_str_utf8safe_unescape (const char *str, char **to_free) +nm_utils_str_utf8safe_unescape (const char *str, NMUtilsStrUtf8SafeFlags flags, char **to_free) { const char *res; gsize len; g_return_val_if_fail (to_free, NULL); - res = nm_utils_buf_utf8safe_unescape (str, &len, (gpointer *) to_free); + res = nm_utils_buf_utf8safe_unescape (str, flags, &len, (gpointer *) to_free); nm_assert ( (!res && len == 0) || (strlen (res) <= len)); @@ -2748,11 +2769,11 @@ nm_utils_str_utf8safe_escape_cp (const char *str, NMUtilsStrUtf8SafeFlags flags) } char * -nm_utils_str_utf8safe_unescape_cp (const char *str) +nm_utils_str_utf8safe_unescape_cp (const char *str, NMUtilsStrUtf8SafeFlags flags) { char *s; - str = nm_utils_str_utf8safe_unescape (str, &s); + str = nm_utils_str_utf8safe_unescape (str, flags, &s); return s ?: g_strdup (str); } diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index fde1499ab3..a2530efafb 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1197,17 +1197,25 @@ typedef enum { * reallocation happens. Thus, unescape always avoids leaking * secrets already. */ NM_UTILS_STR_UTF8_SAFE_FLAG_SECRET = 0x0004, + + /* This flag only has an effect during unescaping. It means + * that non-escaped whitespaces (g_ascii_isspace()) will be + * stripped from the front and end of the string. Note that + * this flag is only useful for gracefully accepting user input + * with spaces. With this flag, escape and unescape may no longer + * yield the original input. */ + NM_UTILS_STR_UTF8_SAFE_UNESCAPE_STRIP_SPACES = 0x0008, } NMUtilsStrUtf8SafeFlags; const char *nm_utils_buf_utf8safe_escape (gconstpointer buf, gssize buflen, NMUtilsStrUtf8SafeFlags flags, char **to_free); const char *nm_utils_buf_utf8safe_escape_bytes (GBytes *bytes, NMUtilsStrUtf8SafeFlags flags, char **to_free); -gconstpointer nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_free); +gconstpointer nm_utils_buf_utf8safe_unescape (const char *str, NMUtilsStrUtf8SafeFlags flags, gsize *out_len, gpointer *to_free); const char *nm_utils_str_utf8safe_escape (const char *str, NMUtilsStrUtf8SafeFlags flags, char **to_free); -const char *nm_utils_str_utf8safe_unescape (const char *str, char **to_free); +const char *nm_utils_str_utf8safe_unescape (const char *str, NMUtilsStrUtf8SafeFlags flags, char **to_free); char *nm_utils_str_utf8safe_escape_cp (const char *str, NMUtilsStrUtf8SafeFlags flags); -char *nm_utils_str_utf8safe_unescape_cp (const char *str); +char *nm_utils_str_utf8safe_unescape_cp (const char *str, NMUtilsStrUtf8SafeFlags flags); char *nm_utils_str_utf8safe_escape_take (char *str, NMUtilsStrUtf8SafeFlags flags); diff --git a/src/nm-config.c b/src/nm-config.c index d638059588..4fa8531941 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -344,7 +344,7 @@ no_auto_default_from_file (const char *no_auto_default_file) if (list) { for (i = 0; list[i]; i++) - list[i] = nm_utils_str_utf8safe_unescape_cp (list[i]); + list[i] = nm_utils_str_utf8safe_unescape_cp (list[i], NM_UTILS_STR_UTF8_SAFE_FLAG_NONE); } /* The returned buffer here is not at all compact. That means, it has additional -- cgit v1.2.1 From bb19f6e29cfef535665564d5150ed9b4ce025591 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 17:14:56 +0200 Subject: shared: add NM_UTILS_NAMED_VALUE_INIT() macro --- shared/nm-glib-aux/nm-shared-utils.c | 3 +++ shared/nm-glib-aux/nm-shared-utils.h | 2 ++ 2 files changed, 5 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 96b09b16d1..39e75a89fe 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -17,6 +17,9 @@ #include "nm-errno.h" #include "nm-str-buf.h" +G_STATIC_ASSERT (sizeof (NMUtilsNamedEntry) == sizeof (const char *)); +G_STATIC_ASSERT (G_STRUCT_OFFSET (NMUtilsNamedValue, value_ptr) == sizeof (const char *)); + /*****************************************************************************/ const void *const _NM_PTRARRAY_EMPTY[1] = { NULL }; diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index a2530efafb..cdf6729076 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1407,6 +1407,8 @@ typedef struct { }; } NMUtilsNamedValue; +#define NM_UTILS_NAMED_VALUE_INIT(n, v) { .name = (n), .value_ptr = (v) } + NMUtilsNamedValue *nm_utils_named_values_from_str_dict_with_sort (GHashTable *hash, guint *out_len, GCompareDataFunc compare_func, -- cgit v1.2.1 From 2285dd38ead5d46a5b81efd3657769555952dce2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 09:04:20 +0200 Subject: cli: support backslash escaping in passwd-file Rework parsing of nmcli's passwd-file. 1) support backslash escaping of secrets. - only the secret can be backslash escaped, the property and setting name cannot. This is a change in behavior for passwd-files with secrets that contain a backslash. 2) strip the white space around the secret. This is a change in behavior for secrets that had leading or trailing spaces. Note that you can backslash escape spaces in secrets. 3) strip white space around the setting.property key. This is also a change in behavior, but such keys would never have been valid previously (or the caller would have performed the same kind of stripping). 4) accept '=' as alternative delimiter beside ':'. The ':' feels really odd and unexpected. Also accept '='. This is a change in behavior if keys would contain '=', which they really shouldn't. 5) reject non-UTF-8 secrets and keys. For keys, that is not an issue, because such keys were never valid. For secrets, it probably didn't work anyway to specify non-UTF-8 secrets, because most (if not all) secrets are transmitted via D-Bus as strings where arbitrary binary is not allowed. 6) ignore empty lines and lines starting with '#'. 7) ensure we don't leak any secrets in memory. 1) to 4) are changes in behavior. 3) and 4) seem less severe, as they only concern unusual setting.property keys, which really shouldn't be used (although, VPN secrets can have almost arbitrary names *sigh*). 1) and 2) is more dangerous, as it changes behavior for secrets that contain backslashes or leading/trailing white space. --- clients/cli/connections.c | 152 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 118 insertions(+), 34 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index e6d6c69905..d4f17cc5dc 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -16,6 +16,8 @@ #include #include +#include "nm-glib-aux/nm-secret-utils.h" +#include "nm-glib-aux/nm-io-utils.h" #include "nm-client-utils.h" #include "nm-vpn-helpers.h" #include "nm-meta-setting-access.h" @@ -2710,70 +2712,152 @@ activate_connection_cb (GObject *client, GAsyncResult *result, gpointer user_dat static GHashTable * parse_passwords (const char *passwd_file, GError **error) { + nm_auto_clear_secret_ptr NMSecretPtr contents = { 0 }; gs_unref_hashtable GHashTable *pwds_hash = NULL; - gs_free char *contents = NULL; - gsize len = 0; - GError *local_err = NULL; - gs_free const char **strv = NULL; - const char *const*iter; - char *pwd_spec, *pwd, *prop; - const char *setting; + gs_free_error GError *local = NULL; + const char *contents_str; + gsize contents_line; - pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, g_free); + pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); if (!passwd_file) return g_steal_pointer (&pwds_hash); - if (!g_file_get_contents (passwd_file, &contents, &len, &local_err)) { + if (!nm_utils_file_get_contents (-1, + passwd_file, + 1024*1024, + NM_UTILS_FILE_GET_CONTENTS_FLAG_SECRET, + &contents.str, + &contents.len, + NULL, + &local)) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, _("failed to read passwd-file '%s': %s"), - passwd_file, local_err->message); - g_error_free (local_err); + passwd_file, local->message); return NULL; } - strv = nm_utils_strsplit_set (contents, "\r\n"); - for (iter = strv; strv && *iter; iter++) { - gs_free char *iter_s = g_strdup (*iter); + contents_str = contents.str; + contents_line = 0; + while (contents_str[0]) { + nm_auto_free_secret char *l_hash_key = NULL; + nm_auto_free_secret char *l_hash_val = NULL; + const char *l_content_line; + const char *l_setting; + const char *l_prop; + const char *l_val; + const char *s; + gsize l_hash_val_len; + + /* consume first line. As line delimiters we accept "\r\n", "\n", and "\r". */ + l_content_line = contents_str; + s = l_content_line; + while (!NM_IN_SET (s[0], '\0', '\r', '\n')) + s++; + if (s[0] != '\0') { + if ( s[0] == '\r' + && s[1] == '\n') { + ((char *) s)[0] = '\0'; + s += 2; + } else { + ((char *) s)[0] = '\0'; + s += 1; + } + } + contents_str = s; + contents_line++; + + l_content_line = nm_str_skip_leading_spaces (l_content_line); + if (NM_IN_SET (l_content_line[0], '\0', '#')) { + /* a comment or empty line. Ignore. */ + continue; + } + + l_setting = l_content_line; + + s = l_setting; + while (!NM_IN_SET (s[0], '\0', ':', '=')) + s++; + if (s[0] == '\0') { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("missing colon in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + return NULL; + } + ((char *) s)[0] = '\0'; + s++; + + l_val = s; + + g_strchomp ((char *) l_setting); - pwd = strchr (iter_s, ':'); - if (!pwd) { + nm_assert (nm_str_is_stripped (l_setting)); + + s = strchr (l_setting, '.'); + if (!s) { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("missing dot in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + return NULL; + } else if (s == l_setting) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing colon in 'password' entry '%s'"), *iter); + _("missing setting name in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); return NULL; } - *(pwd++) = '\0'; + ((char *) s)[0] = '\0'; + s++; - prop = strchr (iter_s, '.'); - if (!prop) { + l_prop = s; + if (l_prop[0] == '\0') { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing dot in 'password' entry '%s'"), *iter); + _("missing property name in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); return NULL; } - *(prop++) = '\0'; - setting = iter_s; - while (g_ascii_isspace (*setting)) - setting++; /* Accept wifi-sec or wifi instead of cumbersome '802-11-wireless-security' */ - if (!strcmp (setting, "wifi-sec") || !strcmp (setting, "wifi")) - setting = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME; - if (nm_setting_lookup_type (setting) == G_TYPE_INVALID) { + if (NM_IN_STRSET (l_setting, "wifi-sec", "wifi")) + l_setting = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME; + + if (nm_setting_lookup_type (l_setting) == G_TYPE_INVALID) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("invalid setting name in 'password' entry '%s'"), setting); + _("invalid setting name in 'password' entry '%s', line %zu"), passwd_file, contents_line); return NULL; } - if ( nm_streq (setting, "vpn") - && g_str_has_prefix (prop, "secret.")) { + if ( nm_streq (l_setting, "vpn") + && NM_STR_HAS_PREFIX (l_prop, "secret.")) { /* in 1.12.0, we wrongly required the VPN secrets to be named * "vpn.secret". It should be "vpn.secrets". Work around it * (rh#1628833). */ - pwd_spec = g_strdup_printf ("vpn.secrets.%s", &prop[NM_STRLEN ("secret.")]); + l_hash_key = g_strdup_printf ("vpn.secrets.%s", &l_prop[NM_STRLEN ("secret.")]); } else - pwd_spec = g_strdup_printf ("%s.%s", setting, prop); + l_hash_key = g_strdup_printf ("%s.%s", l_setting, l_prop); + + if (!g_utf8_validate (l_hash_key, -1, NULL)) { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("invalid non UTF-8 setting name in 'password' entry '%s', line %zu"), passwd_file, contents_line); + return NULL; + } + + /* Support backslash escaping in the secret value. We strip non-escaped leading/trailing whitespaces. */ + s = nm_utils_buf_utf8safe_unescape (l_val, NM_UTILS_STR_UTF8_SAFE_UNESCAPE_STRIP_SPACES, &l_hash_val_len, (gpointer *) &l_hash_val); + if (!l_hash_val) + l_hash_val = g_strdup (s); + + if (!g_utf8_validate (l_hash_val, -1, NULL)) { + /* In some cases it might make sense to support binary secrets (like the WPA-PSK which has no + * defined encoding. However, all API that follows can only handle UTF-8, and no mechanism + * to escape the secrets. Reject non-UTF-8 early. */ + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("invalid non UTF-8 secret in 'password' entry '%s', line %zu"), passwd_file, contents_line); + return NULL; + } + + if (strlen (l_hash_val) != l_hash_val_len) { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("invalid non UTF-8 secret with NUL bytes in 'password' entry '%s', line %zu"), passwd_file, contents_line); + return NULL; + } - g_hash_table_insert (pwds_hash, pwd_spec, g_strdup (pwd)); + g_hash_table_insert (pwds_hash, g_steal_pointer (&l_hash_key), g_steal_pointer (&l_hash_val)); } return g_steal_pointer (&pwds_hash); -- cgit v1.2.1 From 1086a47cdae5b1e10c7fcf4a4f8242381027f6a1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 16:25:53 +0200 Subject: cli: refactor error handling in parse_passwords() --- clients/cli/connections.c | 79 +++++++++++++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index d4f17cc5dc..b2dd0a60a7 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -2699,6 +2699,8 @@ activate_connection_cb (GObject *client, GAsyncResult *result, gpointer user_dat /** * parse_passwords: * @passwd_file: file with passwords to parse + * @out_error_line: returns in case of a syntax error in the file, the line + * on which it occurred. * @error: location to store error, or %NULL * * Parse passwords given in @passwd_file and insert them into a hash table. @@ -2710,16 +2712,19 @@ activate_connection_cb (GObject *client, GAsyncResult *result, gpointer user_dat * Returns: hash table with parsed passwords, or %NULL on an error */ static GHashTable * -parse_passwords (const char *passwd_file, GError **error) +parse_passwords (const char *passwd_file, + gssize *out_error_line, + GError **error) { nm_auto_clear_secret_ptr NMSecretPtr contents = { 0 }; gs_unref_hashtable GHashTable *pwds_hash = NULL; - gs_free_error GError *local = NULL; const char *contents_str; gsize contents_line; pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); + NM_SET_OUT (out_error_line, -1); + if (!passwd_file) return g_steal_pointer (&pwds_hash); @@ -2730,12 +2735,8 @@ parse_passwords (const char *passwd_file, GError **error) &contents.str, &contents.len, NULL, - &local)) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("failed to read passwd-file '%s': %s"), - passwd_file, local->message); + error)) return NULL; - } contents_str = contents.str; contents_line = 0; @@ -2779,8 +2780,9 @@ parse_passwords (const char *passwd_file, GError **error) while (!NM_IN_SET (s[0], '\0', ':', '=')) s++; if (s[0] == '\0') { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing colon in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing colon for \".:\" format")); return NULL; } ((char *) s)[0] = '\0'; @@ -2794,12 +2796,14 @@ parse_passwords (const char *passwd_file, GError **error) s = strchr (l_setting, '.'); if (!s) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing dot in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing dot for \".:\" format")); return NULL; } else if (s == l_setting) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing setting name in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing setting for \".:\" format")); return NULL; } ((char *) s)[0] = '\0'; @@ -2807,8 +2811,9 @@ parse_passwords (const char *passwd_file, GError **error) l_prop = s; if (l_prop[0] == '\0') { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("missing property name in 'password' entry of passwd-file '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing property for \".:\" format")); return NULL; } @@ -2817,8 +2822,9 @@ parse_passwords (const char *passwd_file, GError **error) l_setting = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME; if (nm_setting_lookup_type (l_setting) == G_TYPE_INVALID) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("invalid setting name in 'password' entry '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("invalid setting name")); return NULL; } @@ -2832,8 +2838,9 @@ parse_passwords (const char *passwd_file, GError **error) l_hash_key = g_strdup_printf ("%s.%s", l_setting, l_prop); if (!g_utf8_validate (l_hash_key, -1, NULL)) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("invalid non UTF-8 setting name in 'password' entry '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("property name is not UTF-8")); return NULL; } @@ -2846,14 +2853,16 @@ parse_passwords (const char *passwd_file, GError **error) /* In some cases it might make sense to support binary secrets (like the WPA-PSK which has no * defined encoding. However, all API that follows can only handle UTF-8, and no mechanism * to escape the secrets. Reject non-UTF-8 early. */ - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("invalid non UTF-8 secret in 'password' entry '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("secret is not UTF-8")); return NULL; } if (strlen (l_hash_val) != l_hash_val_len) { - g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, - _("invalid non UTF-8 secret with NUL bytes in 'password' entry '%s', line %zu"), passwd_file, contents_line); + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("secret is not UTF-8")); return NULL; } @@ -2911,9 +2920,27 @@ nmc_activate_connection (NmCli *nmc, } /* Parse passwords given in passwords file */ - pwds_hash = parse_passwords (pwds, error); - if (!pwds_hash) - return FALSE; + { + gs_free_error GError *local = NULL; + gssize error_line; + + pwds_hash = parse_passwords (pwds, &error_line, &local); + if (!pwds_hash) { + if (error_line >= 0) { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("invalid passwd-file '%s' at line %zd: %s"), + pwds, + error_line, + local->message); + } else { + g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, + _("invalid passwd-file '%s': %s"), + pwds, + local->message); + } + return FALSE; + } + } if (nmc->pwds_hash) g_hash_table_destroy (nmc->pwds_hash); -- cgit v1.2.1 From 360f0fae111a76cc47fb559d035096221a3a5993 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 13:33:35 +0200 Subject: cli: move nmc_utils_read_passwd_file() to "common/nm-client-utils.c" --- clients/cli/connections.c | 180 +-------------------------------------- clients/common/nm-client-utils.c | 180 ++++++++++++++++++++++++++++++++++++++- clients/common/nm-client-utils.h | 4 + 3 files changed, 184 insertions(+), 180 deletions(-) diff --git a/clients/cli/connections.c b/clients/cli/connections.c index b2dd0a60a7..a6bbe5669b 100644 --- a/clients/cli/connections.c +++ b/clients/cli/connections.c @@ -16,8 +16,6 @@ #include #include -#include "nm-glib-aux/nm-secret-utils.h" -#include "nm-glib-aux/nm-io-utils.h" #include "nm-client-utils.h" #include "nm-vpn-helpers.h" #include "nm-meta-setting-access.h" @@ -2696,182 +2694,6 @@ activate_connection_cb (GObject *client, GAsyncResult *result, gpointer user_dat } } -/** - * parse_passwords: - * @passwd_file: file with passwords to parse - * @out_error_line: returns in case of a syntax error in the file, the line - * on which it occurred. - * @error: location to store error, or %NULL - * - * Parse passwords given in @passwd_file and insert them into a hash table. - * Example of @passwd_file contents: - * wifi.psk:tajne heslo - * 802-1x.password:krakonos - * 802-11-wireless-security:leap-password:my leap password - * - * Returns: hash table with parsed passwords, or %NULL on an error - */ -static GHashTable * -parse_passwords (const char *passwd_file, - gssize *out_error_line, - GError **error) -{ - nm_auto_clear_secret_ptr NMSecretPtr contents = { 0 }; - gs_unref_hashtable GHashTable *pwds_hash = NULL; - const char *contents_str; - gsize contents_line; - - pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); - - NM_SET_OUT (out_error_line, -1); - - if (!passwd_file) - return g_steal_pointer (&pwds_hash); - - if (!nm_utils_file_get_contents (-1, - passwd_file, - 1024*1024, - NM_UTILS_FILE_GET_CONTENTS_FLAG_SECRET, - &contents.str, - &contents.len, - NULL, - error)) - return NULL; - - contents_str = contents.str; - contents_line = 0; - while (contents_str[0]) { - nm_auto_free_secret char *l_hash_key = NULL; - nm_auto_free_secret char *l_hash_val = NULL; - const char *l_content_line; - const char *l_setting; - const char *l_prop; - const char *l_val; - const char *s; - gsize l_hash_val_len; - - /* consume first line. As line delimiters we accept "\r\n", "\n", and "\r". */ - l_content_line = contents_str; - s = l_content_line; - while (!NM_IN_SET (s[0], '\0', '\r', '\n')) - s++; - if (s[0] != '\0') { - if ( s[0] == '\r' - && s[1] == '\n') { - ((char *) s)[0] = '\0'; - s += 2; - } else { - ((char *) s)[0] = '\0'; - s += 1; - } - } - contents_str = s; - contents_line++; - - l_content_line = nm_str_skip_leading_spaces (l_content_line); - if (NM_IN_SET (l_content_line[0], '\0', '#')) { - /* a comment or empty line. Ignore. */ - continue; - } - - l_setting = l_content_line; - - s = l_setting; - while (!NM_IN_SET (s[0], '\0', ':', '=')) - s++; - if (s[0] == '\0') { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("missing colon for \".:\" format")); - return NULL; - } - ((char *) s)[0] = '\0'; - s++; - - l_val = s; - - g_strchomp ((char *) l_setting); - - nm_assert (nm_str_is_stripped (l_setting)); - - s = strchr (l_setting, '.'); - if (!s) { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("missing dot for \".:\" format")); - return NULL; - } else if (s == l_setting) { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("missing setting for \".:\" format")); - return NULL; - } - ((char *) s)[0] = '\0'; - s++; - - l_prop = s; - if (l_prop[0] == '\0') { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("missing property for \".:\" format")); - return NULL; - } - - /* Accept wifi-sec or wifi instead of cumbersome '802-11-wireless-security' */ - if (NM_IN_STRSET (l_setting, "wifi-sec", "wifi")) - l_setting = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME; - - if (nm_setting_lookup_type (l_setting) == G_TYPE_INVALID) { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("invalid setting name")); - return NULL; - } - - if ( nm_streq (l_setting, "vpn") - && NM_STR_HAS_PREFIX (l_prop, "secret.")) { - /* in 1.12.0, we wrongly required the VPN secrets to be named - * "vpn.secret". It should be "vpn.secrets". Work around it - * (rh#1628833). */ - l_hash_key = g_strdup_printf ("vpn.secrets.%s", &l_prop[NM_STRLEN ("secret.")]); - } else - l_hash_key = g_strdup_printf ("%s.%s", l_setting, l_prop); - - if (!g_utf8_validate (l_hash_key, -1, NULL)) { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("property name is not UTF-8")); - return NULL; - } - - /* Support backslash escaping in the secret value. We strip non-escaped leading/trailing whitespaces. */ - s = nm_utils_buf_utf8safe_unescape (l_val, NM_UTILS_STR_UTF8_SAFE_UNESCAPE_STRIP_SPACES, &l_hash_val_len, (gpointer *) &l_hash_val); - if (!l_hash_val) - l_hash_val = g_strdup (s); - - if (!g_utf8_validate (l_hash_val, -1, NULL)) { - /* In some cases it might make sense to support binary secrets (like the WPA-PSK which has no - * defined encoding. However, all API that follows can only handle UTF-8, and no mechanism - * to escape the secrets. Reject non-UTF-8 early. */ - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("secret is not UTF-8")); - return NULL; - } - - if (strlen (l_hash_val) != l_hash_val_len) { - NM_SET_OUT (out_error_line, contents_line); - nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, - _("secret is not UTF-8")); - return NULL; - } - - g_hash_table_insert (pwds_hash, g_steal_pointer (&l_hash_key), g_steal_pointer (&l_hash_val)); - } - - return g_steal_pointer (&pwds_hash); -} - static gboolean nmc_activate_connection (NmCli *nmc, NMConnection *connection, @@ -2924,7 +2746,7 @@ nmc_activate_connection (NmCli *nmc, gs_free_error GError *local = NULL; gssize error_line; - pwds_hash = parse_passwords (pwds, &error_line, &local); + pwds_hash = nmc_utils_read_passwd_file (pwds, &error_line, &local); if (!pwds_hash) { if (error_line >= 0) { g_set_error (error, NMCLI_ERROR, NMC_RESULT_ERROR_USER_INPUT, diff --git a/clients/common/nm-client-utils.c b/clients/common/nm-client-utils.c index cf8b478560..a296a3b9dc 100644 --- a/clients/common/nm-client-utils.c +++ b/clients/common/nm-client-utils.c @@ -6,8 +6,10 @@ #include "nm-default.h" #include "nm-client-utils.h" -#include "nm-utils.h" +#include "nm-glib-aux/nm-secret-utils.h" +#include "nm-glib-aux/nm-io-utils.h" +#include "nm-utils.h" #include "nm-device-bond.h" #include "nm-device-bridge.h" #include "nm-device-team.h" @@ -587,3 +589,179 @@ nmc_print_qrcode (const char *str) } } } + +/** + * nmc_utils_read_passwd_file: + * @passwd_file: file with passwords to parse + * @out_error_line: returns in case of a syntax error in the file, the line + * on which it occurred. + * @error: location to store error, or %NULL + * + * Parse passwords given in @passwd_file and insert them into a hash table. + * Example of @passwd_file contents: + * wifi.psk:tajne heslo + * 802-1x.password:krakonos + * 802-11-wireless-security:leap-password:my leap password + * + * Returns: (transfer full): hash table with parsed passwords, or %NULL on an error + */ +GHashTable * +nmc_utils_read_passwd_file (const char *passwd_file, + gssize *out_error_line, + GError **error) +{ + nm_auto_clear_secret_ptr NMSecretPtr contents = { 0 }; + gs_unref_hashtable GHashTable *pwds_hash = NULL; + const char *contents_str; + gsize contents_line; + + pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); + + NM_SET_OUT (out_error_line, -1); + + if (!passwd_file) + return g_steal_pointer (&pwds_hash); + + if (!nm_utils_file_get_contents (-1, + passwd_file, + 1024*1024, + NM_UTILS_FILE_GET_CONTENTS_FLAG_SECRET, + &contents.str, + &contents.len, + NULL, + error)) + return NULL; + + contents_str = contents.str; + contents_line = 0; + while (contents_str[0]) { + nm_auto_free_secret char *l_hash_key = NULL; + nm_auto_free_secret char *l_hash_val = NULL; + const char *l_content_line; + const char *l_setting; + const char *l_prop; + const char *l_val; + const char *s; + gsize l_hash_val_len; + + /* consume first line. As line delimiters we accept "\r\n", "\n", and "\r". */ + l_content_line = contents_str; + s = l_content_line; + while (!NM_IN_SET (s[0], '\0', '\r', '\n')) + s++; + if (s[0] != '\0') { + if ( s[0] == '\r' + && s[1] == '\n') { + ((char *) s)[0] = '\0'; + s += 2; + } else { + ((char *) s)[0] = '\0'; + s += 1; + } + } + contents_str = s; + contents_line++; + + l_content_line = nm_str_skip_leading_spaces (l_content_line); + if (NM_IN_SET (l_content_line[0], '\0', '#')) { + /* a comment or empty line. Ignore. */ + continue; + } + + l_setting = l_content_line; + + s = l_setting; + while (!NM_IN_SET (s[0], '\0', ':', '=')) + s++; + if (s[0] == '\0') { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing colon for \".:\" format")); + return NULL; + } + ((char *) s)[0] = '\0'; + s++; + + l_val = s; + + g_strchomp ((char *) l_setting); + + nm_assert (nm_str_is_stripped (l_setting)); + + s = strchr (l_setting, '.'); + if (!s) { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing dot for \".:\" format")); + return NULL; + } else if (s == l_setting) { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing setting for \".:\" format")); + return NULL; + } + ((char *) s)[0] = '\0'; + s++; + + l_prop = s; + if (l_prop[0] == '\0') { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("missing property for \".:\" format")); + return NULL; + } + + /* Accept wifi-sec or wifi instead of cumbersome '802-11-wireless-security' */ + if (NM_IN_STRSET (l_setting, "wifi-sec", "wifi")) + l_setting = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME; + + if (nm_setting_lookup_type (l_setting) == G_TYPE_INVALID) { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("invalid setting name")); + return NULL; + } + + if ( nm_streq (l_setting, "vpn") + && NM_STR_HAS_PREFIX (l_prop, "secret.")) { + /* in 1.12.0, we wrongly required the VPN secrets to be named + * "vpn.secret". It should be "vpn.secrets". Work around it + * (rh#1628833). */ + l_hash_key = g_strdup_printf ("vpn.secrets.%s", &l_prop[NM_STRLEN ("secret.")]); + } else + l_hash_key = g_strdup_printf ("%s.%s", l_setting, l_prop); + + if (!g_utf8_validate (l_hash_key, -1, NULL)) { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("property name is not UTF-8")); + return NULL; + } + + /* Support backslash escaping in the secret value. We strip non-escaped leading/trailing whitespaces. */ + s = nm_utils_buf_utf8safe_unescape (l_val, NM_UTILS_STR_UTF8_SAFE_UNESCAPE_STRIP_SPACES, &l_hash_val_len, (gpointer *) &l_hash_val); + if (!l_hash_val) + l_hash_val = g_strdup (s); + + if (!g_utf8_validate (l_hash_val, -1, NULL)) { + /* In some cases it might make sense to support binary secrets (like the WPA-PSK which has no + * defined encoding. However, all API that follows can only handle UTF-8, and no mechanism + * to escape the secrets. Reject non-UTF-8 early. */ + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("secret is not UTF-8")); + return NULL; + } + + if (strlen (l_hash_val) != l_hash_val_len) { + NM_SET_OUT (out_error_line, contents_line); + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("secret is not UTF-8")); + return NULL; + } + + g_hash_table_insert (pwds_hash, g_steal_pointer (&l_hash_key), g_steal_pointer (&l_hash_val)); + } + + return g_steal_pointer (&pwds_hash); +} diff --git a/clients/common/nm-client-utils.h b/clients/common/nm-client-utils.h index 1e3085d56d..f4f67544e7 100644 --- a/clients/common/nm-client-utils.h +++ b/clients/common/nm-client-utils.h @@ -43,4 +43,8 @@ const char *nmc_password_subst_char (void); void nmc_print_qrcode (const char *str); +GHashTable *nmc_utils_read_passwd_file (const char *passwd_file, + gssize *out_error_line, + GError **error); + #endif /* __NM_CLIENT_UTILS_H__ */ -- cgit v1.2.1 From 38a79ca5cd0366c3c6bac82c963b2d7aace2e4b0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 16:33:59 +0200 Subject: cli: split parsing from nmc_utils_read_passwd_file() Makes it easier testable. --- clients/common/nm-client-utils.c | 25 ++++++++++++++++++------- clients/common/nm-client-utils.h | 4 ++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/clients/common/nm-client-utils.c b/clients/common/nm-client-utils.c index a296a3b9dc..97885291d6 100644 --- a/clients/common/nm-client-utils.c +++ b/clients/common/nm-client-utils.c @@ -611,16 +611,11 @@ nmc_utils_read_passwd_file (const char *passwd_file, GError **error) { nm_auto_clear_secret_ptr NMSecretPtr contents = { 0 }; - gs_unref_hashtable GHashTable *pwds_hash = NULL; - const char *contents_str; - gsize contents_line; - - pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); NM_SET_OUT (out_error_line, -1); if (!passwd_file) - return g_steal_pointer (&pwds_hash); + return g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); if (!nm_utils_file_get_contents (-1, passwd_file, @@ -632,7 +627,23 @@ nmc_utils_read_passwd_file (const char *passwd_file, error)) return NULL; - contents_str = contents.str; + return nmc_utils_parse_passwd_file (contents.str, out_error_line, error); +} + +GHashTable * +nmc_utils_parse_passwd_file (char *contents /* will be modified */, + gssize *out_error_line, + GError **error) +{ + gs_unref_hashtable GHashTable *pwds_hash = NULL; + const char *contents_str; + gsize contents_line; + + pwds_hash = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) nm_free_secret); + + NM_SET_OUT (out_error_line, -1); + + contents_str = contents; contents_line = 0; while (contents_str[0]) { nm_auto_free_secret char *l_hash_key = NULL; diff --git a/clients/common/nm-client-utils.h b/clients/common/nm-client-utils.h index f4f67544e7..8983e6c89b 100644 --- a/clients/common/nm-client-utils.h +++ b/clients/common/nm-client-utils.h @@ -43,6 +43,10 @@ const char *nmc_password_subst_char (void); void nmc_print_qrcode (const char *str); +GHashTable *nmc_utils_parse_passwd_file (char *contents, + gssize *out_error_line, + GError **error); + GHashTable *nmc_utils_read_passwd_file (const char *passwd_file, gssize *out_error_line, GError **error); -- cgit v1.2.1 From 797ee7d5ea720883a9645ebae5aaa8bd845c91d0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 4 May 2020 16:39:41 +0200 Subject: cli/tests: add unit test for nmc_utils_parse_passwd_file() --- clients/common/tests/test-clients-common.c | 73 ++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/clients/common/tests/test-clients-common.c b/clients/common/tests/test-clients-common.c index 70ee2cb19a..0b61e7bb56 100644 --- a/clients/common/tests/test-clients-common.c +++ b/clients/common/tests/test-clients-common.c @@ -7,6 +7,7 @@ #include "nm-meta-setting-access.h" #include "nm-vpn-helpers.h" +#include "nm-client-utils.h" #include "nm-utils/nm-test-utils.h" @@ -235,6 +236,77 @@ test_client_import_wireguard_missing (void) /*****************************************************************************/ +#define _do_test_parse_passwd_file(contents, success, exp_error_line, ...) \ + G_STMT_START { \ + static const NMUtilsNamedValue _values[] = { \ + __VA_ARGS__ \ + }; \ + gs_free char *_contents = g_strndup (contents, NM_STRLEN (contents)); \ + gs_unref_hashtable GHashTable *_secrets = NULL; \ + gs_free_error GError *_local = NULL; \ + gssize _error_line; \ + GError **_p_local = nmtst_get_rand_bool () ? &_local : NULL; \ + gssize *_p_error_line = nmtst_get_rand_bool () ? &_error_line : NULL; \ + gboolean _success = !!(success); \ + gssize _exp_error_line = (exp_error_line); \ + int _i; \ + \ + g_assert (_success || (G_N_ELEMENTS (_values) == 0)); \ + \ + _secrets = nmc_utils_parse_passwd_file (_contents, _p_error_line, _p_local); \ + \ + g_assert (_success == (!!_secrets)); \ + if (!_success) { \ + if (_p_error_line) \ + g_assert_cmpint (_exp_error_line, ==, *_p_error_line); \ + if (_p_local) \ + g_assert (_local); \ + } else { \ + if (_p_error_line) \ + g_assert_cmpint (-1, ==, *_p_error_line); \ + g_assert (!_local); \ + \ + for (_i = 0; _i < G_N_ELEMENTS (_values); _i++) { \ + const NMUtilsNamedValue *_n = &_values[_i]; \ + const char *_v; \ + \ + _v = g_hash_table_lookup (_secrets, _n->name); \ + if (!_v) \ + g_error ("cannot find key \"%s\"", _n->name); \ + g_assert_cmpstr (_v, ==, _n->value_str); \ + } \ + \ + g_assert_cmpint (g_hash_table_size (_secrets), ==, G_N_ELEMENTS (_values)); \ + } \ + } G_STMT_END + +#define _do_test_parse_passwd_file_bad( contents, exp_error_line) _do_test_parse_passwd_file (contents, FALSE, exp_error_line) +#define _do_test_parse_passwd_file_good(contents, ...) _do_test_parse_passwd_file (contents, TRUE, -1, __VA_ARGS__) + +static void +test_nmc_utils_parse_passwd_file (void) +{ + _do_test_parse_passwd_file_good (""); + _do_test_parse_passwd_file_bad ("x", 1); + _do_test_parse_passwd_file_bad ("\r\rx", 3); + _do_test_parse_passwd_file_good ("wifi.psk=abc", + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk", "abc") ); + _do_test_parse_passwd_file_good ("wifi.psk:ABC\r" + "wifi-sec.psk = abc ", + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk", "abc") ); + _do_test_parse_passwd_file_good ("wifi.psk: abc\r" + "wifi-sec.psk2 = d\\145f\r\n" + " wifi.psk3 = e\\ \n" + " #wifi-sec.psk2 = \r\n" + " wifi-sec.psk4:", + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk", "abc"), + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk2", "def"), + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk3", "e "), + NM_UTILS_NAMED_VALUE_INIT ("802-11-wireless-security.psk4", "") ); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -248,6 +320,7 @@ main (int argc, char **argv) g_test_add_func ("/client/import/wireguard/test2", test_client_import_wireguard_test2); g_test_add_func ("/client/import/wireguard/test3", test_client_import_wireguard_test3); g_test_add_func ("/client/import/wireguard/missing", test_client_import_wireguard_missing); + g_test_add_func ("/client/test_nmc_utils_parse_passwd_file", test_nmc_utils_parse_passwd_file); return g_test_run (); } -- cgit v1.2.1