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>

5 files changed, 189 insertions, 14 deletions

diff --git a/common/flatpak-context.c b/common/flatpak-context.c
index ddc891db..666404e8 100644
--- a/common/flatpak-context.c
+++ b/common/flatpak-context.c
@@ -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
@@ -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;
@@ -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;
@@ -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);
@@ -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))
diff --git a/common/flatpak-utils-private.h b/common/flatpak-utils-private.h
index 1ded8afc..b37302f0 100644
--- a/common/flatpak-utils-private.h
+++ b/common/flatpak-utils-private.h
@@ -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"
diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c
index fa8c465a..f28fdc07 100644
--- a/common/flatpak-utils.c
+++ b/common/flatpak-utils.c
@@ -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)
@@ -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)
diff --git a/tests/test-context.c b/tests/test-context.c
index 785ca509..d7afaa06 100644
--- a/tests/test-context.c
+++ b/tests/test-context.c
@@ -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);
@@ -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));
@@ -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
@@ -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));
@@ -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));
@@ -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[])
 {
@@ -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 ();
 }
diff --git a/tests/testcommon.c b/tests/testcommon.c
index d8ad09c7..8ba3770d 100644
--- a/tests/testcommon.c
+++ b/tests/testcommon.c
@@ -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 ' \\\\"},
@@ -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[])
 {
@@ -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);