`flatpak build` regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

smcv · 2021-01-18T16:50:53Z

Linux distribution and version

Originally reported for Debian 10, reproduced in Debian unstable (rolling release)

Flatpak version

Originally reported for 1.2.5-0+deb10u2 (basically a271971), reproduced in 1.10.0

Description of the problem

Downstream bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323

If you are using a setuid-root bwrap, then flatpak build does not set LD_LIBRARY_PATH any more. This means that libraries provided by /app/lib or an extension are not found.

(This is not reproducible if you are not using the setuid-root bwrap.)

Steps to reproduce

$ flatpak install org.freedesktop.Sdk//20.08
$ flatpak install org.freedesktop.Platform//20.08
$ flatpak build-init _flatpak-build com.example.App org.freedesktop.Sdk org.freedesktop.Platform 20.08
$ flatpak build _flatpak-build -- sh -c 'echo "$LD_LIBRARY_PATH"'

Expected result

/app/lib:/usr/lib/x86_64-linux-gnu/GL/default/lib:/usr/lib/x86_64-linux-gnu/openh264/extra

Actual result

(no output)

The text was updated successfully, but these errors were encountered:

smcv · 2021-01-18T17:00:53Z

I think this is because:

In 6d1773d and its backports, I systematically converted all environment variable settings into --setenv arguments. This made it seem unnecessary to do that individually for LD_LIBRARY_PATH (bde6fa5) and TMPDIR (b8d2271), so I removed those from flatpak_run_add_environment_args().

However, my new code to convert environment variable setting into --setenv arguments is only reached when we are in flatpak_run_app() (called from flatpak run and flatpak_installation_launch_full()). It is skipped in the two other callers of flatpak_run_add_environment_args(): those are apply_extra_data(), and flatpak build.

I see two possible solutions to this.

One is to reinstate the code that I removed from flatpak_run_add_environment_args().

The other is to call flatpak_bwrap_envp_to_args() every time we are going to exec bwrap, which means we will systematically transform all environment variables into --setenv, in all code paths that exec bwrap.

smcv · 2021-01-18T18:06:33Z

Uses of FlatpakBwrap to run bwrap that might be affected by this:

The single use calling execvpe() in app/flatpak-builtins-build.c is this bug.
apply_extra_data() in common/flatpak-dir.c uses the flatpak_run_get_minimal_env(), and regressed in the same way as flatpak build.
flatpak_dir_run_triggers() in common/flatpak-dir.c currently ignores bwrap->envp and always inherits the parent process's environment. I'm not completely sure what this is used for, and so I'm not sure whether that's a bug or working as intended, but either way it didn't regress when the recent CVE was fixed.
start_dbus_proxy() in common/flatpak-run.c instantiates the flatpak_run_get_minimal_env(), but then ignores it and inherits the parent process's environment - which seems maybe appropriate, because the dbus-proxy is basically running on a slightly adjusted version of the host system's mount namespace, rather than being in the runtime? Again I'm not completely sure whether that's a bug or working as intended, but either way it didn't regress when the recent CVE was fixed.
regenerate_ld_cache() in common/flatpak-run.c uses the flatpak_run_get_minimal_env(), but doesn't make any attempt to avoid LD_LIBRARY_PATH being filtered out by bwrap. Perhaps that's a bug, but it didn't regress when the recent CVE was fixed.
flatpak_run_app() in common/flatpak-run.c already uses flatpak_bwrap_envp_to_args().

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: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: flatpak#4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com>

This code was removed in 6d1773d while fixing CVE-2021-21261, because it seemed redundant with the more general mechanism in flatpak_bwrap_envp_to_args(). However, it was not *completely* redundant, because not all callers of flatpak_run_add_environment_args() were converted to call flatpak_bwrap_envp_to_args() afterwards. This partially reverts commit 6d1773d. Fixes: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: flatpak#4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323

smcv · 2021-01-18T18:33:42Z

I see two possible solutions to this.

One is to reinstate the code that I removed from flatpak_run_add_environment_args().

#4082.

The other is to call flatpak_bwrap_envp_to_args() every time we are going to exec bwrap, which means we will systematically transform all environment variables into --setenv, in all code paths that exec bwrap.

#4081. I think I prefer this one: it seems more like the direction that we ought to be going in.

smcv · 2021-01-18T18:36:07Z

/cc @RyuzakiKK

alexlarsson · 2021-01-21T12:41:48Z

flatpak_dir_run_triggers() is not used to run a flatpak sandbox, just to make /usr read-only while running triggers such as the mimedata, icon and desktop database generation for the "exports" directory. These will be using the host libraries and binaries in the same place as usual, so I don't think those rely on LD_LIBRARY_PATH.

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: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: #4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com>

smcv · 2021-01-21T13:20:56Z

start_dbus_proxy() in common/flatpak-run.c instantiates the flatpak_run_get_minimal_env(), but then ignores it and inherits the parent process's environment - which seems maybe appropriate, because the dbus-proxy is basically running on a slightly adjusted version of the host system's mount namespace, rather than being in the runtime? Again I'm not completely sure whether that's a bug or working as intended, but either way it didn't regress when the recent CVE was fixed.

I think this is maybe working as intended, then, with the same reasoning you had for flatpak_dir_run_triggers() - so we don't want to touch that one, and I was correct to avoid it in #4081?

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: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: flatpak#4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com> (cherry picked from commit 9a61d2c)

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: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: #4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com> (cherry picked from commit 9a61d2c)

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: 6d1773d "run: Convert all environment variables into bwrap arguments" Resolves: flatpak#4080 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=980323 Signed-off-by: Simon McVittie <smcv@collabora.com> (cherry picked from commit 9a61d2c) (cherry picked from commit 93ecea3)

smcv mentioned this issue Jan 18, 2021

More consistently convert bwrap->envp into --setenv arguments #4081

Merged

smcv mentioned this issue Jan 18, 2021

run: Reinstate special case for LD_LIBRARY_PATH, TMPDIR #4082

Closed

smcv self-assigned this Jan 18, 2021

alexlarsson closed this as completed in #4081 Jan 21, 2021

smcv mentioned this issue Jan 21, 2021

Backport #4081 to 1.8.x #4086

Closed

smcv mentioned this issue Jan 21, 2021

Backport #4081 for 1.2.x #4087

Merged

MrSSvard mentioned this issue Jun 6, 2022

[Bug]: LD_LIBRARY_PATH empty flatpak/flatpak-builder#474

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`flatpak build` regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

`flatpak build` regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

smcv commented Jan 18, 2021 •

edited

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

alexlarsson commented Jan 21, 2021

smcv commented Jan 21, 2021

flatpak build regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

flatpak build regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

Comments

smcv commented Jan 18, 2021 • edited

Linux distribution and version

Flatpak version

Description of the problem

Steps to reproduce

Expected result

Actual result

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

smcv commented Jan 18, 2021

alexlarsson commented Jan 21, 2021

smcv commented Jan 21, 2021

`flatpak build` regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

`flatpak build` regression after recent CVE fixes: LD_LIBRARY_PATH not set #4080

smcv commented Jan 18, 2021 •

edited