keyfile: fix handling unsupported characters in keys

vpn.data, bond.options, and user.data encode their values directly as
keys in keyfile. However, keys for GKeyFile may not contain characters
like '='.

We need to escape such special characters, otherwise an assertion
is hit on the server:

  $ nmcli connection modify "$VPN_NAME" +vpn.data 'aa[=value'

Another example of encountering the assertion is when setting user-data key
with an invalid character "my.this=key=is=causes=a=crash".

5 files changed, 286 insertions, 5 deletions

diff --git a/libnm-core/nm-keyfile-reader.c b/libnm-core/nm-keyfile-reader.c
index 293f03b629..078c517e8e 100644
--- a/libnm-core/nm-keyfile-reader.c
+++ b/libnm-core/nm-keyfile-reader.c
@@ -724,18 +724,23 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key)
 		return;
 
 	for (iter = (const char *const*) keys; *iter; iter++) {
+		gs_free char *to_free = NULL;
+		const char *name;
+
 		value = nm_keyfile_plugin_kf_get_string (file, setting_name, *iter, NULL);
 		if (!value)
 			continue;
 
+		name = nm_keyfile_key_decode (*iter, &to_free);
+
 		if (NM_IS_SETTING_VPN (setting)) {
 			/* Add any item that's not a class property to the data hash */
-			if (!g_object_class_find_property (G_OBJECT_GET_CLASS (setting), *iter))
-				nm_setting_vpn_add_data_item (NM_SETTING_VPN (setting), *iter, value);
+			if (!g_object_class_find_property (G_OBJECT_GET_CLASS (setting), name))
+				nm_setting_vpn_add_data_item (NM_SETTING_VPN (setting), name, value);
 		}
 		if (NM_IS_SETTING_BOND (setting)) {
-			if (strcmp (*iter, "interface-name"))
-				nm_setting_bond_add_option (NM_SETTING_BOND (setting), *iter, value);
+			if (strcmp (name, "interface-name"))
+				nm_setting_bond_add_option (NM_SETTING_BOND (setting), name, value);
 		}
 		g_free (value);
 	}
diff --git a/libnm-core/nm-keyfile-utils.c b/libnm-core/nm-keyfile-utils.c
index 13cf616161..1b7860d873 100644
--- a/libnm-core/nm-keyfile-utils.c
+++ b/libnm-core/nm-keyfile-utils.c
@@ -361,3 +361,205 @@ _nm_keyfile_has_values (GKeyFile *keyfile)
 	groups = g_key_file_get_groups (keyfile, NULL);
 	return groups && groups[0];
 }
