From e88eedce76f79a5573df4fc38b344bbeaf7af024 Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Sat, 4 Mar 2023 21:07:03 -0600 Subject: [PATCH] Reject paths given to --filesystem/--persist with special characters There isn't much in the way of legit reasons for this, but it's a potential security footgun when displaying the text. CVE-2023-28101, GHSA-h43h-fwqx-mpp8 Signed-off-by: Ryan Gonzalez Co-authored-by: Simon McVittie --- common/flatpak-context.c | 36 ++++++++++++--- common/flatpak-utils-private.h | 3 ++ common/flatpak-utils.c | 40 ++++++++++++++++ tests/test-context.c | 84 ++++++++++++++++++++++++++++++++-- tests/testcommon.c | 41 +++++++++++++++-- 5 files changed, 190 insertions(+), 14 deletions(-) diff --git a/common/flatpak-context.c b/common/flatpak-context.c index 9c506499a4..512a577f81 100644 --- a/common/flatpak-context.c +++ b/common/flatpak-context.c @@ -487,11 +487,17 @@ flatpak_context_apply_generic_policy (FlatpakContext *context, g_ptr_array_free (new, FALSE)); } -static void + +static gboolean flatpak_context_set_persistent (FlatpakContext *context, - const char *path) + const char *path, + GError **error) { + if (!flatpak_validate_path_characters (path, error)) + return FALSE; + g_hash_table_insert (context->persistent, g_strdup (path), GINT_TO_POINTER (1)); + return TRUE; } static gboolean @@ -853,6 +859,9 @@ flatpak_context_parse_filesystem (const char *filesystem_and_mode, g_autofree char *filesystem = NULL; char *slash; + if (!flatpak_validate_path_characters (filesystem_and_mode, error)) + return FALSE; + filesystem = parse_filesystem_flags (filesystem_and_mode, negated, mode_out, error); if (filesystem == NULL) return FALSE; @@ -1484,8 +1493,7 @@ option_persist_cb (const gchar *option_name, { FlatpakContext *context = data; - flatpak_context_set_persistent (context, value); - return TRUE; + return flatpak_context_set_persistent (context, value, error); } static gboolean option_no_desktop_deprecated; @@ -1677,11 +1685,24 @@ flatpak_context_load_metadata (FlatpakContext *context, { const char *fs = parse_negated (filesystems[i], &remove); g_autofree char *filesystem = NULL; + g_autoptr(GError) local_error = NULL; FlatpakFilesystemMode mode; if (!flatpak_context_parse_filesystem (fs, remove, - &filesystem, &mode, NULL)) - g_debug ("Unknown filesystem type %s", filesystems[i]); + &filesystem, &mode, &local_error)) + { + if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA)) + { + /* Invalid characters, so just hard-fail. */ + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; + } + else + { + g_debug ("Unknown filesystem type %s", filesystems[i]); + g_clear_error (&local_error); + } + } else { g_assert (mode == FLATPAK_FILESYSTEM_MODE_NONE || !remove); @@ -1698,7 +1719,8 @@ flatpak_context_load_metadata (FlatpakContext *context, return FALSE; for (i = 0; persistent[i] != NULL; i++) - flatpak_context_set_persistent (context, persistent[i]); + if (!flatpak_context_set_persistent (context, persistent[i], error)) + return FALSE; } if (g_key_file_has_group (metakey, FLATPAK_METADATA_GROUP_SESSION_BUS_POLICY)) diff --git a/common/flatpak-utils-private.h b/common/flatpak-utils-private.h index c1282641f9..195023c20f 100644 --- a/common/flatpak-utils-private.h +++ b/common/flatpak-utils-private.h @@ -913,6 +913,9 @@ char * flatpak_escape_string (const char *s, void flatpak_print_escaped_string (const char *s, FlatpakEscapeFlags flags); +gboolean flatpak_validate_path_characters (const char *path, + GError **error); + #define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485" #endif /* __FLATPAK_UTILS_H__ */ diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 0b91ae3f91..b562522e58 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -8641,6 +8641,14 @@ append_hex_escaped_character (GString *result, g_string_append_printf (result, "\\U%08X", c); } +static char * +escape_character (gunichar c) +{ + g_autoptr(GString) res = g_string_new (""); + append_hex_escaped_character (res, c); + return g_string_free (g_steal_pointer (&res), FALSE); +} + char * flatpak_escape_string (const char *s, FlatpakEscapeFlags flags) @@ -8691,3 +8699,35 @@ flatpak_print_escaped_string (const char *s, g_autofree char *escaped = flatpak_escape_string (s, flags); g_print ("%s", escaped); } + +gboolean +flatpak_validate_path_characters (const char *path, + GError **error) +{ + while (*path) + { + gunichar c = g_utf8_get_char_validated (path, -1); + if (c == (gunichar)-1 || c == (gunichar)-2) + { + /* Need to convert to unsigned first, to avoid negative chars becoming + huge gunichars. */ + g_autofree char *escaped_char = escape_character ((unsigned char)*path); + g_autofree char *escaped = flatpak_escape_string (path, FLATPAK_ESCAPE_DEFAULT); + g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA, + "Non-UTF8 byte %s in path %s", escaped_char, escaped); + return FALSE; + } + else if (!is_char_safe (c)) + { + g_autofree char *escaped_char = escape_character (c); + g_autofree char *escaped = flatpak_escape_string (path, FLATPAK_ESCAPE_DEFAULT); + g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA, + "Non-graphical character %s in path %s", escaped_char, escaped); + return FALSE; + } + + path = g_utf8_find_next_char (path, NULL); + } + + return TRUE; +} diff --git a/tests/test-context.c b/tests/test-context.c index c128a83fae..6c15feb013 100644 --- a/tests/test-context.c +++ b/tests/test-context.c @@ -34,13 +34,14 @@ str_has_prefix (gconstpointer candidate, } static void context_parse_args (FlatpakContext *context, + GError **error, ...) G_GNUC_NULL_TERMINATED; static void context_parse_args (FlatpakContext *context, + GError **error, ...) { - g_autoptr(GError) local_error = NULL; g_autoptr(GOptionContext) oc = NULL; g_autoptr(GOptionGroup) group = NULL; g_autoptr(GPtrArray) args = g_ptr_array_new_with_free_func (g_free); @@ -50,7 +51,7 @@ context_parse_args (FlatpakContext *context, g_ptr_array_add (args, g_strdup ("argv[0]")); - va_start (ap, context); + va_start (ap, error); while ((arg = va_arg (ap, const char *)) != NULL) g_ptr_array_add (args, g_strdup (arg)); @@ -63,8 +64,7 @@ context_parse_args (FlatpakContext *context, oc = g_option_context_new (""); group = flatpak_context_get_options (context); g_option_context_add_group (oc, group); - g_option_context_parse_strv (oc, &argv, &local_error); - g_assert_no_error (local_error); + g_option_context_parse_strv (oc, &argv, error); } static void @@ -84,19 +84,26 @@ test_context_merge_fs (void) g_autoptr(FlatpakContext) lowest = flatpak_context_new (); g_autoptr(FlatpakContext) middle = flatpak_context_new (); g_autoptr(FlatpakContext) highest = flatpak_context_new (); + g_autoptr(GError) local_error = NULL; gpointer value; context_parse_args (lowest, + &local_error, "--filesystem=/one", NULL); + g_assert_no_error (local_error); context_parse_args (middle, + &local_error, "--nofilesystem=host:reset", "--filesystem=/two", NULL); + g_assert_no_error (local_error); context_parse_args (highest, + &local_error, "--nofilesystem=host", "--filesystem=/three", NULL); + g_assert_no_error (local_error); g_assert_false (g_hash_table_lookup_extended (lowest->filesystems, "host", NULL, NULL)); g_assert_false (g_hash_table_lookup_extended (lowest->filesystems, "host-reset", NULL, NULL)); @@ -178,20 +185,28 @@ test_context_merge_fs (void) gpointer value; context_parse_args (lowest, + &local_error, "--filesystem=/one", NULL); + g_assert_no_error (local_error); context_parse_args (mid_low, + &local_error, "--nofilesystem=host:reset", "--filesystem=/two", NULL); + g_assert_no_error (local_error); context_parse_args (mid_high, + &local_error, "--filesystem=host", "--filesystem=/three", NULL); + g_assert_no_error (local_error); context_parse_args (highest, + &local_error, "--nofilesystem=host", "--filesystem=/four", NULL); + g_assert_no_error (local_error); g_assert_false (g_hash_table_lookup_extended (lowest->filesystems, "host", NULL, NULL)); g_assert_false (g_hash_table_lookup_extended (lowest->filesystems, "host-reset", NULL, NULL)); @@ -332,12 +347,73 @@ test_context_merge_fs (void) } } +const char *invalid_path_args[] = { + "--filesystem=/\033[J:ro", + "--filesystem=/\033[J", + "--persist=\033[J", +}; + +/* CVE-2023-28101 */ +static void +test_validate_path_args (void) +{ + gsize idx; + + for (idx = 0; idx < G_N_ELEMENTS (invalid_path_args); idx++) + { + g_autoptr(FlatpakContext) context = flatpak_context_new (); + g_autoptr(GError) local_error = NULL; + const char *path = invalid_path_args[idx]; + + context_parse_args (context, &local_error, path, NULL); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA); + g_assert (strstr (local_error->message, "Non-graphical character")); + } +} + +typedef struct { + const char *key; + const char *value; +} PathValidityData; + +PathValidityData invalid_path_meta[] = { + {FLATPAK_METADATA_KEY_FILESYSTEMS, "\033[J"}, + {FLATPAK_METADATA_KEY_PERSISTENT, "\033[J"}, +}; + +/* CVE-2023-28101 */ +static void +test_validate_path_meta (void) +{ + gsize idx; + + for (idx = 0; idx < G_N_ELEMENTS (invalid_path_meta); idx++) + { + g_autoptr(FlatpakContext) context = flatpak_context_new (); + g_autoptr(GKeyFile) metakey = g_key_file_new (); + g_autoptr(GError) local_error = NULL; + PathValidityData *data = &invalid_path_meta[idx]; + gboolean ret = FALSE; + + g_key_file_set_string_list (metakey, FLATPAK_METADATA_GROUP_CONTEXT, + data->key, &data->value, 1); + + ret = flatpak_context_load_metadata (context, metakey, &local_error); + g_assert_false (ret); + g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_INVALID_DATA); + g_assert (strstr (local_error->message, "Non-graphical character")); + } + +} + int main (int argc, char *argv[]) { g_test_init (&argc, &argv, NULL); g_test_add_func ("/context/merge-fs", test_context_merge_fs); + g_test_add_func ("/context/validate-path-args", test_validate_path_args); + g_test_add_func ("/context/validate-path-meta", test_validate_path_meta); return g_test_run (); } diff --git a/tests/testcommon.c b/tests/testcommon.c index daae5e4aac..0840632575 100644 --- a/tests/testcommon.c +++ b/tests/testcommon.c @@ -1567,11 +1567,12 @@ static EscapeData escapes[] = { {"abc def", FLATPAK_ESCAPE_DEFAULT, "abc def"}, {"やあ", FLATPAK_ESCAPE_DEFAULT, "やあ"}, {"\033[;1m", FLATPAK_ESCAPE_DEFAULT, "'\\x1B[;1m'"}, - // non-printable U+061C + /* U+061C ARABIC LETTER MARK, non-printable */ {"\u061C", FLATPAK_ESCAPE_DEFAULT, "'\\u061C'"}, - // non-printable U+1343F + /* U+1343F EGYPTIAN HIEROGLYPH END WALLED ENCLOSURE, non-printable and + * outside BMP */ {"\xF0\x93\x90\xBF", FLATPAK_ESCAPE_DEFAULT, "'\\U0001343F'"}, - // invalid utf-8 + /* invalid utf-8 */ {"\xD8\1", FLATPAK_ESCAPE_DEFAULT, "'\\xD8\\x01'"}, {"\b \n abc ' \\", FLATPAK_ESCAPE_DEFAULT, "'\\x08 \\x0A abc \\' \\\\'"}, {"\b \n abc ' \\", FLATPAK_ESCAPE_DO_NOT_QUOTE, "\\x08 \\x0A abc ' \\\\"}, @@ -1595,6 +1596,39 @@ test_string_escape (void) } } +typedef struct { + const char *path; + gboolean ret; +} PathValidityData; + +static PathValidityData paths[] = { + {"/a/b/../c.def", TRUE}, + {"やあ", TRUE}, + /* U+061C ARABIC LETTER MARK, non-printable */ + {"\u061C", FALSE}, + /* U+1343F EGYPTIAN HIEROGLYPH END WALLED ENCLOSURE, non-printable and + * outside BMP */ + {"\xF0\x93\x90\xBF", FALSE}, + /* invalid utf-8 */ + {"\xD8\1", FALSE}, +}; + +/* CVE-2023-28101 */ +static void +test_validate_path_characters (void) +{ + gsize idx; + + for (idx = 0; idx < G_N_ELEMENTS (paths); idx++) + { + PathValidityData *data = &paths[idx]; + gboolean ret = FALSE; + + ret = flatpak_validate_path_characters (data->path, NULL); + g_assert_cmpint (ret, ==, data->ret); + } +} + int main (int argc, char *argv[]) { @@ -1624,6 +1658,7 @@ main (int argc, char *argv[]) g_test_add_func ("/common/dconf-paths", test_dconf_paths); g_test_add_func ("/common/decompose-ref", test_decompose); g_test_add_func ("/common/string-escape", test_string_escape); + g_test_add_func ("/common/validate-path-characters", test_validate_path_characters); g_test_add_func ("/app/looks-like-branch", test_looks_like_branch); g_test_add_func ("/app/columns", test_columns);