Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-15-SP2:GA
flatpak.28335
CVE-2021-21261.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File CVE-2021-21261.patch of Package flatpak.28335
From: Simon McVittie <smcv@collabora.com> 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 <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 | 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), From: Simon McVittie <smcv@collabora.com> Date: Tue, 12 Jan 2021 12:32:58 +0000 Subject: tests: Add minimal version of "ok" helper This makes it possible to cherry-pick improved test coverage from newer branches without having to edit it. Signed-off-by: Simon McVittie <smcv@collabora.com> --- tests/libtest.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/libtest.sh b/tests/libtest.sh index 4a5e699..87e8944 100644 --- a/tests/libtest.sh +++ b/tests/libtest.sh @@ -34,6 +34,10 @@ assert_not_reached () { echo $@ 1>&2; exit 1 } +ok () { + echo "ok $@" +} + test_tmpdir=$(pwd) # Sanity check that we're in a tmpdir that has From: Simon McVittie <smcv@collabora.com> Date: Mon, 11 Jan 2021 12:14:48 +0000 Subject: 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 2a0aab2..2192289 100755 --- a/tests/test-override.sh +++ b/tests/test-override.sh @@ -10,7 +10,7 @@ reset_overrides () { assert_file_empty info } -echo "1..13" +echo "1..15" setup_repo install_repo @@ -63,14 +63,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 From: Simon McVittie <smcv@collabora.com> Date: Mon, 11 Jan 2021 14:51:36 +0000 Subject: common: Move flatpak_buffer_to_sealed_memfd_or_tmpfile to its own file We'll need this to use it in flatpak-portal without pulling the rest of the common/ directory. [Part of a 1.2.x backport of GHSA-4ppf-fxf6-vxg2 for Debian 10.] Signed-off-by: Simon McVittie <smcv@collabora.com> --- common/Makefile.am.inc | 2 + common/flatpak-utils-memfd-private.h | 32 +++++++++++++ common/flatpak-utils-memfd.c | 90 ++++++++++++++++++++++++++++++++++++ common/flatpak-utils-private.h | 1 + common/flatpak-utils.c | 50 -------------------- 5 files changed, 125 insertions(+), 50 deletions(-) create mode 100644 common/flatpak-utils-memfd-private.h create mode 100644 common/flatpak-utils-memfd.c diff --git a/common/Makefile.am.inc b/common/Makefile.am.inc index 98d51b5..b5a5c5e 100644 --- a/common/Makefile.am.inc +++ b/common/Makefile.am.inc @@ -99,6 +99,8 @@ libflatpak_common_la_SOURCES = \ common/flatpak-utils.c \ common/flatpak-utils-http.c \ common/flatpak-utils-http-private.h \ + common/flatpak-utils-memfd.c \ + common/flatpak-utils-memfd-private.h \ common/flatpak-utils-private.h \ common/flatpak-chain-input-stream.c \ common/flatpak-chain-input-stream-private.h \ diff --git a/common/flatpak-utils-memfd-private.h b/common/flatpak-utils-memfd-private.h new file mode 100644 index 0000000..c0e985f --- /dev/null +++ b/common/flatpak-utils-memfd-private.h @@ -0,0 +1,32 @@ +/* + * Copyright © 2014 Red Hat, Inc + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see <http://www.gnu.org/licenses/>. + * + * Authors: + * Alexander Larsson <alexl@redhat.com> + */ + +#ifndef __FLATPAK_UTILS_MEMFD_H__ +#define __FLATPAK_UTILS_MEMFD_H__ + +#include "libglnx/libglnx.h" + +gboolean flatpak_buffer_to_sealed_memfd_or_tmpfile (GLnxTmpfile *tmpf, + const char *name, + const char *str, + size_t len, + GError **error); + +#endif /* __FLATPAK_UTILS_MEMFD_H__ */ diff --git a/common/flatpak-utils-memfd.c b/common/flatpak-utils-memfd.c new file mode 100644 index 0000000..9a0730f --- /dev/null +++ b/common/flatpak-utils-memfd.c @@ -0,0 +1,90 @@ +/* + * Copyright © 2014 Red Hat, Inc + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see <http://www.gnu.org/licenses/>. + * + * Authors: + * Alexander Larsson <alexl@redhat.com> + */ + +#include "config.h" + +#include "flatpak-utils-memfd-private.h" + +#include "valgrind-private.h" + +#include <string.h> +#include <stdlib.h> +#include <stdio.h> +#include <errno.h> +#include <unistd.h> +#include <fcntl.h> +#include <string.h> +#include <sys/stat.h> +#include <sys/file.h> +#include <sys/mman.h> +#include <sys/types.h> +#include <sys/utsname.h> +#include <sys/ioctl.h> +#include <termios.h> + +/* If memfd_create() is available, generate a sealed memfd with contents of + * @str. Otherwise use an O_TMPFILE @tmpf in anonymous mode, write @str to + * @tmpf, and lseek() back to the start. See also similar uses in e.g. + * rpm-ostree for running dracut. + */ +gboolean +flatpak_buffer_to_sealed_memfd_or_tmpfile (GLnxTmpfile *tmpf, + const char *name, + const char *str, + size_t len, + GError **error) +{ + if (len == -1) + len = strlen (str); + glnx_autofd int memfd = memfd_create (name, MFD_CLOEXEC | MFD_ALLOW_SEALING); + int fd; /* Unowned */ + if (memfd != -1) + { + fd = memfd; + } + else + { + /* We use an anonymous fd (i.e. O_EXCL) since we don't want + * the target container to potentially be able to re-link it. + */ + if (!G_IN_SET (errno, ENOSYS, EOPNOTSUPP)) + return glnx_throw_errno_prefix (error, "memfd_create"); + if (!glnx_open_anonymous_tmpfile (O_RDWR | O_CLOEXEC, tmpf, error)) + return FALSE; + fd = tmpf->fd; + } + if (ftruncate (fd, len) < 0) + return glnx_throw_errno_prefix (error, "ftruncate"); + if (glnx_loop_write (fd, str, len) < 0) + return glnx_throw_errno_prefix (error, "write"); + if (lseek (fd, 0, SEEK_SET) < 0) + return glnx_throw_errno_prefix (error, "lseek"); + if (memfd != -1) + { + /* Valgrind doesn't currently handle G_ADD_SEALS, so lets not seal when debugging... */ + if ((!RUNNING_ON_VALGRIND) && + fcntl (memfd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL) < 0) + return glnx_throw_errno_prefix (error, "fcntl(F_ADD_SEALS)"); + /* The other values can stay default */ + tmpf->fd = glnx_steal_fd (&memfd); + tmpf->initialized = TRUE; + } + return TRUE; +} diff --git a/common/flatpak-utils-private.h b/common/flatpak-utils-private.h index 6d6d467..fb970c8 100644 --- a/common/flatpak-utils-private.h +++ b/common/flatpak-utils-private.h @@ -32,6 +32,7 @@ #include "flatpak-context-private.h" #include "flatpak-error.h" #include "flatpak-utils-http-private.h" +#include "flatpak-utils-memfd-private.h" #include <ostree.h> #include <json-glib/json-glib.h> diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c index 4af7586..c6604b0 100644 --- a/common/flatpak-utils.c +++ b/common/flatpak-utils.c @@ -2281,56 +2281,6 @@ flatpak_file_rename (GFile *from, return TRUE; } -/* If memfd_create() is available, generate a sealed memfd with contents of - * @str. Otherwise use an O_TMPFILE @tmpf in anonymous mode, write @str to - * @tmpf, and lseek() back to the start. See also similar uses in e.g. - * rpm-ostree for running dracut. - */ -gboolean -flatpak_buffer_to_sealed_memfd_or_tmpfile (GLnxTmpfile *tmpf, - const char *name, - const char *str, - size_t len, - GError **error) -{ - if (len == -1) - len = strlen (str); - glnx_autofd int memfd = memfd_create (name, MFD_CLOEXEC | MFD_ALLOW_SEALING); - int fd; /* Unowned */ - if (memfd != -1) - { - fd = memfd; - } - else - { - /* We use an anonymous fd (i.e. O_EXCL) since we don't want - * the target container to potentially be able to re-link it. - */ - if (!G_IN_SET (errno, ENOSYS, EOPNOTSUPP)) - return glnx_throw_errno_prefix (error, "memfd_create"); - if (!glnx_open_anonymous_tmpfile (O_RDWR | O_CLOEXEC, tmpf, error)) - return FALSE; - fd = tmpf->fd; - } - if (ftruncate (fd, len) < 0) - return glnx_throw_errno_prefix (error, "ftruncate"); - if (glnx_loop_write (fd, str, len) < 0) - return glnx_throw_errno_prefix (error, "write"); - if (lseek (fd, 0, SEEK_SET) < 0) - return glnx_throw_errno_prefix (error, "lseek"); - if (memfd != -1) - { - /* Valgrind doesn't currently handle G_ADD_SEALS, so lets not seal when debugging... */ - if ((!RUNNING_ON_VALGRIND) && - fcntl (memfd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL) < 0) - return glnx_throw_errno_prefix (error, "fcntl(F_ADD_SEALS)"); - /* The other values can stay default */ - tmpf->fd = glnx_steal_fd (&memfd); - tmpf->initialized = TRUE; - } - return TRUE; -} - gboolean flatpak_open_in_tmpdir_at (int tmpdir_fd, int mode, From: Simon McVittie <smcv@collabora.com> Date: Sun, 10 Jan 2021 16:18:58 +0000 Subject: 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 6e4d564..36fb177 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, @@ -1206,6 +1265,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") }, { "system-own-name", 0, G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_CALLBACK, &option_system_own_name_cb, N_("Allow app to own name on the system bus"), N_("DBUS_NAME") }, diff --git a/doc/flatpak-build-finish.xml b/doc/flatpak-build-finish.xml index 0754335..4d78a84 100644 --- a/doc/flatpak-build-finish.xml +++ b/doc/flatpak-build-finish.xml @@ -281,6 +281,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 d8ec784..6c3fe16 100644 --- a/doc/flatpak-build.xml +++ b/doc/flatpak-build.xml @@ -283,6 +283,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 6e28c76..6e20960 100644 --- a/doc/flatpak-override.xml +++ b/doc/flatpak-override.xml @@ -257,6 +257,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 e50895d..3f52c99 100644 --- a/doc/flatpak-run.xml +++ b/doc/flatpak-run.xml @@ -391,6 +391,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> From: Simon McVittie <smcv@collabora.com> Date: Tue, 12 Jan 2021 12:25:59 +0000 Subject: 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. [Backported to 1.2.x for Debian 10 security update] Signed-off-by: Simon McVittie <smcv@collabora.com> Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2 --- portal/Makefile.am.inc | 4 ++- portal/flatpak-portal.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/portal/Makefile.am.inc b/portal/Makefile.am.inc index 119c1cf..ef1f5da 100644 --- a/portal/Makefile.am.inc +++ b/portal/Makefile.am.inc @@ -29,11 +29,13 @@ flatpak_portal_SOURCES = \ portal/flatpak-portal-app-info.h \ common/flatpak-portal-error.c \ common/flatpak-portal-error.h \ + common/flatpak-utils-memfd.c \ + common/flatpak-utils-memfd-private.h \ $(NULL) BUILT_SOURCES += $(nodist_flatpak_portal_SOURCES) CLEANFILES += $(nodist_flatpak_portal_SOURCES) -flatpak_portal_LDADD = $(AM_LDADD) $(BASE_LIBS) +flatpak_portal_LDADD = $(AM_LDADD) $(BASE_LIBS) libglnx.la flatpak_portal_CFLAGS = $(AM_CFLAGS) $(BASE_CFLAGS) -DFLATPAK_COMPILATION flatpak_portal_CPPFLAGS = $(AM_CPPFLAGS) -I$(builddir)/portal diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c index 5c4f2fe..b758ad4 100644 --- a/portal/flatpak-portal.c +++ b/portal/flatpak-portal.c @@ -32,6 +32,14 @@ #include "flatpak-portal.h" #include "flatpak-portal-app-info.h" #include "flatpak-portal-error.h" +#include "flatpak-utils-memfd-private.h" + +/* Syntactic sugar added in newer GLib, which makes the error paths more + * clearly correct */ +#ifndef G_DBUS_METHOD_INVOCATION_HANDLED +# define G_DBUS_METHOD_INVOCATION_HANDLED TRUE +# define G_DBUS_METHOD_INVOCATION_UNHANDLED FALSE +#endif #define IDLE_TIMEOUT_SECS 10 * 60 @@ -167,8 +175,15 @@ typedef struct int fd_map_len; gboolean set_tty; int tty; + int env_fd; } ChildSetupData; +static void +drop_cloexec (int fd) +{ + fcntl (fd, F_SETFD, 0); +} + static void child_setup_func (gpointer user_data) { @@ -177,6 +192,9 @@ child_setup_func (gpointer user_data) sigset_t set; int i; + if (data->env_fd != -1) + drop_cloexec (data->env_fd); + /* Unblock all signals */ sigemptyset (&set); if (pthread_sigmask (SIG_SETMASK, &set, NULL) == -1) @@ -323,6 +341,9 @@ handle_spawn (PortalFlatpak *object, g_auto(GStrv) sandbox_expose_ro = NULL; gboolean sandboxed; gboolean devel; + g_autoptr(GString) env_string = g_string_new (""); + + child_setup_data.env_fd = -1; app_info = g_object_get_data (G_OBJECT (invocation), "app-info"); g_assert (app_info != NULL); @@ -525,7 +546,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)); } if (devel) From: Simon McVittie <smcv@collabora.com> Date: Mon, 11 Jan 2021 12:25:50 +0000 Subject: 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 2192289..a86f518 100755 --- a/tests/test-override.sh +++ b/tests/test-override.sh @@ -63,14 +63,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<env.3 4<env.4 ${FLATPAK} override --user --show org.test.Hello > override assert_file_has_content override "^\[Environment\]$" @@ -116,11 +118,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=$' From: Simon McVittie <smcv@collabora.com> Date: Sun, 10 Jan 2021 16:25:29 +0000 Subject: 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 --- portal/flatpak-portal.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c index b758ad4..dee4ba3 100644 --- a/portal/flatpak-portal.c +++ b/portal/flatpak-portal.c @@ -526,6 +526,13 @@ handle_spawn (PortalFlatpak *object, else env = g_get_environ (); + /* Let the environment variables given by the caller override the ones + * from extra_args. Don't add them to @env, because they are controlled + * by our caller, which might be trying to use them to inject code into + * flatpak(1); add them to the environment block instead. + * + * We don't use --env= here, so that if the values are something that + * should not be exposed to other uids, they can remain confidential. */ n_envs = g_variant_n_children (arg_envs); for (i = 0; i < n_envs; i++) { @@ -533,7 +540,26 @@ handle_spawn (PortalFlatpak *object, const char *val = NULL; g_variant_get_child (arg_envs, i, "{&s&s}", &var, &val); - env = g_environ_setenv (env, var, val, TRUE); + if (var[0] == '\0') + { + g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, + G_DBUS_ERROR_INVALID_ARGS, + "Environment variable cannot have empty name"); + return G_DBUS_METHOD_INVOCATION_HANDLED; + } + + if (strchr (var, '=') != NULL) + { + g_dbus_method_invocation_return_error (invocation, G_DBUS_ERROR, + G_DBUS_ERROR_INVALID_ARGS, + "Environment variable name cannot contain '='"); + return G_DBUS_METHOD_INVOCATION_HANDLED; + } + + g_string_append (env_string, var); + g_string_append_c (env_string, '='); + g_string_append (env_string, val); + g_string_append_c (env_string, '\0'); } g_ptr_array_add (flatpak_argv, g_strdup ("flatpak")); From: Simon McVittie <smcv@collabora.com> Date: Mon, 11 Jan 2021 12:48:01 +0000 Subject: tests: Assert that --env= does not go in `flatpak run` or bwrap environ For the portal's use of --env-fd= to be safe, we want the environment variables that it sets to end up in the environment for the program that is run by `bwrap` as process 2, but they must not go into the environment that gets used to run `flatpak run` or `bwrap`. Assert that this is the case. For completeness, we're testing both --env= and --env-fd= here, even though the earlier commit "portal: Do not use caller-supplied variables in environment" always uses --env-fd=. Part-of: https://github.com/flatpak/flatpak/security/advisories/GHSA-4ppf-fxf6-vxg2 Signed-off-by: Simon McVittie <smcv@collabora.com> --- tests/Makefile.am.inc | 10 ++++++++++ tests/libpreload.c | 31 +++++++++++++++++++++++++++++++ tests/test-override.sh | 18 ++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 tests/libpreload.c diff --git a/tests/Makefile.am.inc b/tests/Makefile.am.inc index f473051..f799bbf 100644 --- a/tests/Makefile.am.inc +++ b/tests/Makefile.am.inc @@ -101,6 +101,16 @@ dist_installed_test_data = \ tests/session.conf.in \ $(NULL) +test_ltlibraries = tests/libpreload.la + +tests_libpreload_la_SOURCES = tests/libpreload.c +tests_libpreload_la_LDFLAGS = \ + -avoid-version \ + -module \ + -no-undefined \ + -rpath $(installed_testdir) \ + $(NULL) + installed_test_keyringdir = $(installed_testdir)/test-keyring installed_test_keyring2dir = $(installed_testdir)/test-keyring2 diff --git a/tests/libpreload.c b/tests/libpreload.c new file mode 100644 index 0000000..a640a94 --- /dev/null +++ b/tests/libpreload.c @@ -0,0 +1,31 @@ +/* + * Copyright 2021 Collabora Ltd. + * SPDX-License-Identifier: LGPL-2-or-later + */ + +#include <stdio.h> +#include <stdlib.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +__attribute__((constructor)) static void +ctor (void) +{ + pid_t me = getpid (); + struct stat buf; + + fprintf (stderr, "LD_PRELOAD module got loaded by process %d\n", me); + + if (stat ("/.flatpak-info", &buf) == 0) + { + fprintf (stderr, "OK: pid %d is in a Flatpak sandbox\n", me); + } + else + { + /* If the --env=LD_PRELOAD had come from a call to flatpak-portal, + * then this would be a sandbox escape (GHSA-4ppf-fxf6-vxg2). */ + fprintf (stderr, "Error: pid %d is not in a Flatpak sandbox\n", me); + abort (); + } +} diff --git a/tests/test-override.sh b/tests/test-override.sh index a86f518..e87e458 100755 --- a/tests/test-override.sh +++ b/tests/test-override.sh @@ -3,6 +3,11 @@ set -euo pipefail . $(dirname $0)/libtest.sh +if [ -e "${test_builddir}/.libs/libpreload.so" ]; then + install "${test_builddir}/.libs/libpreload.so" "${test_tmpdir}" +else + install "${test_builddir}/libpreload.so" "${test_tmpdir}" +fi reset_overrides () { ${FLATPAK} override --user --reset org.test.Hello @@ -116,6 +121,7 @@ else ${FLATPAK} override --user --show org.test.Hello > override ${FLATPAK} run --command=bash \ + --filesystem="${test_tmpdir}" \ --env=FOO=BAR \ --env=BAR= \ --env-fd=3 \ @@ -134,6 +140,18 @@ else # could see it assert_not_file_has_content out 3047225e-5e38-4357-b21c-eac83b7e8ea6 + # libpreload.so will abort() if it gets loaded into the `flatpak run` + # or `bwrap` processes, so if this succeeds, everything's OK + ${FLATPAK} run --command=bash \ + --filesystem="${test_tmpdir}" \ + --env=LD_PRELOAD="${test_tmpdir}/libpreload.so" \ + org.test.Hello -c '' + printf '%s\0' "LD_PRELOAD=${test_tmpdir}/libpreload.so" > env.ldpreload + ${FLATPAK} run --command=bash \ + --filesystem="${test_tmpdir}" \ + --env-fd=3 \ + org.test.Hello -c '' 3<env.ldpreload + ok "temporary environment variables" fi From: Simon McVittie <smcv@collabora.com> Date: Mon, 18 Jan 2021 17:52:13 +0000 Subject: build: Convert environment into a sequence of bwrap arguments This means we can systematically pass the environment variables through bwrap(1), even if it is setuid and thus is filtering out security-sensitive environment variables. bwrap itself ends up being run with an empty environment instead. This fixes a regression when CVE-2021-21261 was fixed: before the CVE fixes, LD_LIBRARY_PATH would have been passed through like this and appeared in the `flatpak build` shell, but during the CVE fixes, the special case that protected LD_LIBRARY_PATH was removed in favour of the more general flatpak_bwrap_envp_to_args(). That reasoning only works if we use flatpak_bwrap_envp_to_args(), consistently, everywhere that we run the potentially-setuid bwrap. Fixes: 6d1773d2 "run: Convert all environment variables into bwrap arguments" Bug: https://github.com/flatpak/flatpak/issues/4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com> Applied-upstream: 1.10.1, commit:9a61d2c44f0a58cebcb9b2787ae88db07ca68bb0 --- app/flatpak-builtins-build.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/flatpak-builtins-build.c b/app/flatpak-builtins-build.c index 9410791..5ba3ba8 100644 --- a/app/flatpak-builtins-build.c +++ b/app/flatpak-builtins-build.c @@ -566,6 +566,8 @@ flatpak_builtin_build (int argc, char **argv, GCancellable *cancellable, GError NULL); } + flatpak_bwrap_envp_to_args (bwrap); + if (!flatpak_bwrap_bundle_args (bwrap, 1, -1, FALSE, error)) return FALSE; From: Simon McVittie <smcv@collabora.com> Date: Mon, 18 Jan 2021 18:07:38 +0000 Subject: dir: Pass environment via bwrap --setenv when running apply_extra This means we can systematically pass the environment variables through bwrap(1), even if it is setuid and thus is filtering out security-sensitive environment variables. bwrap ends up being run with an empty environment instead. As with the previous commit, this regressed while fixing CVE-2021-21261. Fixes: 6d1773d2 "run: Convert all environment variables into bwrap arguments" Bug: https://github.com/flatpak/flatpak/issues/4080 Signed-off-by: Simon McVittie <smcv@collabora.com> Applied-upstream: 1.10.1, commit:fb473cad801c6b61706353256cab32330557374a --- common/flatpak-dir.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index 40a2e58..f79ebcd 100644 --- a/common/flatpak-dir.c +++ b/common/flatpak-dir.c @@ -6799,6 +6799,8 @@ apply_extra_data (FlatpakDir *self, app_context, NULL, NULL, cancellable, error)) return FALSE; + flatpak_bwrap_envp_to_args (bwrap); + flatpak_bwrap_add_arg (bwrap, "/app/bin/apply_extra"); flatpak_bwrap_finish (bwrap);
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