+
+/*****************************************************************************/
+
+static const char *
+_keyfile_key_encode (const char *name,
+                     char **out_to_free)
+{
+	gsize len, i;
+	GString *str;
+
+	nm_assert (name);
+	nm_assert (out_to_free && !*out_to_free);
+
+	/* See g_key_file_is_key_name().
+	 *
+	 * GKeyfile allows all UTF-8 characters (even non-well formed sequences),
+	 * except:
+	 *  - no empty keys
+	 *  - no leading/trailing ' '
+	 *  - no '=', '[', ']'
+	 *
+	 * We do something more strict here. All non-ASCII characters, all non-printable
+	 * characters, and all invalid characters are escaped with "\\XX".
+	 *
+	 * We don't escape \\, unless it is followed by two hex digits.
+	 */
+
+	if (!name[0]) {
+		/* empty keys are are backslash encoded. Note that usually
+		 * \\00 is not a valid encode, the only exception is the empty
+		 * word. */
+		return "\\00";
+	}
+
+	/* find the first character that needs escaping. */
+	i = 0;
+	if (name[0] != ' ') {
+		for (;; i++) {
+			const guchar ch = (guchar) name[i];
+
+			if (ch == '\0')
+				return name;
+
+			if (   ch < 0x20
+			    || ch >= 127
+			    || NM_IN_SET (ch, '=', '[', ']')
+			    || (   ch == '\\'
+			        && g_ascii_isxdigit (name[i + 1])
+			        && g_ascii_isxdigit (name[i + 2]))
+			    || (   ch == ' '
+			        && name[i + 1] == '\0'))
+				break;
+		}
+	} else if (name[1] == '\0')
+		return "\\20";
+
+	len = i + strlen (&name[i]);
+	nm_assert (len == strlen (name));
+	str = g_string_sized_new (len + 15);
+
+	if (name[0] == ' ') {
+		nm_assert (i == 0);
+		g_string_append (str, "\\20");
+		i = 1;
+	} else
+		g_string_append_len (str, name, i);
+
+	for (;; i++) {
+		const guchar ch = (guchar) name[i];
+
+		if (ch == '\0')
+			break;
+
+		if (   ch < 0x20
+		    || ch >= 127
+		    || NM_IN_SET (ch, '=', '[', ']')
+		    || (   ch == '\\'
+		        && g_ascii_isxdigit (name[i + 1])
+		        && g_ascii_isxdigit (name[i + 2]))
+		    || (   ch == ' '
+		        && name[i + 1] == '\0'))
+			g_string_append_printf (str, "\\%2X", ch);
+		else
+			g_string_append_c (str, (char) ch);
+	}
+
+	return (*out_to_free = g_string_free (str, FALSE));
+}
+
+static const char *
+_keyfile_key_decode (const char *key,
+                     char **out_to_free)
+{
+	gsize i, len;
+	GString *str;
+
+	nm_assert (key);
+	nm_assert (out_to_free && !*out_to_free);
+
+	if (!key[0])
+		return "";
+
+	for (i = 0; TRUE; i++) {
+		const char ch = key[i];
+
+		if (ch == '\0')
+			return key;
+		if (   ch == '\\'
+		    && g_ascii_isxdigit (key[i + 1])
+		    && g_ascii_isxdigit (key[i + 2]))
+			break;
+	}
+
+	len = i + strlen (&key[i]);
+
+	if (   len == 3
+	    && nm_streq (key, "\\00"))
+		return "";
+
+	nm_assert (len == strlen (key));
+	str = g_string_sized_new (len + 3);
+
+	g_string_append_len (str, key, i);
+	for (;;) {
+		const char ch = key[i];
+		char ch1, ch2;
+		unsigned v;
+
+		if (ch == '\0')
+			break;
+
+		if (   ch == '\\'
+		    && g_ascii_isxdigit ((ch1 = key[i + 1]))
+		    && g_ascii_isxdigit ((ch2 = key[i + 2]))) {
+			v = (g_ascii_xdigit_value (ch1) << 4) + g_ascii_xdigit_value (ch2);
+			if (v != 0) {
+				g_string_append_c (str, (char) v);
+				i += 3;
+				continue;
+			}
+		}
+		g_string_append_c (str, ch);
+		i++;
+	}
+
+	return (*out_to_free = g_string_free (str, FALSE));
+}
+
+/*****************************************************************************/
+
+const char *
+nm_keyfile_key_encode (const char *name,
+                       char **out_to_free)
+{
+	const char *key;
+
+	key = _keyfile_key_encode (name, out_to_free);
+#if NM_MORE_ASSERTS > 5
+	nm_assert (key);
+	nm_assert (!*out_to_free || key == *out_to_free);
+	nm_assert (!*out_to_free || !nm_streq0 (name, key));
+	{
+		gs_free char *to_free2 = NULL;
+		const char *name2;
+
+		name2 = _keyfile_key_decode (key, &to_free2);
+		/* name2, the result of encode()+decode() is identical to name.
+		 * That is because
+		 *   - encode() is a injective function.
+		 *   - decode() is a surjective function, however for output
+		 *     values of encode() is behaves injective too. */
+		nm_assert (nm_streq0 (name2, name));
+	}
+#endif
+	return key;
+}
+
+const char *
+nm_keyfile_key_decode (const char *key,
+                       char **out_to_free)
+{
+	const char *name;
+
+	name = _keyfile_key_decode (key, out_to_free);
+#if NM_MORE_ASSERTS > 5
+	nm_assert (name);
+	nm_assert (!*out_to_free || name == *out_to_free);
+	{
+		gs_free char *to_free2 = NULL;
+		const char *key2;
+
+		key2 = _keyfile_key_encode (name, &to_free2);
+		/* key2, the result of decode+encode may not be idential
+		 * to the original key. That is, decode() is a surjective
+		 * function mapping different keys to the same name.
+		 * However, decode() behaves injective for input that
+		 * are valid output of encode(). */
+		nm_assert (key2);
+	}
+#endif
+	return name;
+}
diff --git a/libnm-core/nm-keyfile-utils.h b/libnm-core/nm-keyfile-utils.h
index 89fd3041a0..1f63af8c2c 100644
--- a/libnm-core/nm-keyfile-utils.h
+++ b/libnm-core/nm-keyfile-utils.h
@@ -81,5 +81,11 @@ gboolean nm_keyfile_plugin_kf_has_key     (GKeyFile *kf,
                                            const char *key,
                                            GError **error);
 
+const char *nm_keyfile_key_encode (const char *name,
+                                   char **out_to_free);
+
+const char *nm_keyfile_key_decode (const char *key,
+                                   char **out_to_free);
+
 #endif  /* __NM_KEYFILE_UTILS_H__ */
 
