From 6cac99dafe6003c8a4bd5666341c217876536869 Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Sat, 4 Mar 2023 16:23:37 -0600 Subject: [PATCH 1/3] Ensure special characters in permissions and metadata are escaped This prevents someone from placing special characters in order to manipulate the appearance of the permissions list. CVE-2023-28101, GHSA-h43h-fwqx-mpp8 Signed-off-by: Ryan Gonzalez --- app/flatpak-builtins-info.c | 8 ++- app/flatpak-builtins-remote-info.c | 5 +- app/flatpak-cli-transaction.c | 12 +++-- common/flatpak-utils-private.h | 11 ++++ common/flatpak-utils.c | 82 +++++++++++++++++++++++++++++- tests/make-test-app.sh | 8 +++ tests/test-info.sh | 14 +++-- tests/testcommon.c | 39 ++++++++++++++ 8 files changed, 168 insertions(+), 11 deletions(-) Index: flatpak-1.4.2/app/flatpak-builtins-info.c =================================================================== --- flatpak-1.4.2.orig/app/flatpak-builtins-info.c +++ flatpak-1.4.2/app/flatpak-builtins-info.c @@ -415,7 +415,9 @@ flatpak_builtin_info (int argc, char **a if (!g_file_load_contents (file, cancellable, &data, &data_size, NULL, error)) return FALSE; - g_print ("%s", data); + flatpak_print_escaped_string (data, + FLATPAK_ESCAPE_ALLOW_NEWLINES + | FLATPAK_ESCAPE_DO_NOT_QUOTE); } if (opt_show_permissions || opt_file_access) @@ -437,6 +439,9 @@ flatpak_builtin_info (int argc, char **a return FALSE; g_print ("%s", contents); + flatpak_print_escaped_string (contents, + FLATPAK_ESCAPE_ALLOW_NEWLINES + | FLATPAK_ESCAPE_DO_NOT_QUOTE); } if (opt_file_access) Index: flatpak-1.4.2/app/flatpak-builtins-remote-info.c =================================================================== --- flatpak-1.4.2.orig/app/flatpak-builtins-remote-info.c +++ flatpak-1.4.2/app/flatpak-builtins-remote-info.c @@ -450,7 +450,10 @@ flatpak_builtin_remote_info (int argc, c if (opt_show_metadata) { - g_print ("%s", xa_metadata ? xa_metadata : ""); + if (xa_metadata != NULL) + flatpak_print_escaped_string (xa_metadata, + FLATPAK_ESCAPE_ALLOW_NEWLINES + | FLATPAK_ESCAPE_DO_NOT_QUOTE); if (xa_metadata == NULL || !g_str_has_suffix (xa_metadata, "\n")) g_print ("\n"); } Index: flatpak-1.4.2/app/flatpak-cli-transaction.c =================================================================== --- flatpak-1.4.2.orig/app/flatpak-cli-transaction.c +++ flatpak-1.4.2/app/flatpak-cli-transaction.c @@ -515,7 +515,12 @@ end_of_lifed_with_rebase (FlatpakTransac if (rebased_to_ref) g_print (_("Info: %s is end-of-life, in preference of %s\n"), flatpak_ref_get_name (rref), rebased_to_ref); else if (reason) - g_print (_("Info: %s is end-of-life, with reason: %s\n"), flatpak_ref_get_name (rref), reason); + { + g_autofree char *escaped_reason = flatpak_escape_string (reason, + FLATPAK_ESCAPE_ALLOW_NEWLINES | + FLATPAK_ESCAPE_DO_NOT_QUOTE); + g_print (_("Info: %s is end-of-life, with reason: %s\n"), flatpak_ref_get_name (rref), escaped_reason); + } if (rebased_to_ref && remote) { @@ -650,12 +655,16 @@ print_perm_line (int idx, int cols) { g_autoptr(GString) res = g_string_new (NULL); + g_autofree char *escaped_first_perm = NULL; int i; - g_string_append_printf (res, " [%d] %s", idx, (char *) items->pdata[0]); + escaped_first_perm = flatpak_escape_string (items->pdata[0], FLATPAK_ESCAPE_DEFAULT); + g_string_append_printf (res, " [%d] %s", idx, escaped_first_perm); for (i = 1; i < items->len; i++) { + g_autofree char *escaped = flatpak_escape_string (items->pdata[i], + FLATPAK_ESCAPE_DEFAULT); char *p; int len; @@ -664,10 +673,10 @@ print_perm_line (int idx, p = res->str; len = (res->str + strlen (res->str)) - p; - if (len + strlen ((char *) items->pdata[i]) + 2 >= cols) - g_string_append_printf (res, ",\n %s", (char *) items->pdata[i]); + if (len + strlen (escaped) + 2 >= cols) + g_string_append_printf (res, ",\n %s", escaped); else - g_string_append_printf (res, ", %s", (char *) items->pdata[i]); + g_string_append_printf (res, ", %s", escaped); } g_print ("%s\n", res->str); Index: flatpak-1.4.2/common/flatpak-utils-private.h =================================================================== --- flatpak-1.4.2.orig/common/flatpak-utils-private.h +++ flatpak-1.4.2/common/flatpak-utils-private.h @@ -844,6 +844,20 @@ gboolean flatpak_repo_resolve_rev (Ostre GCancellable *cancellable, GError **error); +typedef enum { + FLATPAK_ESCAPE_DEFAULT = 0, + FLATPAK_ESCAPE_ALLOW_NEWLINES = 1 << 0, + FLATPAK_ESCAPE_DO_NOT_QUOTE = 1 << 1, +} FlatpakEscapeFlags; + +char * flatpak_escape_string (const char *s, + FlatpakEscapeFlags flags); +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__ */ Index: flatpak-1.4.2/common/flatpak-utils.c =================================================================== --- flatpak-1.4.2.orig/common/flatpak-utils.c +++ flatpak-1.4.2/common/flatpak-utils.c @@ -7196,4 +7196,121 @@ flatpak_dconf_path_is_similar (const cha return TRUE; } +static gboolean +is_char_safe (gunichar c) +{ + return g_unichar_isgraph (c) || c == ' '; +} + +static gboolean +should_hex_escape (gunichar c, + FlatpakEscapeFlags flags) +{ + if ((flags & FLATPAK_ESCAPE_ALLOW_NEWLINES) && c == '\n') + return FALSE; + + return !is_char_safe (c); +} + +static void +append_hex_escaped_character (GString *result, + gunichar c) +{ + if (c <= 0xFF) + g_string_append_printf (result, "\\x%02X", c); + else if (c <= 0xFFFF) + g_string_append_printf (result, "\\u%04X", c); + else + 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) +{ + g_autoptr(GString) res = g_string_new (""); + gboolean did_escape = FALSE; + + while (*s) + { + gunichar c = g_utf8_get_char_validated (s, -1); + if (c == (gunichar)-2 || c == (gunichar)-1) + { + /* Need to convert to unsigned first, to avoid negative chars becoming + huge gunichars. */ + append_hex_escaped_character (res, (unsigned char)*s++); + did_escape = TRUE; + continue; + } + else if (should_hex_escape (c, flags)) + { + append_hex_escaped_character (res, c); + did_escape = TRUE; + } + else if (c == '\\' || (!(flags & FLATPAK_ESCAPE_DO_NOT_QUOTE) && c == '\'')) + { + g_string_append_printf (res, "\\%c", (char) c); + did_escape = TRUE; + } + else + g_string_append_unichar (res, c); + s = g_utf8_find_next_char (s, NULL); + } + + if (did_escape && !(flags & FLATPAK_ESCAPE_DO_NOT_QUOTE)) + { + g_string_prepend_c (res, '\''); + g_string_append_c (res, '\''); + } + + return g_string_free (g_steal_pointer (&res), FALSE); +} + +void +flatpak_print_escaped_string (const char *s, + FlatpakEscapeFlags flags) +{ + 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; +} Index: flatpak-1.4.2/tests/make-test-app.sh =================================================================== --- flatpak-1.4.2.orig/tests/make-test-app.sh +++ flatpak-1.4.2/tests/make-test-app.sh @@ -34,6 +34,14 @@ autodelete=true locale-subset=true EOF +if [ x${INCLUDE_SPECIAL_CHARACTER-} != x ]; then +TAB=$'\t' +cat >> ${DIR}/metadata < ${DIR}/files/bin/hello.sh < info + +# CVE-2023-28101 +assert_file_has_content info "name=org\.test\.Hello" +assert_file_has_content info "^A=x\\\\x09y" + +ok "info --show-metadata" + ${FLATPAK} info --show-permissions org.test.Hello > info -assert_file_empty info +assert_file_has_content info "^A=x\\\\x09y" echo "ok info --show-permissions" Index: flatpak-1.4.2/tests/testcommon.c =================================================================== --- flatpak-1.4.2.orig/tests/testcommon.c +++ flatpak-1.4.2/tests/testcommon.c @@ -1138,6 +1138,78 @@ test_dconf_paths (void) } } +typedef struct { + const char *in; + FlatpakEscapeFlags flags; + const char *out; +} EscapeData; + +static EscapeData escapes[] = { + {"abc def", FLATPAK_ESCAPE_DEFAULT, "abc def"}, + {"やあ", FLATPAK_ESCAPE_DEFAULT, "やあ"}, + {"\033[;1m", FLATPAK_ESCAPE_DEFAULT, "'\\x1B[;1m'"}, + /* U+061C ARABIC LETTER MARK, non-printable */ + {"\u061C", FLATPAK_ESCAPE_DEFAULT, "'\\u061C'"}, + /* U+1343F EGYPTIAN HIEROGLYPH END WALLED ENCLOSURE, non-printable and + * outside BMP */ + {"\xF0\x93\x90\xBF", FLATPAK_ESCAPE_DEFAULT, "'\\U0001343F'"}, + /* 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 ' \\\\"}, + {"abc\tdef\n\033[;1m ghi\b", FLATPAK_ESCAPE_ALLOW_NEWLINES | FLATPAK_ESCAPE_DO_NOT_QUOTE, + "abc\\x09def\n\\x1B[;1m ghi\\x08"}, +}; + +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); + } +} + +/* CVE-2023-28101 */ +static void +test_string_escape (void) +{ + gsize idx; + + for (idx = 0; idx < G_N_ELEMENTS (escapes); idx++) + { + EscapeData *data = &escapes[idx]; + g_autofree char *ret = NULL; + + ret = flatpak_escape_string (data->in, data->flags); + g_assert_cmpstr (ret, ==, data->out); + } +} + int main (int argc, char *argv[]) { @@ -1165,6 +1237,8 @@ main (int argc, char *argv[]) g_test_add_func ("/common/filter", test_filter); g_test_add_func ("/common/dconf-app-id", test_dconf_app_id); g_test_add_func ("/common/dconf-paths", test_dconf_paths); + 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); Index: flatpak-1.4.2/common/flatpak-context.c =================================================================== --- flatpak-1.4.2.orig/common/flatpak-context.c +++ flatpak-1.4.2/common/flatpak-context.c @@ -475,11 +475,16 @@ flatpak_context_apply_generic_policy (Fl 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 @@ -745,6 +750,9 @@ static gboolean flatpak_context_verify_filesystem (const char *filesystem_and_mode, GError **error) { + if (!flatpak_validate_path_characters (filesystem_and_mode, error)) + return FALSE; + g_autofree char *filesystem = parse_filesystem_flags (filesystem_and_mode, NULL); if (strcmp (filesystem, "host") == 0) @@ -1216,8 +1224,7 @@ option_persist_cb (const gchar *option_n { 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; @@ -1406,8 +1413,22 @@ flatpak_context_load_metadata (FlatpakCo for (i = 0; filesystems[i] != NULL; i++) { const char *fs = parse_negated (filesystems[i], &remove); - if (!flatpak_context_verify_filesystem (fs, NULL)) - g_debug ("Unknown filesystem type %s", filesystems[i]); + g_autoptr(GError) local_error = NULL; + + if (!flatpak_context_verify_filesystem (fs, &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_info ("Unknown filesystem type %s", filesystems[i]); + g_clear_error (&local_error); + } + } else { if (remove) @@ -1426,7 +1447,8 @@ flatpak_context_load_metadata (FlatpakCo 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))