diff --git a/SOURCES/flatpak-1.6.2-fix-CVE-2021-21261.patch b/SOURCES/flatpak-1.6.2-fix-CVE-2021-21261.patch new file mode 100644 index 0000000..fa89ea0 --- /dev/null +++ b/SOURCES/flatpak-1.6.2-fix-CVE-2021-21261.patch @@ -0,0 +1,983 @@ +From 038655c9ee29ee659e5fede4d36871cf8ccef803 Mon Sep 17 00:00:00 2001 +From: Simon McVittie +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 +--- + 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 ++#include ++ ++#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 +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 +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 +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 +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 +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 +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; + + + ++ ++ ++ ++ ++ Read environment variables from the file descriptor ++ FD, and set them as if ++ via . This can be used to avoid ++ environment variables and their values becoming visible ++ to other users. ++ ++ Each environment variable is in the form ++ VAR=VALUE ++ followed by a zero byte. This is the same format used by ++ env -0 and ++ /proc/*/environ. ++ ++ ++ + + + +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; + + + ++ ++ ++ ++ ++ Read environment variables from the file descriptor ++ FD, and set them as if ++ via . This can be used to avoid ++ environment variables and their values becoming visible ++ to other users. ++ ++ Each environment variable is in the form ++ VAR=VALUE ++ followed by a zero byte. This is the same format used by ++ env -0 and ++ /proc/*/environ. ++ ++ ++ + + + +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; + + + ++ ++ ++ ++ ++ Read environment variables from the file descriptor ++ FD, and set them as if ++ via . This can be used to avoid ++ environment variables and their values becoming visible ++ to other users. ++ ++ Each environment variable is in the form ++ VAR=VALUE ++ followed by a zero byte. This is the same format used by ++ env -0 and ++ /proc/*/environ. ++ ++ ++ + + + +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; + + + ++ ++ ++ ++ ++ Read environment variables from the file descriptor ++ FD, and set them as if ++ via . This can be used to avoid ++ environment variables and their values becoming visible ++ to other users. ++ ++ Each environment variable is in the form ++ VAR=VALUE ++ followed by a zero byte. This is the same format used by ++ env -0 and ++ /proc/*/environ. ++ ++ ++ + + + +-- +2.29.2 + + +From 6c244791c912fe0c5ae2e140f251dc02c46cc0aa Mon Sep 17 00:00:00 2001 +From: Simon McVittie +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 +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 +Date: Mon, 11 Jan 2021 12:25:50 +0000 +Subject: [PATCH 06/10] tests: Exercise --env-fd + +Signed-off-by: Simon McVittie +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 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 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 +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 +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 03ea58cc..fe8a514f 100644 +--- a/portal/flatpak-portal.c ++++ b/portal/flatpak-portal.c +@@ -760,6 +760,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++) + { +@@ -767,7 +774,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")); +-- +2.29.2 + + +From 1fb13b40cea72ded0ca804a990e84b12454a30a1 Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Mon, 11 Jan 2021 12:48:01 +0000 +Subject: [PATCH 08/10] 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 +--- + 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 15f52148..c95ed3bc 100644 +--- a/tests/Makefile.am.inc ++++ b/tests/Makefile.am.inc +@@ -156,6 +156,16 @@ dist_installed_test_data = \ + tests/org.flatpak.Authenticator.test.service.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 00000000..a640a945 +--- /dev/null ++++ b/tests/libpreload.c +@@ -0,0 +1,31 @@ ++/* ++ * Copyright 2021 Collabora Ltd. ++ * SPDX-License-Identifier: LGPL-2-or-later ++ */ ++ ++#include ++#include ++#include ++#include ++#include ++ ++__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 1c7dafd3..47416a6d 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 + + skip_revokefs_without_fuse + +@@ -118,6 +123,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 \ +@@ -136,6 +142,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 +Date: Mon, 18 Jan 2021 17:52:13 +0000 +Subject: [PATCH 09/10] 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" +Resolves: https://github.com/flatpak/flatpak/issues/4080 +Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 +Signed-off-by: Simon McVittie +--- + 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 5ecd2ef3..8616f3c8 100644 +--- a/app/flatpak-builtins-build.c ++++ b/app/flatpak-builtins-build.c +@@ -569,6 +569,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; + +-- +2.29.2 + + +From f830b97e81a626a43b160ccb5dad4fe934ab03fa Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Mon, 18 Jan 2021 18:07:38 +0000 +Subject: [PATCH 10/10] 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" +Signed-off-by: Simon McVittie +--- + common/flatpak-dir.c | 2 ++ + 1 file changed, 2 insertions(+) + +diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c +index c3ab8bcf..030b8a31 100644 +--- a/common/flatpak-dir.c ++++ b/common/flatpak-dir.c +@@ -7751,6 +7751,8 @@ apply_extra_data (FlatpakDir *self, + app_context, NULL, 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); +-- +2.29.2 + diff --git a/SPECS/flatpak.spec b/SPECS/flatpak.spec index 585b2d4..f3881b9 100644 --- a/SPECS/flatpak.spec +++ b/SPECS/flatpak.spec @@ -3,7 +3,7 @@ Name: flatpak Version: 1.6.2 -Release: 4%{?dist} +Release: 5%{?dist} Summary: Application deployment framework for desktop apps License: LGPLv2+ @@ -16,6 +16,8 @@ Patch1: flatpak-1.6.2-oci-fixes2.patch # https://bugzilla.redhat.com/show_bug.cgi?id=1878231 # https://github.com/flatpak/flatpak/pull/3845 Patch2: 3845.patch +# https://bugzilla.redhat.com/show_bug.cgi?id=1918774 +Patch3: flatpak-1.6.2-fix-CVE-2021-21261.patch BuildRequires: pkgconfig(appstream-glib) BuildRequires: pkgconfig(dconf) @@ -244,6 +246,9 @@ fi %changelog +* Tue Jan 26 2021 David King - 1.6.2-5 +- Fix CVE-2021-21261 (#1918774) + * Mon Sep 14 2020 Kalev Lember - 1.6.2-4 - OCI: extract appstream data for runtimes (#1878231)