CVE-2021-21261: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: Flatpak is a system for building, distributing, and running sandboxed desktop applications on Linux. A bug was discovered in the `flatpak-portal` service that can allow sandboxed applications to execute arbitrary code on the host system (a sandbox escape). This sandbox-escape bug is present in versions from 0.11.4 and before fixed versions 1.8.5 and 1.10.0. The Flatpak portal D-Bus service (`flatpak-portal`, also known by its D-Bus service name `org.freedesktop.portal.Flatpak`) allows apps in a Flatpak sandbox to launch their own subprocesses in a new sandbox instance, either with the same security settings as the caller or with more restrictive security settings. For example, this is used in Flatpak-packaged web browsers such as Chromium to launch subprocesses that will process untrusted web content, and give those subprocesses a more restrictive sandbox than the browser itself. In vulnerable versions, the Flatpak portal service passes caller-specified environment variables to non-sandboxed processes on the host system, and in particular to the `flatpak run` command that is used to launch the new sandbox instance. A malicious or compromised Flatpak app could set environment variables that are trusted by the `flatpak run` command, and use them to execute arbitrary code that is not in a sandbox. As a workaround, this vulnerability can be mitigated by preventing the `flatpak-portal` service from starting, but that mitigation will prevent many Flatpak apps from working correctly. This is fixed in versions 1.8.5 and 1.10.0.
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 8.8
Published: 2021-01-14T20:15Z
Repository: https://github.com/flatpak/flatpak
Language: C
Files changed: common/flatpak-bwrap-private.h, common/flatpak-bwrap.c, common/flatpak-run.c
Commit date: 2021-01-14 09:33:24 +0100
Commit message: run: Convert all environment variables into bwrap arguments

This avoids some of them being filtered out by a setuid bwrap. It also
means that if they came from an untrusted source, they cannot be used
to inject arbitrary code into a non-setuid bwrap via mechanisms like
LD_PRELOAD.

Because they get bundled into a memfd or temporary file, they do not
actually appear in argv, ensuring that they remain inaccessible to
processes running under a different uid (which is important if their
values are tokens or other secrets).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2

Diff: diff --git a/common/flatpak-bwrap-private.h b/common/flatpak-bwrap-private.h
index 92d6e9de..6c31b789 100644
--- a/common/flatpak-bwrap-private.h
+++ b/common/flatpak-bwrap-private.h
@@ -41,10 +41,12 @@ gboolean      flatpak_bwrap_is_empty (FlatpakBwrap *bwrap);
 void          flatpak_bwrap_finish (FlatpakBwrap *bwrap);
 void          flatpak_bwrap_unset_env (FlatpakBwrap *bwrap,
                                        const char   *variable);
 void          flatpak_bwrap_add_arg (FlatpakBwrap *bwrap,
                                      const char   *arg);
+void          flatpak_bwrap_take_arg (FlatpakBwrap *bwrap,
+                                      char         *arg);
 void          flatpak_bwrap_add_noinherit_fd (FlatpakBwrap *bwrap,
                                               int           fd);
 void          flatpak_bwrap_add_fd (FlatpakBwrap *bwrap,
                                     int           fd);
 void          flatpak_bwrap_add_args (FlatpakBwrap *bwrap,
@@ -71,10 +73,11 @@ gboolean      flatpak_bwrap_add_args_data (FlatpakBwrap *bwrap,
                                            GError      **error);
 void          flatpak_bwrap_add_bind_arg (FlatpakBwrap *bwrap,
                                           const char   *type,
                                           const char   *src,
                                           const char   *dest);
+void          flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap);
 gboolean      flatpak_bwrap_bundle_args (FlatpakBwrap *bwrap,
                                          int           start,
                                          int           end,
                                          gboolean      one_arg,
                                          GError      **error);
diff --git a/common/flatpak-bwrap.c b/common/flatpak-bwrap.c
index c45154cc..6a02e868 100644
--- a/common/flatpak-bwrap.c
+++ b/common/flatpak-bwrap.c
@@ -107,10 +107,22 @@ void
 flatpak_bwrap_add_arg (FlatpakBwrap *bwrap, const char *arg)
 {
   g_ptr_array_add (bwrap->argv, g_strdup (arg));
 }
 
