Skip to content
Permalink
Browse files Browse the repository at this point in the history
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 <ryan.gonzalez@collabora.com>
Co-authored-by: Simon McVittie <smcv@collabora.com>
  • Loading branch information
refi64 and smcv committed Mar 16, 2023
1 parent 6cac99d commit 7fe63f2
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 14 deletions.
36 changes: 29 additions & 7 deletions common/flatpak-context.c
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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_info ("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_info ("Unknown filesystem type %s", filesystems[i]);
g_clear_error (&local_error);
}
}
else
{
g_assert (mode == FLATPAK_FILESYSTEM_MODE_NONE || !remove);
Expand All @@ -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))
Expand Down
3 changes: 3 additions & 0 deletions common/flatpak-utils-private.h
Expand Up @@ -937,6 +937,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"
Expand Down
39 changes: 39 additions & 0 deletions common/flatpak-utils.c
Expand Up @@ -9250,6 +9250,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)
Expand Down Expand Up @@ -9301,6 +9309,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)
Expand Down
84 changes: 80 additions & 4 deletions tests/test-context.c
Expand Up @@ -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);
Expand All @@ -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));
Expand All @@ -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
Expand All @@ -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));
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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[])
{
Expand All @@ -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 ();
}
41 changes: 38 additions & 3 deletions tests/testcommon.c
Expand Up @@ -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 ' \\\\"},
Expand All @@ -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[])
{
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 7fe63f2

Please sign in to comment.