Origin:
https://github.com/flatpak/flatpak/commit/3caeb16c31a3ed62d744e2aaf01d684f7991051a
https://github.com/flatpak/flatpak/commit/2cdd1e1e5ae90d7c3a4b60ce2e36e4d609e44e72
https://github.com/flatpak/flatpak/commit/6bd603f6836e9b38b9b937d3b78f3fbf36e7ff75

From 6bd603f6836e9b38b9b937d3b78f3fbf36e7ff75 Mon Sep 17 00:00:00 2001
From: Alexander Larsson <[email protected]>
Date: Tue, 18 Jun 2024 11:31:05 +0200
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 <[email protected]>
Signed-off-by: Simon McVittie <[email protected]>


---
 common/flatpak-context.c | 109 +++++++++++++++++++++++++++++++++++++--
 tests/test-run.sh        |  41 ++++++++++++++-
 2 files changed, 145 insertions(+), 5 deletions(-)

diff --git a/common/flatpak-context.c b/common/flatpak-context.c
index 2d98d5c..b4b2108 100644
--- a/common/flatpak-context.c
+++ b/common/flatpak-context.c
@@ -2859,6 +2859,90 @@ flatpak_context_apply_env_appid (FlatpakBwrap *bwrap,
     flatpak_bwrap_set_env (bwrap, "HOST_XDG_STATE_HOME", g_getenv ("XDG_STATE_HOME"), TRUE);
 }
 
+/* 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,
@@ -2882,13 +2966,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;
+            }
 
-          if (g_mkdir_with_parents (src, 0755) != 0)
-            g_info ("Unable to create directory %s", src);
+          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, g_steal_fd (&src_fd));
+          flatpak_bwrap_add_bind_arg (bwrap, "--bind-fd", src_via_proc, dest);
         }
     }
 
diff --git a/tests/test-run.sh b/tests/test-run.sh
index 7138bc6..244b533 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..26"
 
 # Use stable rather than master as the branch so we can test that the run
 # command automatically finds the branch correctly
@@ -543,3 +543,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