+/*
+ * flatpak_bwrap_take_arg:
+ * @arg: (transfer full): Take ownership of this argument
+ *
+ * Add @arg to @bwrap's argv, taking ownership of the pointer.
+ */
+void
+flatpak_bwrap_take_arg (FlatpakBwrap *bwrap, char *arg)
+{
+  g_ptr_array_add (bwrap->argv, arg);
+}
+
 void
 flatpak_bwrap_finish (FlatpakBwrap *bwrap)
 {
   g_ptr_array_add (bwrap->argv, NULL);
 }
@@ -272,10 +284,41 @@ flatpak_bwrap_add_bind_arg (FlatpakBwrap *bwrap,
       g_autofree char *dest_real = g_build_filename (dest_dirname_real, dest_basename, NULL);
       flatpak_bwrap_add_args (bwrap, type, src, dest_real, NULL);
     }
 }
 
+/*
+ * Convert bwrap->envp into a series of --setenv arguments for bwrap(1),
+ * assumed to be applied to an empty environment. Reset envp to be an
+ * empty environment.
+ */
+void
+flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap)
+{
+  gsize i;
+
+  for (i = 0; bwrap->envp[i] != NULL; i++)
+    {
+      char *key_val = bwrap->envp[i];
+      char *eq = strchr (key_val, '=');
+
+      if (eq)
+        {
+          flatpak_bwrap_add_arg (bwrap, "--setenv");
+          flatpak_bwrap_take_arg (bwrap, g_strndup (key_val, eq - key_val));
+          flatpak_bwrap_add_arg (bwrap, eq + 1);
+        }
+      else
+        {
+          g_warn_if_reached ();
+        }
+    }
+
+  g_strfreev (g_steal_pointer (&bwrap->envp));
+  bwrap->envp = g_strdupv (flatpak_bwrap_empty_env);
+}
+
 gboolean
 flatpak_bwrap_bundle_args (FlatpakBwrap *bwrap,
                            int           start,
                            int           end,
                            gboolean      one_arg,
diff --git a/common/flatpak-run.c b/common/flatpak-run.c
index e8e49a7f..c69f2c38 100644
--- a/common/flatpak-run.c
+++ b/common/flatpak-run.c
@@ -1461,28 +1461,10 @@ flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,
 
   flatpak_run_add_session_dbus_args (bwrap, proxy_arg_bwrap, context, flags, app_id);
   flatpak_run_add_system_dbus_args (bwrap, proxy_arg_bwrap, context, flags);
   flatpak_run_add_a11y_dbus_args (bwrap, proxy_arg_bwrap, context, flags);
 
-  if (g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH") != NULL)
-    {
-      /* LD_LIBRARY_PATH is overridden for setuid helper, so pass it as cmdline arg */
-      flatpak_bwrap_add_args (bwrap,
-                              "--setenv", "LD_LIBRARY_PATH", g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH"),
-                              NULL);
-      flatpak_bwrap_unset_env (bwrap, "LD_LIBRARY_PATH");
-    }
-
-  if (g_environ_getenv (bwrap->envp, "TMPDIR") != NULL)
-    {
-      /* TMPDIR is overridden for setuid helper, so pass it as cmdline arg */
-      flatpak_bwrap_add_args (bwrap,
-                              "--setenv", "TMPDIR", g_environ_getenv (bwrap->envp, "TMPDIR"),
-                              NULL);
-      flatpak_bwrap_unset_env (bwrap, "TMPDIR");
-    }
-
   /* Must run this before spawning the dbus proxy, to ensure it
      ends up in the app cgroup */
   if (!flatpak_run_in_transient_unit (app_id, &my_error))
     {
       /* We still run along even if we don't get a cgroup, as nothing
@@ -4066,10 +4048,12 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
           return FALSE;
         }
       command = default_command;
     }
 
+  flatpak_bwrap_envp_to_args (bwrap);
+
   if (!flatpak_bwrap_bundle_args (bwrap, 1, -1, FALSE, error))
     return FALSE;
 
   flatpak_bwrap_add_arg (bwrap, command);
 
@@ -4096,10 +4080,16 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
         spawn_flags |= G_SPAWN_DO_NOT_REAP_CHILD;
 
       /* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
       spawn_flags |= G_SPAWN_LEAVE_DESCRIPTORS_OPEN;
 
+      /* flatpak_bwrap_envp_to_args() moved the environment variables to
+       * be set into --setenv instructions in argv, so the environment
+       * in which the bwrap command runs must be empty. */
+      g_assert (bwrap->envp != NULL);
+      g_assert (bwrap->envp[0] == NULL);
+
       if (!g_spawn_async (NULL,
                           (char **) bwrap->argv->pdata,
                           bwrap->envp,
                           spawn_flags,
                           flatpak_bwrap_child_setup_cb, bwrap->fds,
@@ -4123,10 +4113,16 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
       /* Ensure we unset O_CLOEXEC for marked fds and rewind fds as needed.
        * Note that this does not close fds that are not already marked O_CLOEXEC, because
        * we do want to allow inheriting fds into flatpak run. */
       flatpak_bwrap_child_setup (bwrap->fds, FALSE);
 
+      /* flatpak_bwrap_envp_to_args() moved the environment variables to
+       * be set into --setenv instructions in argv, so the environment
+       * in which the bwrap command runs must be empty. */
+      g_assert (bwrap->envp != NULL);
+      g_assert (bwrap->envp[0] == NULL);
+
       if (execvpe (flatpak_get_bwrap (), (char **) bwrap->argv->pdata, bwrap->envp) == -1)
         {
           g_set_error_literal (error, G_IO_ERROR, g_io_error_from_errno (errno),
                                _("Unable to start app"));
           return FALSE;

Vulnerable code:   if (g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH") != NULL)
    {
      /* LD_LIBRARY_PATH is overridden for setuid helper, so pass it as cmdline arg */
      flatpak_bwrap_add_args (bwrap,
                              "--setenv", "LD_LIBRARY_PATH", g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH"),
                              NULL);
      flatpak_bwrap_unset_env (bwrap, "LD_LIBRARY_PATH");
    }

  if (g_environ_getenv (bwrap->envp, "TMPDIR") != NULL)
    {
      /* TMPDIR is overridden for setuid helper, so pass it as cmdline arg */
      flatpak_bwrap_add_args (bwrap,
                              "--setenv", "TMPDIR", g_environ_getenv (bwrap->envp, "TMPDIR"),
                              NULL);
      flatpak_bwrap_unset_env (bwrap, "TMPDIR");
    }

Fixed code: void          flatpak_bwrap_take_arg (FlatpakBwrap *bwrap,
                                      char         *arg);
void          flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap);
/*
 * flatpak_bwrap_take_arg:
 * @arg: (transfer full): Take ownership of this argument
 *
 * Add @arg to @bwrap's argv, taking ownership of the pointer.
 */
void
flatpak_bwrap_take_arg (FlatpakBwrap *bwrap, char *arg)
{
  g_ptr_array_add (bwrap->argv, arg);
}

/*
 * Convert bwrap->envp into a series of --setenv arguments for bwrap(1),
 * assumed to be applied to an empty environment. Reset envp to be an
 * empty environment.
 */
void
flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap)
{
  gsize i;

  for (i = 0; bwrap->envp[i] != NULL; i++)
    {
      char *key_val = bwrap->envp[i];
      char *eq = strchr (key_val, '=');

      if (eq)
        {
          flatpak_bwrap_add_arg (bwrap, "--setenv");
          flatpak_bwrap_take_arg (bwrap, g_strndup (key_val, eq - key_val));
          flatpak_bwrap_add_arg (bwrap, eq + 1);
        }
      else
        {
          g_warn_if_reached ();
        }
    }

  g_strfreev (g_steal_pointer (&bwrap->envp));
  bwrap->envp = g_strdupv (flatpak_bwrap_empty_env);
}

  flatpak_bwrap_envp_to_args (bwrap);

      /* flatpak_bwrap_envp_to_args() moved the environment variables to
       * be set into --setenv instructions in argv, so the environment
       * in which the bwrap command runs must be empty. */
      g_assert (bwrap->envp != NULL);
      g_assert (bwrap->envp[0] == NULL);

      /* flatpak_bwrap_envp_to_args() moved the environment variables to
       * be set into --setenv instructions in argv, so the environment
       * in which the bwrap command runs must be empty. */
      g_assert (bwrap->envp != NULL);
      g_assert (bwrap->envp[0] == NULL);