From acd627a2fabe9856947399044dbf7aa79247c75b Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Sat, 4 Mar 2023 16:23:37 -0600 Subject: [PATCH] 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 | 77 ++++++++++++++++++++++++++++++ tests/make-test-app.sh | 8 ++++ tests/test-info.sh | 14 ++++-- tests/testcommon.c | 39 +++++++++++++++ 8 files changed, 164 insertions(+), 10 deletions(-) diff --git a/app/flatpak-builtins-info.c b/app/flatpak-builtins-info.c index c13d2d89eb..35d49c446a 100644 --- a/app/flatpak-builtins-info.c +++ b/app/flatpak-builtins-info.c @@ -400,7 +400,9 @@ flatpak_builtin_info (int argc, char **argv, GCancellable *cancellable, GError * 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) @@ -421,7 +423,9 @@ flatpak_builtin_info (int argc, char **argv, GCancellable *cancellable, GError * if (contents == NULL) return FALSE; - g_print ("%s", contents); + flatpak_print_escaped_string (contents, + FLATPAK_ESCAPE_ALLOW_NEWLINES + | FLATPAK_ESCAPE_DO_NOT_QUOTE); } if (opt_file_access) diff --git a/app/flatpak-builtins-remote-info.c b/app/flatpak-builtins-remote-info.c index 20705a97ca..0ab05b7ca4 100644 --- a/app/flatpak-builtins-remote-info.c +++ b/app/flatpak-builtins-remote-info.c @@ -441,7 +441,10 @@ flatpak_builtin_remote_info (int argc, char **argv, GCancellable *cancellable, G 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"); } diff --git a/app/flatpak-cli-transaction.c b/app/flatpak-cli-transaction.c index a258f905c4..b915bedd04 100644 --- a/app/flatpak-cli-transaction.c +++ b/app/flatpak-cli-transaction.c @@ -894,12 +894,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; @@ -908,10 +912,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); diff --git a/common/flatpak-utils-private.h b/common/flatpak-utils-private.h index 1a92154706..c1282641f9 100644 --- a/common/flatpak-utils-private.h +++ b/common/flatpak-utils-private.h @@ -902,6 +902,17 @@ null_safe_g_ptr_array_unref (gpointer data) g_clear_pointer (&data, g_ptr_array_unref); } +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); + #define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485" #endif /* __FLATPAK_UTILS_H__ */ diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 3a5adb82ee..0b91ae3f91 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -8613,4 +8613,81 @@ flatpak_dconf_path_is_similar (const char *path1, return (path1[i1] == '\0'); } +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); +} + +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); +} diff --git a/tests/make-test-app.sh b/tests/make-test-app.sh index 731160535c..125e97f6b5 100755 --- a/tests/make-test-app.sh +++ b/tests/make-test-app.sh @@ -40,6 +40,14 @@ required-flatpak=$REQUIRED_VERSION EOF fi +if [ x${INCLUDE_SPECIAL_CHARACTER-} != x ]; then +TAB=$'\t' +cat >> ${DIR}/metadata <> ${DIR}/metadata < 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" ok "info --show-permissions" diff --git a/tests/testcommon.c b/tests/testcommon.c index 1d217c072a..daae5e4aac 100644 --- a/tests/testcommon.c +++ b/tests/testcommon.c @@ -1557,6 +1557,44 @@ 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'"}, + // non-printable U+061C + {"\u061C", FLATPAK_ESCAPE_DEFAULT, "'\\u061C'"}, + // non-printable U+1343F + {"\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"}, +}; + +/* 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[]) { @@ -1585,6 +1623,7 @@ main (int argc, char *argv[]) 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/decompose-ref", test_decompose); + g_test_add_func ("/common/string-escape", test_string_escape); g_test_add_func ("/app/looks-like-branch", test_looks_like_branch); g_test_add_func ("/app/columns", test_columns);