From 6cac99dafe6003c8a4bd5666341c217876536869 Mon Sep 17 00:00:00 2001
From: Ryan Gonzalez <ryan.gonzalez@collabora.com>
Date: Sat, 4 Mar 2023 16:23:37 -0600
Subject: [PATCH 1/3] 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>
---
 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             | 82 +++++++++++++++++++++++++++++-
 tests/make-test-app.sh             |  8 +++
 tests/test-info.sh                 | 14 +++--
 tests/testcommon.c                 | 39 ++++++++++++++
 8 files changed, 168 insertions(+), 11 deletions(-)

Index: flatpak-1.2.3/app/flatpak-builtins-info.c
===================================================================
--- flatpak-1.2.3.orig/app/flatpak-builtins-info.c
+++ flatpak-1.2.3/app/flatpak-builtins-info.c
@@ -415,7 +415,9 @@ flatpak_builtin_info (int argc, char **a
           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)
@@ -436,7 +438,9 @@ flatpak_builtin_info (int argc, char **a
               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)
Index: flatpak-1.2.3/app/flatpak-builtins-remote-info.c
===================================================================
--- flatpak-1.2.3.orig/app/flatpak-builtins-remote-info.c
+++ flatpak-1.2.3/app/flatpak-builtins-remote-info.c
@@ -404,7 +404,10 @@ flatpak_builtin_remote_info (int argc, c
             g_print ("\n");
 
           if (opt_show_metadata)
-            g_print ("%s", xa_metadata);
+            if (xa_metadata != NULL)
+              flatpak_print_escaped_string (xa_metadata,
+                                            FLATPAK_ESCAPE_ALLOW_NEWLINES
+                                            | FLATPAK_ESCAPE_DO_NOT_QUOTE);
 
           g_free (c);
           c = g_steal_pointer (&p);
Index: flatpak-1.2.3/app/flatpak-cli-transaction.c
===================================================================
--- flatpak-1.2.3.orig/app/flatpak-cli-transaction.c
+++ flatpak-1.2.3/app/flatpak-cli-transaction.c
@@ -516,8 +516,13 @@ end_of_lifed (FlatpakTransaction *transa
     msg = g_strdup_printf (_("Info: %s is end-of-life, in preference of %s"),
                            flatpak_ref_get_name (rref), rebase);
   else if (reason)
-    msg = g_strdup_printf (_("Info: %s is end-of-life, with reason: %s\n"),
-                           flatpak_ref_get_name (rref), reason);
+    {
+      g_autofree char *escaped_reason = flatpak_escape_string (reason,
+                                                               FLATPAK_ESCAPE_ALLOW_NEWLINES |
+                                                               FLATPAK_ESCAPE_DO_NOT_QUOTE);
+      msg = g_strdup_printf (_("Info: %s is end-of-life, with reason: %s\n"),
+                             flatpak_ref_get_name (rref), escaped_reason);
+    }
 
   if (flatpak_fancy_output ())
     {
@@ -638,12 +643,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;
 
@@ -652,10 +661,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);
Index: flatpak-1.2.3/common/flatpak-utils-private.h
===================================================================
--- flatpak-1.2.3.orig/common/flatpak-utils-private.h
+++ flatpak-1.2.3/common/flatpak-utils-private.h
@@ -698,7 +698,7 @@ gboolean flatpak_allocate_tmpdir (int
 gboolean flatpak_yes_no_prompt (gboolean default_yes,
                                 const char *prompt,
                                 ...) G_GNUC_PRINTF (2, 3);
-                            
+
 long flatpak_number_prompt (gboolean    default_yes,
                             int         min,
                             int         max,
@@ -733,6 +733,20 @@ gboolean flatpak_check_required_version
 
 int flatpak_levenshtein_distance (const char *s, const char *t);
 
-#define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485" 
+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 flatpak_validate_path_characters (const char *path,
+                                           GError    **error);
+
+#define FLATPAK_MESSAGE_ID "c7b39b1e006b464599465e105b361485"
 
 #endif /* __FLATPAK_UTILS_H__ */
Index: flatpak-1.2.3/common/flatpak-utils.c
===================================================================
--- flatpak-1.2.3.orig/common/flatpak-utils.c
+++ flatpak-1.2.3/common/flatpak-utils.c
@@ -995,7 +995,7 @@ flatpak_is_valid_branch (const char *str
   len = strlen (string);
   if (G_UNLIKELY (len == 0))
     {
-      flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME, 
+      flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME,
                           _("Branch can't be empty"));
       goto out;
     }
@@ -1005,7 +1005,7 @@ flatpak_is_valid_branch (const char *str
   s = string;
   if (G_UNLIKELY (!is_valid_initial_branch_character (*s)))
     {
-      flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME, 
+      flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME,
                           _("Branch can't start with %c"), *s);
       goto out;
     }
@@ -1015,7 +1015,7 @@ flatpak_is_valid_branch (const char *str
     {
       if (G_UNLIKELY (!is_valid_branch_character (*s)))
         {
-          flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME, 
+          flatpak_fail_error (error, FLATPAK_ERROR_INVALID_NAME,
                               _("Branch can't contain %c"), *s);
           goto out;
         }
@@ -1630,7 +1630,7 @@ flatpak_find_deploy_for_ref_in (GPtrArra
   for (i = 0; deploy == NULL && i < dirs->len; i++)
     {
       FlatpakDir *dir = g_ptr_array_index (dirs, i);
- 
+
       flatpak_log_dir_access (dir);
       g_clear_error (&my_error);
       deploy = flatpak_dir_load_deployed (dir, ref, commit, cancellable, &my_error);
@@ -6508,3 +6508,122 @@ out:
     return datetime;
 }
 #endif
+
+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);
+}
+
+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)
+{
+  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
+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;
+}
Index: flatpak-1.2.3/tests/make-test-app.sh
===================================================================
--- flatpak-1.2.3.orig/tests/make-test-app.sh
+++ flatpak-1.2.3/tests/make-test-app.sh
@@ -33,6 +33,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 org.test.Hello.Locale]
 directory=share/runtime/locale
Index: flatpak-1.2.3/tests/test-info.sh
===================================================================
--- flatpak-1.2.3.orig/tests/test-info.sh
+++ flatpak-1.2.3/tests/test-info.sh
@@ -4,9 +4,9 @@ set -euo pipefail
 
 . $(dirname $0)/libtest.sh
 
-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`
@@ -17,9 +17,17 @@ assert_file_has_content info "^app/org.t
 
 echo "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"
 
 echo "ok info --show-permissions"
 
Index: flatpak-1.2.3/tests/testcommon.c
===================================================================
--- flatpak-1.2.3.orig/tests/testcommon.c
+++ flatpak-1.2.3/tests/testcommon.c
@@ -343,18 +343,18 @@ test_parse_numbers (void)
   numbers = flatpak_parse_numbers ("1", 0, 10);
   assert_numbers (numbers, 1, 0);
   g_clear_pointer (&numbers, g_free);
-  
+
   numbers = flatpak_parse_numbers ("1 3 2", 0, 10);
   assert_numbers (numbers, 1, 3, 2, 0);
   g_clear_pointer (&numbers, g_free);
-  
+
   numbers = flatpak_parse_numbers ("1-3", 0, 10);
   assert_numbers (numbers, 1, 2, 3, 0);
   g_clear_pointer (&numbers, g_free);
-  
+
   numbers = flatpak_parse_numbers ("1", 2, 4);
   g_assert_null (numbers);
-  
+
   numbers = flatpak_parse_numbers ("2-6", 2, 4);
   g_assert_null (numbers);
 
@@ -396,23 +396,23 @@ test_subpaths_merge (void)
   res = flatpak_subpaths_merge (NULL, bla);
   assert_strv_equal (res, bla);
   g_clear_pointer (&res, g_strfreev);
-  
+
   res = flatpak_subpaths_merge (bla, NULL);
   assert_strv_equal (res, bla);
   g_clear_pointer (&res, g_strfreev);
-  
+
   res = flatpak_subpaths_merge (empty, bla);
   assert_strv_equal (res, empty);
   g_clear_pointer (&res, g_strfreev);
-  
+
   res = flatpak_subpaths_merge (bla, empty);
   assert_strv_equal (res, empty);
   g_clear_pointer (&res, g_strfreev);
-  
+
   res = flatpak_subpaths_merge (buba, bla);
   assert_strv_equal (res, bubabla);
   g_clear_pointer (&res, g_strfreev);
-  
+
   res = flatpak_subpaths_merge (bla, buba);
   assert_strv_equal (res, bubabla);
   g_clear_pointer (&res, g_strfreev);
@@ -485,7 +485,7 @@ test_parse_appdata (void)
   gboolean res;
   char *name;
   char *comment;
- 
+
   res = flatpak_parse_appdata (appdata1, "org.test.Hello", &names, &comments, &version, &license);
   g_assert_true (res);
   g_assert_cmpstr (version, ==, "0.0.1");
@@ -950,6 +950,78 @@ test_parse_datetime (void)
   g_assert_false (ret);
 }
 
+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'"},
+  /* U+061C ARABIC LETTER MARK, non-printable */
+  {"\u061C", FLATPAK_ESCAPE_DEFAULT, "'\\u061C'"},
+  /* U+1343F EGYPTIAN HIEROGLYPH END WALLED ENCLOSURE, non-printable and
+   * outside BMP */
+  {"\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);
+    }
+}
+
+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[])
 {
@@ -973,6 +1045,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/common/lang-from-locale", test_lang_from_locale);
   g_test_add_func ("/common/appdata", test_parse_appdata);
   g_test_add_func ("/common/name-matching", test_name_matching);
+  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);
Index: flatpak-1.2.3/common/flatpak-context.c
===================================================================
--- flatpak-1.2.3.orig/common/flatpak-context.c
+++ flatpak-1.2.3/common/flatpak-context.c
@@ -475,11 +475,16 @@ flatpak_context_apply_generic_policy (Fl
                        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
@@ -745,6 +750,9 @@ static gboolean
 flatpak_context_verify_filesystem (const char *filesystem_and_mode,
                                    GError    **error)
 {
+  if (!flatpak_validate_path_characters (filesystem_and_mode, error))
+    return FALSE;
+
   g_autofree char *filesystem = parse_filesystem_flags (filesystem_and_mode, NULL);
 
   if (strcmp (filesystem, "host") == 0)
@@ -1247,8 +1255,7 @@ option_persist_cb (const gchar *option_n
 {
   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;
@@ -1436,9 +1443,22 @@ flatpak_context_load_metadata (FlatpakCo
 
       for (i = 0; filesystems[i] != NULL; i++)
         {
+          g_autoptr(GError) local_error = NULL;
           const char *fs = parse_negated (filesystems[i], &remove);
-          if (!flatpak_context_verify_filesystem (fs, NULL))
-            g_debug ("Unknown filesystem type %s", filesystems[i]);
+          if (!flatpak_context_verify_filesystem (fs, &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
             {
               if (remove)
@@ -1457,7 +1477,8 @@ flatpak_context_load_metadata (FlatpakCo
         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))
