From: Simon McVittie Date: Tue, 12 Jan 2021 12:21:31 +0000 Subject: 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). [Backported to 1.2.x for Debian 10 security update.] Signed-off-by: Simon McVittie 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 | 24 ++++++++++++++--------- 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/common/flatpak-bwrap-private.h b/common/flatpak-bwrap-private.h index 2a633d3..b6f3df9 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 f419d23..57cd750 100644 --- a/common/flatpak-bwrap.c +++ b/common/flatpak-bwrap.c @@ -108,6 +108,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) { @@ -273,6 +285,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 68719ec..1fa2d43 100644 --- a/common/flatpak-run.c +++ b/common/flatpak-run.c @@ -1140,15 +1140,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"); - } - /* 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)) @@ -3376,6 +3367,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; @@ -3403,6 +3396,12 @@ flatpak_run_app (const char *app_ref, if (flags & FLATPAK_RUN_FLAG_DO_NOT_REAP) spawn_flags |= G_SPAWN_DO_NOT_REAP_CHILD; + /* 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, @@ -3427,6 +3426,13 @@ flatpak_run_app (const char *app_ref, /* Ensure we unset O_CLOEXEC */ flatpak_bwrap_child_setup_cb (bwrap->fds); + + /* 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),