From 79f4d79cb93a48c0256b399c69e677170acbd3bd Mon Sep 17 00:00:00 2001 From: Kalev Lember Date: Mon, 2 Sep 2024 23:27:47 +0200 Subject: [PATCH] Backport upstream patches for CVE-2024-32462 - Require bubblewrap version that has new --bind-fd option backported for addressing CVE-2024-32462 Resolves: RHEL-54555 --- flatpak-1.12.x-CVE-2024-42472.patch | 330 ++++++++++++++++++++++++++++ flatpak.spec | 11 +- 2 files changed, 339 insertions(+), 2 deletions(-) create mode 100644 flatpak-1.12.x-CVE-2024-42472.patch diff --git a/flatpak-1.12.x-CVE-2024-42472.patch b/flatpak-1.12.x-CVE-2024-42472.patch new file mode 100644 index 0000000..0fc23a5 --- /dev/null +++ b/flatpak-1.12.x-CVE-2024-42472.patch @@ -0,0 +1,330 @@ +From 8451fa0ae30397b83705a193aa0d3f7752486dda Mon Sep 17 00:00:00 2001 +From: Alexander Larsson +Date: Mon, 3 Jun 2024 12:22:30 +0200 +Subject: [PATCH 1/4] Don't follow symlinks when mounting persisted directories + +These directories are in a location under application control, so we +can't trust them to not be a symlink outside of the files accessibe to +the application. + +Continue to treat --persist=/foo as --persist=foo for backwards compat, +since this is how it (accidentally) worked before, but print a warning. + +Don't allow ".." elements in persist paths: these would not be useful +anyway, and are unlikely to be in use, however they could potentially +be used to confuse the persist path handling. + +This partially addresses CVE-2024-42472. If only one instance of the +malicious or compromised app is run at a time, the vulnerability +is avoided. If two instances can run concurrently, there is a +time-of-check/time-of-use issue remaining, which can only be resolved +with changes to bubblewrap; this will be resolved in a separate commit, +because the bubblewrap dependency might be more difficult to provide in +LTS distributions. + +Helps: CVE-2024-42472, GHSA-7hgv-f2j8-xw87 +[smcv: Make whitespace consistent] +[smcv: Use g_warning() if unable to create --persist paths] +[smcv: Use stat() to detect symlinks and warn about them] +[smcv: Use glnx_steal_fd() for portability to older GLib] +Co-authored-by: Simon McVittie +Signed-off-by: Simon McVittie +--- + common/flatpak-context.c | 108 +++++++++++++++++++++++++++++++++++++-- + 1 file changed, 105 insertions(+), 3 deletions(-) + +diff --git a/common/flatpak-context.c b/common/flatpak-context.c +index 53b79807..8c784acf 100644 +--- a/common/flatpak-context.c ++++ b/common/flatpak-context.c +@@ -2686,6 +2686,90 @@ flatpak_context_get_exports_full (FlatpakContext *context, + return g_steal_pointer (&exports); + } + ++/* This creates zero or more directories unders base_fd+basedir, each ++ * being guaranteed to either exist and be a directory (no symlinks) ++ * or be created as a directory. The last directory is opened ++ * and the fd is returned. ++ */ ++static gboolean ++mkdir_p_open_nofollow_at (int base_fd, ++ const char *basedir, ++ int mode, ++ const char *subdir, ++ int *out_fd, ++ GError **error) ++{ ++ glnx_autofd int parent_fd = -1; ++ ++ if (g_path_is_absolute (subdir)) ++ { ++ const char *skipped_prefix = subdir; ++ ++ while (*skipped_prefix == '/') ++ skipped_prefix++; ++ ++ g_warning ("--persist=\"%s\" is deprecated, treating it as --persist=\"%s\"", subdir, skipped_prefix); ++ subdir = skipped_prefix; ++ } ++ ++ g_autofree char *subdir_dirname = g_path_get_dirname (subdir); ++ ++ if (strcmp (subdir_dirname, ".") == 0) ++ { ++ /* It is ok to open basedir with follow=true */ ++ if (!glnx_opendirat (base_fd, basedir, TRUE, &parent_fd, error)) ++ return FALSE; ++ } ++ else if (strcmp (subdir_dirname, "..") == 0) ++ { ++ return glnx_throw (error, "'..' not supported in --persist paths"); ++ } ++ else ++ { ++ if (!mkdir_p_open_nofollow_at (base_fd, basedir, mode, ++ subdir_dirname, &parent_fd, error)) ++ return FALSE; ++ } ++ ++ g_autofree char *subdir_basename = g_path_get_basename (subdir); ++ ++ if (strcmp (subdir_basename, ".") == 0) ++ { ++ *out_fd = glnx_steal_fd (&parent_fd); ++ return TRUE; ++ } ++ else if (strcmp (subdir_basename, "..") == 0) ++ { ++ return glnx_throw (error, "'..' not supported in --persist paths"); ++ } ++ ++ if (!glnx_shutil_mkdir_p_at (parent_fd, subdir_basename, mode, NULL, error)) ++ return FALSE; ++ ++ int fd = openat (parent_fd, subdir_basename, O_PATH | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_NOCTTY | O_NOFOLLOW); ++ if (fd == -1) ++ { ++ int saved_errno = errno; ++ struct stat stat_buf; ++ ++ /* If it's a symbolic link, that could be a user trying to offload ++ * large data to another filesystem, but it could equally well be ++ * a malicious or compromised app trying to exploit GHSA-7hgv-f2j8-xw87. ++ * Produce a clearer error message in this case. ++ * Unfortunately the errno we get in this case is ENOTDIR, so we have ++ * to ask again to find out whether it's really a symlink. */ ++ if (saved_errno == ENOTDIR && ++ fstatat (parent_fd, subdir_basename, &stat_buf, AT_SYMLINK_NOFOLLOW) == 0 && ++ S_ISLNK (stat_buf.st_mode)) ++ return glnx_throw (error, "Symbolic link \"%s\" not allowed to avoid sandbox escape", subdir_basename); ++ ++ return glnx_throw_errno_prefix (error, "openat(%s)", subdir_basename); ++ } ++ ++ *out_fd = fd; ++ return TRUE; ++} ++ + void + flatpak_context_append_bwrap_filesystem (FlatpakContext *context, + FlatpakBwrap *bwrap, +@@ -2709,12 +2793,30 @@ flatpak_context_append_bwrap_filesystem (FlatpakContext *context, + while (g_hash_table_iter_next (&iter, &key, NULL)) + { + const char *persist = key; +- g_autofree char *src = g_build_filename (g_get_home_dir (), ".var/app", app_id, persist, NULL); ++ g_autofree char *appdir = g_build_filename (g_get_home_dir (), ".var/app", app_id, NULL); + g_autofree char *dest = g_build_filename (g_get_home_dir (), persist, NULL); ++ g_autoptr(GError) local_error = NULL; ++ ++ if (g_mkdir_with_parents (appdir, 0755) != 0) ++ { ++ g_warning ("Unable to create directory %s", appdir); ++ continue; ++ } ++ ++ /* Don't follow symlinks from the persist directory, as it is under user control */ ++ glnx_autofd int src_fd = -1; ++ if (!mkdir_p_open_nofollow_at (AT_FDCWD, appdir, 0755, ++ persist, &src_fd, ++ &local_error)) ++ { ++ g_warning ("Failed to create persist path %s: %s", persist, local_error->message); ++ continue; ++ } + +- g_mkdir_with_parents (src, 0755); ++ g_autofree char *src_via_proc = g_strdup_printf ("/proc/self/fd/%d", src_fd); + +- flatpak_bwrap_add_bind_arg (bwrap, "--bind", src, dest); ++ flatpak_bwrap_add_fd (bwrap, glnx_steal_fd (&src_fd)); ++ flatpak_bwrap_add_bind_arg (bwrap, "--bind", src_via_proc, dest); + } + } + +-- +2.46.0 + + +From 5462c9b1e1a34b1104c8a0843a10382e90c9bb6b Mon Sep 17 00:00:00 2001 +From: Alexander Larsson +Date: Mon, 3 Jun 2024 12:59:05 +0200 +Subject: [PATCH 2/4] Add test coverage for --persist + +This adds three "positive" tests: the common case --persist=.persist, the +deprecated spelling --persist=/.persist, and the less common special case +--persist=. as used by Steam. + +It also adds "negative" tests for CVE-2024-42472: if the --persist +directory is a symbolic link or contains path segment "..", we want that +to be rejected. + +Reproduces: CVE-2024-42472, GHSA-7hgv-f2j8-xw87 +[smcv: Add "positive" tests] +[smcv: Exercise --persist=..] +[smcv: Assert that --persist with a symlink produces expected message] +Co-authored-by: Simon McVittie +Signed-off-by: Simon McVittie +--- + tests/test-run.sh | 41 ++++++++++++++++++++++++++++++++++++++++- + 1 file changed, 40 insertions(+), 1 deletion(-) + +diff --git a/tests/test-run.sh b/tests/test-run.sh +index dd371df3..bca0845d 100644 +--- a/tests/test-run.sh ++++ b/tests/test-run.sh +@@ -24,7 +24,7 @@ set -euo pipefail + skip_without_bwrap + skip_revokefs_without_fuse + +-echo "1..20" ++echo "1..21" + + # Use stable rather than master as the branch so we can test that the run + # command automatically finds the branch correctly +@@ -512,3 +512,42 @@ ${FLATPAK} ${U} info -m org.test.App > out + assert_file_has_content out "^sdk=org\.test\.Sdk/$(flatpak --default-arch)/stable$" + + ok "--sdk option" ++ ++rm -fr "$HOME/.var/app/org.test.Hello" ++mkdir -p "$HOME/.var/app/org.test.Hello" ++run --command=sh --persist=.persist org.test.Hello -c 'echo can-persist > .persist/rc' ++sed -e 's,^,#--persist=.persist# ,g' < "$HOME/.var/app/org.test.Hello/.persist/rc" >&2 ++assert_file_has_content "$HOME/.var/app/org.test.Hello/.persist/rc" "can-persist" ++ ++ok "--persist=.persist persists a directory" ++ ++rm -fr "$HOME/.var/app/org.test.Hello" ++mkdir -p "$HOME/.var/app/org.test.Hello" ++# G_DEBUG= to avoid the deprecation warning being fatal ++G_DEBUG= run --command=sh --persist=/.persist org.test.Hello -c 'echo can-persist > .persist/rc' ++sed -e 's,^,#--persist=/.persist# ,g' < "$HOME/.var/app/org.test.Hello/.persist/rc" >&2 ++assert_file_has_content "$HOME/.var/app/org.test.Hello/.persist/rc" "can-persist" ++ ++ok "--persist=/.persist is a deprecated form of --persist=.persist" ++ ++rm -fr "$HOME/.var/app/org.test.Hello" ++mkdir -p "$HOME/.var/app/org.test.Hello" ++run --command=sh --persist=. org.test.Hello -c 'echo can-persist > .persistrc' ++sed -e 's,^,#--persist=.# ,g' < "$HOME/.var/app/org.test.Hello/.persistrc" >&2 ++assert_file_has_content "$HOME/.var/app/org.test.Hello/.persistrc" "can-persist" ++ ++ok "--persist=. persists all files" ++ ++mkdir "${TEST_DATA_DIR}/inaccessible" ++echo FOO > ${TEST_DATA_DIR}/inaccessible/secret-file ++rm -fr "$HOME/.var/app/org.test.Hello" ++mkdir -p "$HOME/.var/app/org.test.Hello" ++ln -fns "${TEST_DATA_DIR}/inaccessible" "$HOME/.var/app/org.test.Hello/persist" ++# G_DEBUG= to avoid the warnings being fatal when we reject a --persist option. ++# LC_ALL=C so we get the expected non-localized string. ++LC_ALL=C G_DEBUG= run --command=ls --persist=persist --persist=relative/../escape org.test.Hello -la ~/persist &> hello_out || true ++sed -e 's,^,#--persist=symlink# ,g' < hello_out >&2 ++assert_file_has_content hello_out "not allowed to avoid sandbox escape" ++assert_not_file_has_content hello_out "secret-file" ++ ++ok "--persist doesn't allow sandbox escape via a symlink (CVE-2024-42472)" +-- +2.46.0 + + +From 04d8ad3009cd8a4350fba6cf7cc6c7819ccdfd34 Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Mon, 12 Aug 2024 19:48:18 +0100 +Subject: [PATCH 3/4] build: Require a version of bubblewrap with the --bind-fd + option + +We need this for the --bind-fd option, which will close a race +condition in our solution to CVE-2024-42472. + +For this stable branch, check the --help output for a --bind-fd option +instead of requiring a specific version number, to accommodate possible +backports in LTS distributions. + +Signed-off-by: Simon McVittie +--- + configure.ac | 3 +++ + 1 file changed, 3 insertions(+) + +diff --git a/configure.ac b/configure.ac +index 0a44e11a..0c8e2d0e 100644 +--- a/configure.ac ++++ b/configure.ac +@@ -175,6 +175,9 @@ if test "x$BWRAP" != xfalse; then + BWRAP_VERSION=`$BWRAP --version | sed 's,.*\ \([0-9]*\.[0-9]*\.[0-9]*\)$,\1,'` + AX_COMPARE_VERSION([$SYSTEM_BWRAP_REQS],[gt],[$BWRAP_VERSION], + [AC_MSG_ERROR([You need at least version $SYSTEM_BWRAP_REQS of bubblewrap to use the system installed version])]) ++ AS_IF([$BWRAP --help | grep '@<:@-@:>@-bind-fd' >/dev/null], ++ [:], ++ [AC_MSG_ERROR([$BWRAP does not list required option --bind-fd in its --help])]) + AM_CONDITIONAL([WITH_SYSTEM_BWRAP], [true]) + else + AC_CHECK_LIB(cap, cap_from_text, CAP_LIB=-lcap) +-- +2.46.0 + + +From 2772f19e50c0e809dde8cf3c105d90ee8baf4fa8 Mon Sep 17 00:00:00 2001 +From: Simon McVittie +Date: Wed, 14 Aug 2024 13:44:30 +0100 +Subject: [PATCH 4/4] persist directories: Pass using new bwrap --bind-fd + option + +Instead of passing a /proc/self/fd bind mount we use --bind-fd, which +has two advantages: + * bwrap closes the fd when used, so it doesn't leak into the started app + * bwrap ensures that what was mounted was the passed in fd (same dev/ino), + as there is a small (required) gap between symlink resolve and mount + where the target path could be replaced. + +Please note that this change requires an updated version of bubblewrap. + +Resolves: CVE-2024-42472, GHSA-7hgv-f2j8-xw87 +[smcv: Make whitespace consistent] +Co-authored-by: Simon McVittie +Signed-off-by: Simon McVittie +--- + common/flatpak-context.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/common/flatpak-context.c b/common/flatpak-context.c +index 8c784acf..baa62728 100644 +--- a/common/flatpak-context.c ++++ b/common/flatpak-context.c +@@ -2813,10 +2813,10 @@ flatpak_context_append_bwrap_filesystem (FlatpakContext *context, + continue; + } + +- g_autofree char *src_via_proc = g_strdup_printf ("/proc/self/fd/%d", src_fd); ++ g_autofree char *src_via_proc = g_strdup_printf ("%d", src_fd); + + flatpak_bwrap_add_fd (bwrap, glnx_steal_fd (&src_fd)); +- flatpak_bwrap_add_bind_arg (bwrap, "--bind", src_via_proc, dest); ++ flatpak_bwrap_add_bind_arg (bwrap, "--bind-fd", src_via_proc, dest); + } + } + +-- +2.46.0 + diff --git a/flatpak.spec b/flatpak.spec index 0e3f1ab..16d15ff 100644 --- a/flatpak.spec +++ b/flatpak.spec @@ -1,9 +1,9 @@ -%global bubblewrap_version 0.4.0 +%global bubblewrap_version 0.4.1-8 %global ostree_version 2020.8 Name: flatpak Version: 1.12.9 -Release: 1%{?dist} +Release: 2%{?dist} Summary: Application deployment framework for desktop apps License: LGPLv2+ @@ -17,6 +17,8 @@ Source1: flatpak-add-fedora-repos.service # https://bugzilla.redhat.com/show_bug.cgi?id=1935508 Patch0: flatpak-dir-Use-SHA256-not-SHA1-to-name-the-cache-for-a-filt.patch +# Backported upstream patch for CVE-2024-42472 +Patch1: flatpak-1.12.x-CVE-2024-42472.patch BuildRequires: pkgconfig(appstream-glib) BuildRequires: pkgconfig(dconf) @@ -276,6 +278,11 @@ fi %changelog +* Mon Sep 02 2024 Kalev Lember - 1.12.9-2 +- Backport upstream patches for CVE-2024-32462 +- Require bubblewrap version that has new --bind-fd option backported for + addressing CVE-2024-32462 + * Tue Apr 30 2024 Kalev Lember - 1.12.9-1 - Update to 1.12.9 (CVE-2024-32462)