From 038655c9ee29ee659e5fede4d36871cf8ccef803 Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Mon, 7 Dec 2020 18:08:16 +0000

Subject: [PATCH 01/10] common: Add a backport of

 G_DBUS_METHOD_INVOCATION_HANDLED

This is syntactic sugar added in GLib 2.67.0, which makes it more clearly

correct when we return TRUE after a GDBus error.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

 common/flatpak-utils-base-private.h | 6 ++++++

 1 file changed, 6 insertions(+)

diff --git a/common/flatpak-utils-base-private.h b/common/flatpak-utils-base-private.h

index 181a0c95..9d10a530 100644

--- a/common/flatpak-utils-base-private.h

+++ b/common/flatpak-utils-base-private.h

@@ -22,6 +22,12 @@

 #define __FLATPAK_UTILS_BASE_H__

 #include <glib.h>

+#include <gio/gio.h>

+

+#ifndef G_DBUS_METHOD_INVOCATION_HANDLED

+# define G_DBUS_METHOD_INVOCATION_HANDLED TRUE

+# define G_DBUS_METHOD_INVOCATION_UNHANDLED FALSE

+#endif

 char *flatpak_get_timezone (void);

-- 

2.29.2

From 6ed3e235459f5fe8b58aecbab744166a5ee66f9d Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Sun, 10 Jan 2021 16:11:28 +0000

Subject: [PATCH 02/10] 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

---

 common/flatpak-bwrap-private.h |  3 +++

 common/flatpak-bwrap.c         | 43 ++++++++++++++++++++++++++++++++++

 common/flatpak-run.c           | 32 +++++++++++--------------

 3 files changed, 60 insertions(+), 18 deletions(-)

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

@@ -43,6 +43,8 @@ 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,

@@ -73,6 +75,7 @@ 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,

diff --git a/common/flatpak-bwrap.c b/common/flatpak-bwrap.c

index 7e5d38d1..d383d25f 100644

--- a/common/flatpak-bwrap.c

+++ b/common/flatpak-bwrap.c

@@ -109,6 +109,18 @@ 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)

 {

@@ -274,6 +286,37 @@ flatpak_bwrap_add_bind_arg (FlatpakBwrap *bwrap,

     }

 }

