diff options
author | Ryan Gonzalez <ryan.gonzalez@collabora.com> | 2023-03-04 16:23:37 -0600 |
---|---|---|
committer | Simon McVittie <smcv@collabora.com> | 2023-03-16 09:54:14 +0000 |
commit | 6cac99dafe6003c8a4bd5666341c217876536869 (patch) | |
tree | c92a66516af774fa92e4b24898f4a55419b335c6 | |
parent | 3abfddba92411c16f0690913f9d092ed055df587 (diff) | |
download | flatpak-6cac99dafe6003c8a4bd5666341c217876536869.tar.gz |
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 <ryan.gonzalez@collabora.com>
-rw-r--r-- | app/flatpak-builtins-info.c | 8 | ||||
-rw-r--r-- | app/flatpak-builtins-remote-info.c | 5 | ||||
-rw-r--r-- | app/flatpak-cli-transaction.c | 12 | ||||
-rw-r--r-- | common/flatpak-utils-private.h | 11 | ||||
-rw-r--r-- | common/flatpak-utils.c | 82 | ||||
-rwxr-xr-x | tests/make-test-app.sh | 8 | ||||
-rw-r--r-- | tests/test-info.sh | 14 | ||||
-rw-r--r-- | tests/testcommon.c | 39 |
8 files changed, 168 insertions, 11 deletions
diff --git a/app/flatpak-builtins-info.c b/app/flatpak-builtins-info.c index 5f544579..ed905836 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 c15eb8a2..bec93e89 100644 --- a/app/flatpak-builtins-remote-info.c +++ b/app/flatpak-builtins-remote-info.c @@ -431,7 +431,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 5a08d537..53593bfc 100644 --- a/app/flatpak-cli-transaction.c +++ b/app/flatpak-cli-transaction.c @@ -1121,12 +1121,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; @@ -1135,10 +1139,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 ceecb5c6..1ded8afc 100644 --- a/common/flatpak-utils-private.h +++ b/common/flatpak-utils-private.h @@ -926,6 +926,17 @@ gboolean flatpak_str_is_integer (const char *s); gboolean flatpak_uri_equal (const char *uri1, const char *uri2); +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 running_under_sudo (void); #define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485" diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 53b73ff0..fa8c465a 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -613,7 +613,7 @@ load_kernel_module_list (void) g_autofree char *modules_data = NULL; g_autoptr(GError) error = NULL; char *start, *end; - + if (!g_file_get_contents ("/proc/modules", &modules_data, NULL, &error)) { g_info ("Failed to read /proc/modules: %s", error->message); @@ -9222,6 +9222,86 @@ flatpak_uri_equal (const char *uri1, return g_strcmp0 (uri1_norm, uri2_norm) == 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); +} + + gboolean running_under_sudo (void) { diff --git a/tests/make-test-app.sh b/tests/make-test-app.sh index afa11a6b..06cae5f9 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 <<EOF +[Environment] +A=x${TAB}y +EOF +fi + cat >> ${DIR}/metadata <<EOF [Extension $APP_ID.Locale] directory=share/runtime/locale diff --git a/tests/test-info.sh b/tests/test-info.sh index 4a247bdb..2158c11b 100644 --- a/tests/test-info.sh +++ b/tests/test-info.sh @@ -6,9 +6,9 @@ set -euo pipefail skip_revokefs_without_fuse -echo "1..7" +echo "1..8" -setup_repo +INCLUDE_SPECIAL_CHARACTER=1 setup_repo install_repo COMMIT=`${FLATPAK} ${U} info --show-commit org.test.Hello` @@ -19,9 +19,17 @@ assert_file_has_content info "^app/org\.test\.Hello/$(flatpak --default-arch)/ma ok "info -rcos" +${FLATPAK} info --show-metadata org.test.Hello > 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 c527b20e..d8ad09c7 100644 --- a/tests/testcommon.c +++ b/tests/testcommon.c @@ -1837,6 +1837,44 @@ test_parse_x11_display (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[]) { @@ -1870,6 +1908,7 @@ main (int argc, char *argv[]) g_test_add_func ("/common/quote-argv", test_quote_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 ("/app/looks-like-branch", test_looks_like_branch); g_test_add_func ("/app/columns", test_columns); |