Origin: https://github.com/flatpak/flatpak/commit/8a18137d7e80f0575e8defabf677d81e5cc3a788 https://github.com/flatpak/flatpak/commit/db3a785241fda63bf53f0ec12bb519aa5210de19 https://github.com/flatpak/flatpak/commit/847dfb88cebbdf8825332730b837489684dfb91e https://github.com/flatpak/flatpak/commit/7c63e53bb2af0aae9097fd2edfd6a9ba9d453e97 From 7c63e53bb2af0aae9097fd2edfd6a9ba9d453e97 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 14 Aug 2024 13:44:30 +0100 Subject: [PATCH] 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 | 109 +++++++++++++++++++++++++++++++++++++-- configure.ac | 3 ++ tests/test-run.sh | 39 ++++++++++++++ 3 files changed, 148 insertions(+), 3 deletions(-) diff --git a/common/flatpak-context.c b/common/flatpak-context.c index 6303c71..09dd440 100644 --- a/common/flatpak-context.c +++ b/common/flatpak-context.c @@ -2616,6 +2616,90 @@ flatpak_context_get_run_flags (FlatpakContext *context) return flags; } +/* 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, @@ -2643,12 +2727,31 @@ 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_mkdir_with_parents (src, 0755); + 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_autofree char *src_via_proc = g_strdup_printf ("%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-fd", src_via_proc, dest); } } diff --git a/configure.ac b/configure.ac index 8bf37b0..0862ae5 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) diff --git a/tests/test-run.sh b/tests/test-run.sh index 3c344df..f087ff2 100644 --- a/tests/test-run.sh +++ b/tests/test-run.sh @@ -494,3 +494,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.33.0