Skip to content

Commit

Permalink
Ensure special characters in permissions and metadata are escaped
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
refi64 authored and smcv committed Mar 16, 2023
1 parent 3abfddb commit 6cac99d
Show file tree
Hide file tree
Showing 8 changed files with 168 additions and 11 deletions.
8 changes: 6 additions & 2 deletions app/flatpak-builtins-info.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion app/flatpak-builtins-remote-info.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
12 changes: 8 additions & 4 deletions app/flatpak-cli-transaction.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
Expand Down
11 changes: 11 additions & 0 deletions common/flatpak-utils-private.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
82 changes: 81 additions & 1 deletion common/flatpak-utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
{
Expand Down
8 changes: 8 additions & 0 deletions tests/make-test-app.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 11 additions & 3 deletions tests/test-info.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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"

Expand Down
39 changes: 39 additions & 0 deletions tests/testcommon.c
Original file line number Diff line number Diff line change
Expand Up @@ -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[])
{
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 6cac99d

Please sign in to comment.