diff --git a/libnm-core/nm-keyfile-writer.c b/libnm-core/nm-keyfile-writer.c
index 13a29ccdcc..6a3d9a9f46 100644
--- a/libnm-core/nm-keyfile-writer.c
+++ b/libnm-core/nm-keyfile-writer.c
@@ -300,8 +300,12 @@ write_hash_of_string (GKeyFile *file,
 		}
 
 		if (write_item) {
+			gs_free char *to_free = NULL;
+
 			data = g_hash_table_lookup (hash, property);
-			nm_keyfile_plugin_kf_set_string (file, group_name, property, data);
+			nm_keyfile_plugin_kf_set_string (file, group_name,
+			                                 nm_keyfile_key_encode (property, &to_free),
+			                                 data);
 		}
 	}
 }
diff --git a/libnm-core/tests/test-keyfile.c b/libnm-core/tests/test-keyfile.c
index 9944eb5ef8..f7525317a9 100644
--- a/libnm-core/tests/test-keyfile.c
+++ b/libnm-core/tests/test-keyfile.c
@@ -36,6 +36,69 @@
 #define TEST_WIRED_TLS_CA_CERT     TEST_CERT_DIR"/test-ca-cert.pem"
 #define TEST_WIRED_TLS_PRIVKEY     TEST_CERT_DIR"/test-key-and-cert.pem"
 
+/*****************************************************************************/
+
+static void
+do_test_encode_key_full (GKeyFile *kf, const char *name, const char *key, const char *key_decode_encode)
+{
+	gs_free char *to_free1 = NULL;
+	gs_free char *to_free2 = NULL;
+	const char *key2;
+	const char *name2;
+
+	g_assert (key);
+
+	if (name) {
+		key2 = nm_keyfile_key_encode (name, &to_free1);
+		g_assert (key2);
+		g_assert (NM_STRCHAR_ALL (key2, ch, (guchar) ch < 127));
+		g_assert_cmpstr (key2, ==, key);
+
+		/* try to add the encoded key to the keyfile. We expect
+		 * no g_critical warning about invalid key. */
+		g_key_file_set_value (kf, "group", key, "dummy");
+	}
+
+	name2 = nm_keyfile_key_decode (key, &to_free2);
+	if (name)
+		g_assert_cmpstr (name2, ==, name);
+	else {
+		key2 = nm_keyfile_key_encode (name2, &to_free1);
+		g_assert (key2);
+		g_assert (NM_STRCHAR_ALL (key2, ch, (guchar) ch < 127));
+		if (key_decode_encode)
+			g_assert_cmpstr (key2, ==, key_decode_encode);
+		g_key_file_set_value (kf, "group", key2, "dummy");
+	}
+}
+
+#define do_test_encode_key_bijection(kf, name, key)                      do_test_encode_key_full (kf, ""name, ""key,  NULL)
+#define do_test_encode_key_identity(kf, name)                            do_test_encode_key_full (kf, ""name, ""name, NULL)
+#define do_test_encode_key_decode_surjection(kf, key, key_decode_encode) do_test_encode_key_full (kf, NULL,   ""key,  ""key_decode_encode)
+
+static void
+test_encode_key (void)
+{
+	gs_unref_keyfile GKeyFile *kf = g_key_file_new ();
+
+	do_test_encode_key_identity (kf, "a");
+	do_test_encode_key_bijection (kf, "", "\\00");
+	do_test_encode_key_bijection (kf, " ", "\\20");
+	do_test_encode_key_bijection (kf, "\\ ", "\\\\20");
+	do_test_encode_key_identity (kf, "\\0");
+	do_test_encode_key_identity (kf, "\\a");
+	do_test_encode_key_identity (kf, "\\0g");
+	do_test_encode_key_bijection (kf, "\\0f", "\\5C0f");
+	do_test_encode_key_bijection (kf, "\\0f ", "\\5C0f\\20");
+	do_test_encode_key_bijection (kf, " \\0f ", "\\20\\5C0f\\20");
+	do_test_encode_key_bijection (kf, "\xF5", "\\F5");
+	do_test_encode_key_bijection (kf, "\x7F", "\\7F");
+	do_test_encode_key_bijection (kf, "\x1f", "\\1F");
+	do_test_encode_key_bijection (kf, "  ", "\\20\\20");
+	do_test_encode_key_bijection (kf, "   ", "\\20 \\20");
+	do_test_encode_key_decode_surjection (kf, "f\\20c", "f c");
+	do_test_encode_key_decode_surjection (kf, "\\20\\20\\20", "\\20 \\20");
+}
 
 /*****************************************************************************/
 
@@ -589,6 +652,7 @@ int main (int argc, char **argv)
 {
 	nmtst_init (&argc, &argv, TRUE);
 
+	g_test_add_func ("/core/keyfile/encode_key", test_encode_key);
 	g_test_add_func ("/core/keyfile/test_8021x_cert", test_8021x_cert);
 	g_test_add_func ("/core/keyfile/test_8021x_cert_read", test_8021x_cert_read);
 	g_test_add_func ("/core/keyfile/test_team_conf_read/valid", test_team_conf_read_valid);