Skip to content

Commit 6d1773d

Browse files
smcvalexlarsson
authored andcommitted
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: GHSA-4ppf-fxf6-vxg2
1 parent fe95ef6 commit 6d1773d

File tree

3 files changed

+60
-18
lines changed

3 files changed

+60
-18
lines changed

Diff for: common/flatpak-bwrap-private.h

+3
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ void flatpak_bwrap_unset_env (FlatpakBwrap *bwrap,
4343
const char *variable);
4444
void flatpak_bwrap_add_arg (FlatpakBwrap *bwrap,
4545
const char *arg);
46+
void flatpak_bwrap_take_arg (FlatpakBwrap *bwrap,
47+
char *arg);
4648
void flatpak_bwrap_add_noinherit_fd (FlatpakBwrap *bwrap,
4749
int fd);
4850
void flatpak_bwrap_add_fd (FlatpakBwrap *bwrap,
@@ -73,6 +75,7 @@ void flatpak_bwrap_add_bind_arg (FlatpakBwrap *bwrap,
7375
const char *type,
7476
const char *src,
7577
const char *dest);
78+
void flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap);
7679
gboolean flatpak_bwrap_bundle_args (FlatpakBwrap *bwrap,
7780
int start,
7881
int end,

Diff for: common/flatpak-bwrap.c

+43
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,18 @@ flatpak_bwrap_add_arg (FlatpakBwrap *bwrap, const char *arg)
109109
g_ptr_array_add (bwrap->argv, g_strdup (arg));
110110
}
111111

112+
/*
113+
* flatpak_bwrap_take_arg:
114+
* @arg: (transfer full): Take ownership of this argument
115+
*
116+
* Add @arg to @bwrap's argv, taking ownership of the pointer.
117+
*/
118+
void
119+
flatpak_bwrap_take_arg (FlatpakBwrap *bwrap, char *arg)
120+
{
121+
g_ptr_array_add (bwrap->argv, arg);
122+
}
123+
112124
void
113125
flatpak_bwrap_finish (FlatpakBwrap *bwrap)
114126
{
@@ -274,6 +286,37 @@ flatpak_bwrap_add_bind_arg (FlatpakBwrap *bwrap,
274286
}
275287
}
276288

289+
/*
290+
* Convert bwrap->envp into a series of --setenv arguments for bwrap(1),
291+
* assumed to be applied to an empty environment. Reset envp to be an
292+
* empty environment.
293+
*/
294+
void
295+
flatpak_bwrap_envp_to_args (FlatpakBwrap *bwrap)
296+
{
297+
gsize i;
298+
299+
for (i = 0; bwrap->envp[i] != NULL; i++)
300+
{
301+
char *key_val = bwrap->envp[i];
302+
char *eq = strchr (key_val, '=');
303+
304+
if (eq)
305+
{
306+
flatpak_bwrap_add_arg (bwrap, "--setenv");
307+
flatpak_bwrap_take_arg (bwrap, g_strndup (key_val, eq - key_val));
308+
flatpak_bwrap_add_arg (bwrap, eq + 1);
309+
}
310+
else
311+
{
312+
g_warn_if_reached ();
313+
}
314+
}
315+
316+
g_strfreev (g_steal_pointer (&bwrap->envp));
317+
bwrap->envp = g_strdupv (flatpak_bwrap_empty_env);
318+
}
319+
277320
gboolean
278321
flatpak_bwrap_bundle_args (FlatpakBwrap *bwrap,
279322
int start,

Diff for: common/flatpak-run.c

+14-18
Original file line numberDiff line numberDiff line change
@@ -1463,24 +1463,6 @@ flatpak_run_add_environment_args (FlatpakBwrap *bwrap,
14631463
flatpak_run_add_system_dbus_args (bwrap, proxy_arg_bwrap, context, flags);
14641464
flatpak_run_add_a11y_dbus_args (bwrap, proxy_arg_bwrap, context, flags);
14651465

1466-
if (g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH") != NULL)
1467-
{
1468-
/* LD_LIBRARY_PATH is overridden for setuid helper, so pass it as cmdline arg */
1469-
flatpak_bwrap_add_args (bwrap,
1470-
"--setenv", "LD_LIBRARY_PATH", g_environ_getenv (bwrap->envp, "LD_LIBRARY_PATH"),
1471-
NULL);
1472-
flatpak_bwrap_unset_env (bwrap, "LD_LIBRARY_PATH");
1473-
}
1474-
1475-
if (g_environ_getenv (bwrap->envp, "TMPDIR") != NULL)
1476-
{
1477-
/* TMPDIR is overridden for setuid helper, so pass it as cmdline arg */
1478-
flatpak_bwrap_add_args (bwrap,
1479-
"--setenv", "TMPDIR", g_environ_getenv (bwrap->envp, "TMPDIR"),
1480-
NULL);
1481-
flatpak_bwrap_unset_env (bwrap, "TMPDIR");
1482-
}
1483-
14841466
/* Must run this before spawning the dbus proxy, to ensure it
14851467
ends up in the app cgroup */
14861468
if (!flatpak_run_in_transient_unit (app_id, &my_error))
@@ -4068,6 +4050,8 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
40684050
command = default_command;
40694051
}
40704052

4053+
flatpak_bwrap_envp_to_args (bwrap);
4054+
40714055
if (!flatpak_bwrap_bundle_args (bwrap, 1, -1, FALSE, error))
40724056
return FALSE;
40734057

@@ -4098,6 +4082,12 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
40984082
/* We use LEAVE_DESCRIPTORS_OPEN to work around dead-lock, see flatpak_close_fds_workaround */
40994083
spawn_flags |= G_SPAWN_LEAVE_DESCRIPTORS_OPEN;
41004084

4085+
/* flatpak_bwrap_envp_to_args() moved the environment variables to
4086+
* be set into --setenv instructions in argv, so the environment
4087+
* in which the bwrap command runs must be empty. */
4088+
g_assert (bwrap->envp != NULL);
4089+
g_assert (bwrap->envp[0] == NULL);
4090+
41014091
if (!g_spawn_async (NULL,
41024092
(char **) bwrap->argv->pdata,
41034093
bwrap->envp,
@@ -4125,6 +4115,12 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
41254115
* we do want to allow inheriting fds into flatpak run. */
41264116
flatpak_bwrap_child_setup (bwrap->fds, FALSE);
41274117

4118+
/* flatpak_bwrap_envp_to_args() moved the environment variables to
4119+
* be set into --setenv instructions in argv, so the environment
4120+
* in which the bwrap command runs must be empty. */
4121+
g_assert (bwrap->envp != NULL);
4122+
g_assert (bwrap->envp[0] == NULL);
4123+
41284124
if (execvpe (flatpak_get_bwrap (), (char **) bwrap->argv->pdata, bwrap->envp) == -1)
41294125
{
41304126
g_set_error_literal (error, G_IO_ERROR, g_io_error_from_errno (errno),

0 commit comments

Comments
 (0)