+/*

+ * 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,

diff --git a/common/flatpak-run.c b/common/flatpak-run.c

index 51c002ff..9383610b 100644

--- a/common/flatpak-run.c

+++ b/common/flatpak-run.c

@@ -1314,24 +1314,6 @@ flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,

   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))

@@ -3852,6 +3834,8 @@ flatpak_run_app (const char     *app_ref,

       command = default_command;

     }

+  flatpak_bwrap_envp_to_args (bwrap);

+

   if (!flatpak_bwrap_bundle_args (bwrap, 1, -1, FALSE, error))

     return FALSE;

@@ -3882,6 +3866,12 @@ flatpak_run_app (const char     *app_ref,

       /* 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,

@@ -3909,6 +3899,12 @@ flatpak_run_app (const char     *app_ref,

        * 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),

-- 

2.29.2

From a1dec4885218f40b03044c82245d5b362c0affeb Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Mon, 11 Jan 2021 12:14:48 +0000

Subject: [PATCH 03/10] tests: Expand coverage for environment variable

 overrides

This checks that `flatpak run --env=` takes precedence over

`flatpak override --env=`, and that environment variables don't get

onto the bwrap command-line (which would be information disclosure

if their values are secret).

Signed-off-by: Simon McVittie <smcv@collabora.com>

Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2

---

 tests/test-override.sh | 68 +++++++++++++++++++++++++++++++++++++++++-

 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/tests/test-override.sh b/tests/test-override.sh

index 93009ee3..1213bbd4 100755

--- a/tests/test-override.sh

+++ b/tests/test-override.sh

@@ -12,7 +12,7 @@ reset_overrides () {

     assert_file_empty info

 }

-echo "1..13"

+echo "1..15"

 setup_repo

 install_repo

@@ -65,14 +65,80 @@ reset_overrides

 ${FLATPAK} override --user --env=FOO=BAR org.test.Hello

 ${FLATPAK} override --user --env=BAR= org.test.Hello

+# TODO: A future commit will add a way to avoid this ever being present in argv

+${FLATPAK} override --user --env=SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6 org.test.Hello

+# TMPDIR and TZDIR are filtered out by ld.so for setuid processes,

+# so setting these gives us a way to verify that we can pass them through

+# a setuid bwrap (without special-casing them, as we previously did for

+# TMPDIR).

+${FLATPAK} override --user --env=TMPDIR=/nonexistent/tmp org.test.Hello

+${FLATPAK} override --user --env=TZDIR=/nonexistent/tz org.test.Hello

 ${FLATPAK} override --user --show org.test.Hello > override

 assert_file_has_content override "^\[Environment\]$"

 assert_file_has_content override "^FOO=BAR$"

 assert_file_has_content override "^BAR=$"

+assert_file_has_content override "^SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6$"

+assert_file_has_content override "^TMPDIR=/nonexistent/tmp$"

+assert_file_has_content override "^TZDIR=/nonexistent/tz$"

 echo "ok override --env"

+if skip_one_without_bwrap "sandbox environment variables"; then

+  :

+else

+  ${FLATPAK} run --command=bash org.test.Hello \

+      -c 'echo "FOO=$FOO"; echo "BAR=$BAR"; echo "SECRET_TOKEN=$SECRET_TOKEN"; echo "TMPDIR=$TMPDIR"; echo "TZDIR=$TZDIR"' > out

+  assert_file_has_content out '^FOO=BAR$'

+  assert_file_has_content out '^BAR=$'

+  assert_file_has_content out '^SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6$'

+  # The variables that would be filtered out by a setuid bwrap get set

+  assert_file_has_content out '^TZDIR=/nonexistent/tz$'

+  assert_file_has_content out '^TMPDIR=/nonexistent/tmp$'

+  ${FLATPAK} run --command=cat org.test.Hello -- /proc/1/cmdline > out

+  # The secret doesn't end up in bubblewrap's cmdline where other users

+  # could see it

+  assert_not_file_has_content out 3047225e-5e38-4357-b21c-eac83b7e8ea6

+

+  ok "sandbox environment variables"

+fi

+

+reset_overrides

+

+if skip_one_without_bwrap "temporary environment variables"; then

+  :

+else

+  ${FLATPAK} override --user --env=FOO=wrong org.test.Hello

+  ${FLATPAK} override --user --env=BAR=wrong org.test.Hello

+  ${FLATPAK} override --user --env=SECRET_TOKEN=wrong org.test.Hello

+  ${FLATPAK} override --user --env=TMPDIR=/nonexistent/wrong org.test.Hello

+  ${FLATPAK} override --user --env=TZDIR=/nonexistent/wrong org.test.Hello

+  ${FLATPAK} override --user --show org.test.Hello > override

+

+  ${FLATPAK} run --command=bash \

+      --env=FOO=BAR \

+      --env=BAR= \

+      --env=SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6 \

+      --env=TMPDIR=/nonexistent/tmp \

+      --env=TZDIR=/nonexistent/tz \

+      org.test.Hello \

+      -c 'echo "FOO=$FOO"; echo "BAR=$BAR"; echo "SECRET_TOKEN=$SECRET_TOKEN"; echo "TMPDIR=$TMPDIR"; echo "TZDIR=$TZDIR"' > out

+  # The versions from `flatpak run` overrule `flatpak override`

+  assert_file_has_content out '^FOO=BAR$'

+  assert_file_has_content out '^BAR=$'

+  assert_file_has_content out '^SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6$'

+  assert_file_has_content out '^TZDIR=/nonexistent/tz$'

+  assert_file_has_content out '^TMPDIR=/nonexistent/tmp$'

+  ${FLATPAK} run --command=cat org.test.Hello -- /proc/1/cmdline > out

+  # The secret doesn't end up in bubblewrap's cmdline where other users

+  # could see it

+  assert_not_file_has_content out 3047225e-5e38-4357-b21c-eac83b7e8ea6

+

+  ok "temporary environment variables"

+fi

+

+reset_overrides

+

 ${FLATPAK} override --user --filesystem=home org.test.Hello

 ${FLATPAK} override --user --filesystem=xdg-desktop/foo:create org.test.Hello

 ${FLATPAK} override --user --filesystem=xdg-config:ro org.test.Hello

-- 

2.29.2

From 1b80c139f02cff6eeec39ecdf1a8f531ab6d7d9b Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Sun, 10 Jan 2021 16:18:58 +0000

Subject: [PATCH 04/10] context: Add --env-fd option

This allows environment variables to be added to the context without

making their values visible to processes running under a different uid,

which might be significant if the variable's value is a token or some

other secret value.

Signed-off-by: Simon McVittie <smcv@collabora.com>

Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2

---

 common/flatpak-context.c     | 60 ++++++++++++++++++++++++++++++++++++

 doc/flatpak-build-finish.xml | 18 +++++++++++

 doc/flatpak-build.xml        | 18 +++++++++++

 doc/flatpak-override.xml     | 18 +++++++++++

 doc/flatpak-run.xml          | 18 +++++++++++

 5 files changed, 132 insertions(+)

diff --git a/common/flatpak-context.c b/common/flatpak-context.c

index 462134aa..7e1bc46c 100644

--- a/common/flatpak-context.c

+++ b/common/flatpak-context.c

@@ -1039,6 +1039,65 @@ option_env_cb (const gchar *option_name,

   return TRUE;

 }

+static gboolean

+option_env_fd_cb (const gchar *option_name,

+                  const gchar *value,

+                  gpointer     data,

+                  GError     **error)

+{

+  FlatpakContext *context = data;

+  g_autoptr(GBytes) env_block = NULL;

+  gsize remaining;

+  const char *p;

+  guint64 fd;

+  gchar *endptr;

+

+  fd = g_ascii_strtoull (value, &endptr, 10);

+

+  if (endptr == NULL || *endptr != '\0' || fd > G_MAXINT)

+    return glnx_throw (error, "Not a valid file descriptor: %s", value);

+

+  env_block = glnx_fd_readall_bytes ((int) fd, NULL, error);

+

+  if (env_block == NULL)

+    return FALSE;

+

+  p = g_bytes_get_data (env_block, &remaining);

+

+  /* env_block might not be \0-terminated */

