From e2c4ded323161f47609d0af97d00019d04a635f1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 11 Jan 2021 16:40:43 +0000 Subject: [PATCH] portal: Let --env= and --env-fd= take precedence over extra-args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, if you launched a subsandbox while specifying environment variable overrides, any environment variable overrides that existed in the parent Flatpak app would take precedence: host$ flatpak run --env=FOO=1 --command=bash example.app [📦 example.app ~]$ env | grep FOO FOO=1 [📦 example.app ~]$ flatpak-spawn --env=FOO=x sh -c 'env | grep FOO' FOO=1 This does not seem like least-astonishment, and in particular will cause problems if the app wants to override LD_LIBRARY_PATH in the subsandbox. Change the precedence so that the environment variables set by flatpak-spawn will "win": host$ flatpak run --env=FOO1=1 --env=FOO2=2 --command=bash example.app [📦 example.app ~]$ env | grep FOO FOO1=1 FOO2=2 [📦 example.app ~]$ flatpak-spawn --env=FOO1=x sh -c 'env | grep FOO' FOO1=x FOO2=2 This follows up from GHSA-4ppf-fxf6-vxg2 to fix an issue that I noticed while resolving that vulnerability, but is not required for fixing the vulnerability. Signed-off-by: Simon McVittie --- portal/flatpak-portal.c | 72 ++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/portal/flatpak-portal.c b/portal/flatpak-portal.c index 41dbbfbb..61bb9dac 100644 --- a/portal/flatpak-portal.c +++ b/portal/flatpak-portal.c @@ -1003,42 +1003,6 @@ 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++) - { - const char *var = NULL; - const char *val = NULL; - g_variant_get_child (arg_envs, i, "{&s&s}", &var, &val); - - 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")); g_ptr_array_add (flatpak_argv, g_strdup ("run")); @@ -1108,6 +1072,42 @@ handle_spawn (PortalFlatpak *object, } } + /* 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++) + { + const char *var = NULL; + const char *val = NULL; + g_variant_get_child (arg_envs, i, "{&s&s}", &var, &val); + + 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'); + } + if (env_string->len > 0) { g_auto(GLnxTmpfile) env_tmpf = { 0, };