extra-data: Don't allow creating files with non-canonical permissions in apply_extra

When installing a flatpak with extra-data we execute the apply_extra
script from the flatpak to extract the extra data files we
created. This script runs with very little filesystem acces, but it
does have write permissions to the location that will eventually be
/app/extra in the finished flatpak. This is especially problematic for
the systemwide install case, because the script is then run as root,
so it could potentially create a setuid file there.

Such a file would not be usable inside the sandbox (because setuid is
disabled in the sandbox), but it could potentially be a problem if the
user could be tricked into running the file directly on the host. This
is the same behaviour as e.g. rpm or deb which both can install setuid
files, but we want to guarantee that flatpak is better than that.

The fix is to run the script with all capabilities dropped (bwrap
--cap-drop ALL) which removes a bunch of possible attack vectors (for
instance setting file capabilities). However, even without
capabilities, it is possible for a user to make any file setuid to the
same user, so we also need to canonicalize the permissions of all
files generated by running the script.

Additionally, while running the script we set the toplevel directory
only be accessible to the user, meaning we will not temporarily leak
any potential setuid files to other users.

Note, this commit actually goes furthen than that and completely
canonicalizes all the file permissions to be the same as those
otherwise used by flatpak. For example we fix up cases where the
script creates files writable or unreadable by non-root users.

Closes: #2323
Approved by: alexlarsson

(cherry picked from commit 35598f46a5ccff3e726c148314aa6577fac2cf82)

3 files changed, 90 insertions, 24 deletions

diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
index f1a36ebc..0809a42b 100644
--- a/common/flatpak-dir.c
+++ b/common/flatpak-dir.c
@@ -6502,6 +6502,8 @@ apply_extra_data (FlatpakDir   *self,
                           "--ro-bind", flatpak_file_get_path_cached (app_files), "/app",
                           "--bind", flatpak_file_get_path_cached (extra_files), "/app/extra",
                           "--chdir", "/app/extra",
+                          /* We run as root in the system-helper case, so drop all caps */
+                          "--cap-drop", "ALL",
                           NULL);
 
   if (!flatpak_run_setup_base_argv (bwrap, runtime_files, NULL, runtime_ref_parts[2],
@@ -6525,6 +6527,17 @@ apply_extra_data (FlatpakDir   *self,
 
   g_debug ("Running /app/bin/apply_extra ");
 
+  /* We run the sandbox without caps, but it can still create files owned by itself with
+   * arbitrary permissions, including setuid myself. This is extra risky in the case where
+   * this runs as root in the system helper case. We canonicalize the permissions at the
+   * end, but do avoid non-canonical permissions leaking out before then we make the
+   * toplevel dir only accessible to the user */
+  if (chmod (flatpak_file_get_path_cached (extra_files), 0700) != 0)
+    {
+      glnx_set_error_from_errno (error);
+      return FALSE;
+    }
+
   if (!g_spawn_sync (NULL,
                      (char **) bwrap->argv->pdata,
                      bwrap->envp,
@@ -6535,6 +6548,9 @@ apply_extra_data (FlatpakDir   *self,
                      error))
     return FALSE;
 
+  if (!flatpak_canonicalize_permissions (AT_FDCWD, flatpak_file_get_path_cached (extra_files), error))
+    return FALSE;
+
   if (exit_status != 0)
     {
       g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
diff --git a/common/flatpak-utils-private.h b/common/flatpak-utils-private.h
index 8fa70eb0..8ed25f5a 100644
--- a/common/flatpak-utils-private.h
+++ b/common/flatpak-utils-private.h
@@ -494,6 +494,10 @@ gboolean flatpak_rm_rf (GFile        *dir,
                         GCancellable *cancellable,
                         GError      **error);
 
+gboolean flatpak_canonicalize_permissions (int           parent_dfd,
+                                           const char   *rel_path,
+                                           GError      **error);
+
 char * flatpak_readlink (const char *path,
                          GError    **error);
 char * flatpak_resolve_link (const char *path,
diff --git a/common/flatpak-utils.c b/common/flatpak-utils.c
index 9e68eec4..55132ddc 100644
--- a/common/flatpak-utils.c
+++ b/common/flatpak-utils.c
@@ -2093,13 +2093,18 @@ out:
   return ret;
 }
 
-gboolean
-flatpak_zero_mtime (int           parent_dfd,
-                    const char   *rel_path,
-                    GCancellable *cancellable,
-                    GError      **error)
+static gboolean
+_flatpak_canonicalize_permissions (int           parent_dfd,
+                                   const char   *rel_path,
+                                   gboolean      toplevel,
+                                   GError      **error)
 {
   struct stat stbuf;
+  gboolean res = TRUE;
+
+  /* Note, in order to not leave non-canonical things around in case
+   * of error, this continues after errors, but returns the first
+   * error. */
 
   if (TEMP_FAILURE_RETRY (fstatat (parent_dfd, rel_path, &stbuf, AT_SYMLINK_NOFOLLOW)) != 0)
     {
@@ -2110,43 +2115,84 @@ flatpak_zero_mtime (int           parent_dfd,
   if (S_ISDIR (stbuf.st_mode))
     {
       g_auto(GLnxDirFdIterator) dfd_iter = { 0, };
-      gboolean inited;
 
-      inited = glnx_dirfd_iterator_init_at (parent_dfd, rel_path, FALSE, &dfd_iter, NULL);
+      /* For the toplevel we set to 0700 so we can modify it, but not
+         expose any non-canonical files to any other user, then we set
+         it to 0755 afterwards. */
+      if (fchmodat (parent_dfd, rel_path, toplevel ? 0700 : 0755, 0) != 0)
+        {
+          glnx_set_error_from_errno (error);
+          error = NULL;
+          res = FALSE;
+        }
 
-      while (inited)
+      if (glnx_dirfd_iterator_init_at (parent_dfd, rel_path, FALSE, &dfd_iter, NULL))
         {
-          struct dirent *dent;
+          while (TRUE)
+            {
+              struct dirent *dent;
 
-          if (!glnx_dirfd_iterator_next_dent (&dfd_iter, &dent, NULL, NULL) || dent == NULL)
-            break;
+              if (!glnx_dirfd_iterator_next_dent (&dfd_iter, &dent, NULL, NULL) || dent == NULL)
+                break;
 
-          if (!flatpak_zero_mtime (dfd_iter.fd, dent->d_name,
-                                   cancellable, error))
-            return FALSE;
+              if (!_flatpak_canonicalize_permissions (dfd_iter.fd, dent->d_name, FALSE, error))
+                {
+                  error = NULL;
+                  res = FALSE;
+                }
+            }
         }
 
-      /* Update stbuf */
-      if (TEMP_FAILURE_RETRY (fstat (dfd_iter.fd, &stbuf)) != 0)
+      if (toplevel &&
+          fchmodat (parent_dfd, rel_path, 0755, 0) != 0)
         {
           glnx_set_error_from_errno (error);
-          return FALSE;
+          error = NULL;
+          res = FALSE;
         }
-    }
 
-  /* OSTree checks out to mtime 0, so we do the same */
-  if (stbuf.st_mtime != OSTREE_TIMESTAMP)
+        return res;
+    }
+  else if (S_ISREG(stbuf.st_mode))
     {
-      const struct timespec times[2] = { { 0, UTIME_OMIT }, { OSTREE_TIMESTAMP, } };
+      mode_t mode;
+
+      /* If use can execute, make executable by all */
+      if (stbuf.st_mode & S_IXUSR)
+        mode = 0755;
+      else /* otherwise executable by none */
+        mode = 0644;
 
-      if (TEMP_FAILURE_RETRY (utimensat (parent_dfd, rel_path, times, AT_SYMLINK_NOFOLLOW)) != 0)
+      if (fchmodat (parent_dfd, rel_path, mode, 0) != 0)
         {
           glnx_set_error_from_errno (error);
-          return FALSE;
+          res = FALSE;
+        }
+    }
+  else if (S_ISLNK(stbuf.st_mode))
+    {
+      /* symlinks have no permissions */
+    }
+  else
+    {
+      /* some weird non-canonical type, lets delete it */
+      if (unlinkat(parent_dfd, rel_path, 0) != 0)
+        {
+          glnx_set_error_from_errno (error);
+          res = FALSE;
         }
     }
 
-  return TRUE;
+  return res;
+}
+
+/* Canonicalizes files to the same permissions as bare-user-only checkouts */
+gboolean
+flatpak_canonicalize_permissions (int           parent_dfd,
+                                  const char   *rel_path,
+                                  GError      **error)
+{
+  return _flatpak_canonicalize_permissions (parent_dfd, rel_path, TRUE, error);
 }
 
 /* Make a directory, and its parent. Don't error if it already exists.