+  while (remaining > 0)

+    {

+      size_t len = strnlen (p, remaining);

+      const char *equals;

+

+      g_assert (len <= remaining);

+

+      equals = memchr (p, '=', len);

+

+      if (equals == NULL || equals == p)

+        return glnx_throw (error,

+                           "Environment variable must be given in the form VARIABLE=VALUE, not %.*s", (int) len, p);

+

+      flatpak_context_set_env_var (context,

+                                   g_strndup (p, equals - p),

+                                   g_strndup (equals + 1, len - (equals - p) - 1));

+      p += len;

+      remaining -= len;

+

+      if (remaining > 0)

+        {

+          g_assert (*p == '\0');

+          p += 1;

+          remaining -= 1;

+        }

+    }

+

+  if (fd >= 3)

+    close (fd);

+

+  return TRUE;

+}

+

 static gboolean

 option_own_name_cb (const gchar *option_name,

                     const gchar *value,

@@ -1236,6 +1295,7 @@ static GOptionEntry context_options[] = {

   { "filesystem", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_filesystem_cb, N_("Expose filesystem to app (:ro for read-only)"), N_("FILESYSTEM[:ro]") },

   { "nofilesystem", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_nofilesystem_cb, N_("Don't expose filesystem to app"), N_("FILESYSTEM") },

   { "env", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_env_cb, N_("Set environment variable"), N_("VAR=VALUE") },

+  { "env-fd", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_env_fd_cb, N_("Read environment variables in env -0 format from FD"), N_("FD") },

   { "own-name", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_own_name_cb, N_("Allow app to own name on the session bus"), N_("DBUS_NAME") },

   { "talk-name", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_talk_name_cb, N_("Allow app to talk to name on the session bus"), N_("DBUS_NAME") },

   { "no-talk-name", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_no_talk_name_cb, N_("Don't allow app to talk to name on the session bus"), N_("DBUS_NAME") },

diff --git a/doc/flatpak-build-finish.xml b/doc/flatpak-build-finish.xml

index d92eeb4d..2e01cd3e 100644

--- a/doc/flatpak-build-finish.xml

+++ b/doc/flatpak-build-finish.xml

@@ -286,6 +286,24 @@ key=v1;v2;

                 </para></listitem>

             </varlistentry>

+            <varlistentry>

+                <term><option>--env-fd=<replaceable>FD</replaceable></option></term>

+

+                <listitem><para>

+                    Read environment variables from the file descriptor

+                    <replaceable>FD</replaceable>, and set them as if

+                    via <option>--env</option>. This can be used to avoid

+                    environment variables and their values becoming visible

+                    to other users.

+                </para><para>

+                    Each environment variable is in the form

+                    <replaceable>VAR</replaceable>=<replaceable>VALUE</replaceable>

+                    followed by a zero byte. This is the same format used by

+                    <literal>env -0</literal> and

+                    <filename>/proc/*/environ</filename>.

+                </para></listitem>

+            </varlistentry>

+

             <varlistentry>

                 <term><option>--own-name=NAME</option></term>

diff --git a/doc/flatpak-build.xml b/doc/flatpak-build.xml

index 55e3ec89..e3eb9bc8 100644

--- a/doc/flatpak-build.xml

+++ b/doc/flatpak-build.xml

@@ -288,6 +288,24 @@ key=v1;v2;

                 </para></listitem>

             </varlistentry>

+            <varlistentry>

+                <term><option>--env-fd=<replaceable>FD</replaceable></option></term>

+

+                <listitem><para>

+                    Read environment variables from the file descriptor

+                    <replaceable>FD</replaceable>, and set them as if

+                    via <option>--env</option>. This can be used to avoid

+                    environment variables and their values becoming visible

+                    to other users.

+                </para><para>

+                    Each environment variable is in the form

+                    <replaceable>VAR</replaceable>=<replaceable>VALUE</replaceable>

+                    followed by a zero byte. This is the same format used by

+                    <literal>env -0</literal> and

+                    <filename>/proc/*/environ</filename>.

+                </para></listitem>

+            </varlistentry>

+

             <varlistentry>

                 <term><option>--own-name=NAME</option></term>

diff --git a/doc/flatpak-override.xml b/doc/flatpak-override.xml

index 8f131575..137e1251 100644

--- a/doc/flatpak-override.xml

+++ b/doc/flatpak-override.xml

@@ -262,6 +262,24 @@ key=v1;v2;

                 </para></listitem>

             </varlistentry>

+            <varlistentry>

+                <term><option>--env-fd=<replaceable>FD</replaceable></option></term>

+

+                <listitem><para>

+                    Read environment variables from the file descriptor

+                    <replaceable>FD</replaceable>, and set them as if

+                    via <option>--env</option>. This can be used to avoid

+                    environment variables and their values becoming visible

+                    to other users.

+                </para><para>

+                    Each environment variable is in the form

+                    <replaceable>VAR</replaceable>=<replaceable>VALUE</replaceable>

+                    followed by a zero byte. This is the same format used by

+                    <literal>env -0</literal> and

+                    <filename>/proc/*/environ</filename>.

+                </para></listitem>

+            </varlistentry>

+

             <varlistentry>

                 <term><option>--own-name=NAME</option></term>

diff --git a/doc/flatpak-run.xml b/doc/flatpak-run.xml

index 5077c1a9..ed157341 100644

--- a/doc/flatpak-run.xml

+++ b/doc/flatpak-run.xml

@@ -402,6 +402,24 @@ key=v1;v2;

                 </para></listitem>

             </varlistentry>

+            <varlistentry>

+                <term><option>--env-fd=<replaceable>FD</replaceable></option></term>

+

+                <listitem><para>

+                    Read environment variables from the file descriptor

+                    <replaceable>FD</replaceable>, and set them as if

+                    via <option>--env</option>. This can be used to avoid

+                    environment variables and their values becoming visible

+                    to other users.

+                </para><para>

+                    Each environment variable is in the form

+                    <replaceable>VAR</replaceable>=<replaceable>VALUE</replaceable>

+                    followed by a zero byte. This is the same format used by

+                    <literal>env -0</literal> and

+                    <filename>/proc/*/environ</filename>.

+                </para></listitem>

+            </varlistentry>

+

             <varlistentry>

                 <term><option>--own-name=NAME</option></term>

-- 

2.29.2

From 6c244791c912fe0c5ae2e140f251dc02c46cc0aa Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Tue, 12 Jan 2021 10:11:51 +0000

Subject: [PATCH 05/10] portal: Convert --env in extra-args into --env-fd

This hides overridden variables from the command-line, which means

processes running under other uids can't see them in /proc/*/cmdline,

which might be important if they contain secrets.

Signed-off-by: Simon McVittie <smcv@collabora.com>

Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2

---

 portal/flatpak-portal.c | 51 ++++++++++++++++++++++++++++++++++++++++-

 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c

index 060987b7..03ea58cc 100644

--- a/portal/flatpak-portal.c

+++ b/portal/flatpak-portal.c

@@ -249,6 +249,7 @@ typedef struct

   int         fd_map_len;

   gboolean    set_tty;

   int         tty;

+  int         env_fd;

 } ChildSetupData;

 static void

@@ -267,6 +268,9 @@ child_setup_func (gpointer user_data)

   flatpak_close_fds_workaround (3);

+  if (data->env_fd != -1)

+    drop_cloexec (data->env_fd);

+

   /* Unblock all signals */

   sigemptyset (&set);

   if (pthread_sigmask (SIG_SETMASK, &set, NULL) == -1)

@@ -553,6 +557,9 @@ handle_spawn (PortalFlatpak         *object,

   gboolean sandboxed;

   gboolean devel;

   gboolean expose_pids;

+  g_autoptr(GString) env_string = g_string_new ("");

+

+  child_setup_data.env_fd = -1;

   if (fd_list != NULL)

     fds = g_unix_fd_list_peek_fds (fd_list, &fds_len);

@@ -805,7 +812,49 @@ handle_spawn (PortalFlatpak         *object,

   else

     {

       for (i = 0; extra_args != NULL && extra_args[i] != NULL; i++)

-        g_ptr_array_add (flatpak_argv, g_strdup (extra_args[i]));

+        {

+          if (g_str_has_prefix (extra_args[i], "--env="))

+            {

+              const char *var_val = extra_args[i] + strlen ("--env=");

+

+              if (var_val[0] == '\0' || var_val[0] == '=')

+                {

+                  g_warning ("Environment variable in extra-args has empty name");

+                  continue;

+                }

+

+              if (strchr (var_val, '=') == NULL)

+                {

+                  g_warning ("Environment variable in extra-args has no value");

+                  continue;

+                }

+

+              g_string_append (env_string, var_val);

+              g_string_append_c (env_string, '\0');

+            }

+          else

+            {

+              g_ptr_array_add (flatpak_argv, g_strdup (extra_args[i]));

+            }

+        }

+    }

+

+  if (env_string->len > 0)

+    {

+      g_auto(GLnxTmpfile) env_tmpf  = { 0, };

+

+      if (!flatpak_buffer_to_sealed_memfd_or_tmpfile (&env_tmpf, "environ",

+                                                      env_string->str,

+                                                      env_string->len, &error))

+        {

+          g_dbus_method_invocation_return_gerror (invocation, error);

+          return G_DBUS_METHOD_INVOCATION_HANDLED;

+        }

+

+      child_setup_data.env_fd = glnx_steal_fd (&env_tmpf.fd);

+      g_ptr_array_add (flatpak_argv,

+                       g_strdup_printf ("--env-fd=%d",

+                                        child_setup_data.env_fd));

     }

   expose_pids = (arg_flags & FLATPAK_SPAWN_FLAGS_EXPOSE_PIDS) != 0;

-- 

2.29.2

From f1725cd4fc6164d33f7a92bba673e8718655c1f1 Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Mon, 11 Jan 2021 12:25:50 +0000

Subject: [PATCH 06/10] tests: Exercise --env-fd

Signed-off-by: Simon McVittie <smcv@collabora.com>

Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2

---

 tests/test-override.sh | 18 ++++++++++--------

 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tests/test-override.sh b/tests/test-override.sh

index 1213bbd4..1c7dafd3 100755

--- a/tests/test-override.sh

+++ b/tests/test-override.sh

@@ -65,14 +65,16 @@ reset_overrides

 ${FLATPAK} override --user --env=FOO=BAR org.test.Hello

 ${FLATPAK} override --user --env=BAR= org.test.Hello

-# TODO: A future commit will add a way to avoid this ever being present in argv

-${FLATPAK} override --user --env=SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6 org.test.Hello

+# --env-fd with terminating \0 (strictly as documented).

+printf '%s\0' "SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6" > env.3

+# --env-fd without terminating \0 (which we also accept).

 # TMPDIR and TZDIR are filtered out by ld.so for setuid processes,

 # so setting these gives us a way to verify that we can pass them through

 # a setuid bwrap (without special-casing them, as we previously did for

 # TMPDIR).

-${FLATPAK} override --user --env=TMPDIR=/nonexistent/tmp org.test.Hello

-${FLATPAK} override --user --env=TZDIR=/nonexistent/tz org.test.Hello

+printf '%s\0%s' "TMPDIR=/nonexistent/tmp" "TZDIR=/nonexistent/tz" > env.4

+${FLATPAK} override --user --env-fd=3 --env-fd=4 org.test.Hello \

+    3

 ${FLATPAK} override --user --show org.test.Hello > override

 assert_file_has_content override "^\[Environment\]$"

@@ -118,11 +120,11 @@ else

   ${FLATPAK} run --command=bash \

       --env=FOO=BAR \

       --env=BAR= \

-      --env=SECRET_TOKEN=3047225e-5e38-4357-b21c-eac83b7e8ea6 \

-      --env=TMPDIR=/nonexistent/tmp \

-      --env=TZDIR=/nonexistent/tz \

+      --env-fd=3 \

+      --env-fd=4 \

       org.test.Hello \

-      -c 'echo "FOO=$FOO"; echo "BAR=$BAR"; echo "SECRET_TOKEN=$SECRET_TOKEN"; echo "TMPDIR=$TMPDIR"; echo "TZDIR=$TZDIR"' > out

+      -c 'echo "FOO=$FOO"; echo "BAR=$BAR"; echo "SECRET_TOKEN=$SECRET_TOKEN"; echo "TMPDIR=$TMPDIR"; echo "TZDIR=$TZDIR"' \

+      3<env.3 4<env.4 > out

   # The versions from `flatpak run` overrule `flatpak override`

   assert_file_has_content out '^FOO=BAR$'

   assert_file_has_content out '^BAR=$'

-- 

2.29.2

From adcb3b2608caa1ab6647f59e31a9084ec0b66bbb Mon Sep 17 00:00:00 2001

From: Simon McVittie <smcv@collabora.com>

Date: Sun, 10 Jan 2021 16:25:29 +0000

Subject: [PATCH 07/10] portal: Do not use caller-supplied variables in

 environment

If the caller specifies a variable that can be used to inject arbitrary

code into processes, we must not allow it to enter the environment

block used to run `flatpak run`, which runs unsandboxed.

This change requires the previous commit "context: Add --env-fd option",

which adds infrastructure used here.

To be secure, this change also requires the previous commit

"run: Convert all environment variables into bwrap arguments", which

protects a non-setuid bwrap(1) from the same attack.

Signed-off-by: Simon McVittie <smcv@collabora.com>

Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2