From 20cf7360ff92dff58abb1cebc36d1ea737d3e26c Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Sat, 4 Mar 2023 21:07:03 -0600 Subject: 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 | 39 ++++++++++++++++++++ tests/test-context.c | 84 ++++++++++++++++++++++++++++++++++++++++-- tests/testcommon.c | 41 +++++++++++++++++++-- 5 files changed, 189 insertions(+), 14 deletions(-) diff --git a/common/flatpak-context.c b/common/flatpak-context.c index f277e4ac..38653804 100644 --- a/common/flatpak-context.c +++ b/common/flatpak-context.c @@ -488,11 +488,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 @@ -854,6 +860,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; @@ -1510,8 +1519,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; @@ -1703,11 +1711,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); @@ -1724,7 +1745,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 84ad6e10..eb97a8dc 100644 --- a/common/flatpak-utils-private.h +++ b/common/flatpak-utils-private.h @@ -939,6 +939,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); + gboolean running_under_sudo (void); #define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485" diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 29476e42..0589a61e 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -9249,6 +9249,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) @@ -9300,6 +9308,37 @@ flatpak_print_escaped_string (const char *s, 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; +} gboolean running_under_sudo (void) diff --git a/tests/test-context.c b/tests/test-context.c index 785ca509..d7afaa06 100644 --- a/tests/test-context.c +++ b/tests/test-context.c @@ -129,13 +129,14 @@ test_context_env_fd (void) } 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); @@ -145,7 +146,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)); @@ -158,8 +159,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 @@ -179,19 +179,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)); @@ -273,20 +280,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)); @@ -427,6 +442,65 @@ 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[]) { @@ -435,6 +509,8 @@ main (int argc, char *argv[]) g_test_add_func ("/context/env", test_context_env); g_test_add_func ("/context/env-fd", test_context_env_fd); 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 d8ad09c7..8ba3770d 100644 --- a/tests/testcommon.c +++ b/tests/testcommon.c @@ -1847,11 +1847,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 ' \\\\"}, @@ -1875,6 +1876,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[]) { @@ -1909,6 +1943,7 @@ main (int argc, char *argv[]) g_test_add_func ("/common/str-is-integer", test_str_is_integer); g_test_add_func ("/common/parse-x11-display", test_parse_x11_display); 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); -- cgit v1.2.1