run: Share /tmp between all instances of an app-ID

This allows apps that use /tmp as an IPC rendezvous point, such as those
that embed Chromium-derived browsers, to communicate between instances;
this would not previously have worked without --filesystem=/tmp, which
is a significant weakening of the sandbox.

It also allows /tmp to be shared with subsandboxes (if they are not
sandboxed more strictly).

The temporary directory is actually created in XDG_RUNTIME_DIR,
to avoid it becoming visible to unrelated apps that happen to have
--filesystem=/tmp.

Signed-off-by: Simon McVittie <smcv@collabora.com>

app/flatpak-builtins-build.c

@@ -553,7 +553,8 @@ flatpak_builtin_build (int argc, char **argv, GCancellable *cancellable, GError
     return FALSE;
 
   if (!flatpak_run_add_environment_args (bwrap, app_info_path, run_flags, id,
-                                         app_context, app_id_dir, NULL, NULL, cancellable, error))
+                                         app_context, app_id_dir, NULL, -1,
+                                         NULL, cancellable, error))
     return FALSE;
 
   for (i = 0; opt_bind_mounts != NULL && opt_bind_mounts[i] != NULL; i++)

common/flatpak-dir.c

@@ -7852,7 +7852,8 @@ apply_extra_data (FlatpakDir   *self,
                                          FLATPAK_RUN_FLAG_NO_SYSTEM_BUS_PROXY |
                                          FLATPAK_RUN_FLAG_NO_A11Y_BUS_PROXY,
                                          id,
-                                         app_context, NULL, NULL, NULL, cancellable, error))
+                                         app_context, NULL, NULL, -1,
+                                         NULL, cancellable, error))
     return FALSE;
 
   flatpak_bwrap_envp_to_args (bwrap);

common/flatpak-instance-private.h

@@ -36,5 +36,9 @@ gboolean flatpak_instance_ensure_per_app_dir (const char *app_id,
                                               int *lock_fd_out,
                                               char **lock_path_out,
                                               GError **error);
+gboolean flatpak_instance_ensure_per_app_tmp (const char *app_id,
+                                              int per_app_dir_lock_fd,
+                                              char **shared_tmp_out,
+                                              GError **error);
 
 #endif /* __FLATPAK_INSTANCE_PRIVATE_H__ */

common/flatpak-instance.c

@@ -530,6 +530,47 @@ flatpak_instance_ensure_per_app_dir (const char *app_id,
   return TRUE;
 }
 
+/*
+ * @app_id: $FLATPAK_ID
+ * @per_app_dir_lock_fd: Used to prove that we have already taken out
+ *  a per-app non-exclusive lock to stop this directory from being
+ *  garbage-collected
+ * @shared_tmp: (out) (not optional) (not nullable): Used to return
+ *  the path to the shared /tmp
+ *
+ * Create the per-app /tmp.
+ */
+gboolean
+flatpak_instance_ensure_per_app_tmp (const char *app_id,
+                                     int per_app_dir_lock_fd,
+                                     char **shared_tmp_out,
+                                     GError **error)
+{
+  g_autofree char *per_app_parent = NULL;
+  g_autofree char *shared_tmp = NULL;
+
+  g_return_val_if_fail (app_id != NULL, FALSE);
+  g_return_val_if_fail (shared_tmp_out != NULL, FALSE);
+  g_return_val_if_fail (*shared_tmp_out == NULL, FALSE);
+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
+
+  /* We don't actually do anything with this, we just pass it in here as
+   * proof that we already set up the directory that contains the lock
+   * file for per-app things, to force us to get the sequence right */
+  g_return_val_if_fail (per_app_dir_lock_fd >= 0, FALSE);
+
+  per_app_parent = flatpak_instance_get_apps_directory ();
+  shared_tmp = g_build_filename (per_app_parent, app_id, "tmp", NULL);
+
+  if (g_mkdir_with_parents (shared_tmp, 0700) != 0)
+    return glnx_throw_errno_prefix (error,
+                                    _("Unable to create directory %s"),
+                                    shared_tmp);
+
+  *shared_tmp_out = g_steal_pointer (&shared_tmp);
+  return TRUE;
+}
+
 /*
  * @host_dir_out: (not optional): used to return the directory on the host
  *  system representing this instance
@@ -685,11 +726,9 @@ flatpak_instance_gc_per_app_dirs (const char *instance_id,
 
   g_debug ("Cleaning up per-app-ID state for %s", app_id);
 
-  /* We don't practically use per_app_dir for anything yet, so delete
-   * an arbitrary subdirectory in order to have something to test */
-  if (!glnx_shutil_rm_rf_at (per_app_dir_fd, "test-cleanup", NULL, &local_error))
+  if (!glnx_shutil_rm_rf_at (per_app_dir_fd, "tmp", NULL, &local_error))
     {
-      g_debug ("Unable to clean up %s/test-cleanup: %s", per_app_dir,
+      g_debug ("Unable to clean up %s/tmp: %s", per_app_dir,
                local_error->message);
       g_clear_error (&local_error);
     }

common/flatpak-run-private.h

@@ -130,6 +130,7 @@ gboolean flatpak_run_add_environment_args (FlatpakBwrap       *bwrap,
                                            FlatpakContext     *context,
                                            GFile              *app_id_dir,
                                            GPtrArray          *previous_app_id_dirs,
+                                           int                 per_app_dir_lock_fd,
                                            FlatpakExports    **exports_out,
                                            GCancellable       *cancellable,
                                            GError            **error);

common/flatpak-run.c

@@ -209,6 +209,23 @@ flatpak_run_add_x11_args (FlatpakBwrap *bwrap,
   /* Always cover /tmp/.X11-unix, that way we never see the host one in case
    * we have access to the host /tmp. If you request X access we'll put the right
    * thing in this anyway.
+   *
+   * We need to be a bit careful here, because there are two situations in
+   * which potentially hostile processes have access to /tmp and could
+   * create symlinks, which in principle could cause us to create the
+   * directory and mount the tmpfs at the target of the symlink instead
+   * of in the intended place:
+   *
+   * - With --filesystem=/tmp, it's the host /tmp - but because of the
+   *   special historical status of /tmp/.X11-unix, we can assume that
+   *   it is pre-created by the host system before user code gets to run.
+   *
+   * - When /tmp is shared between all instances of the same app ID,
+   *   in principle the app has control over what's in /tmp, but in
+   *   practice it can't interfere with /tmp/.X11-unix, because we do
+   *   this unconditionally - therefore by the time app code runs,
+   *   /tmp/.X11-unix is already a mount point, meaning the app cannot
+   *   rename or delete it.
    */
   flatpak_bwrap_add_args (bwrap,
                           "--tmpfs", "/tmp/.X11-unix",
@@ -1337,6 +1354,10 @@ flatpak_run_add_extension_args (FlatpakBwrap      *bwrap,
   return TRUE;
 }
 
+/*
+ * @per_app_dir_lock_fd: If >= 0, make use of per-app directories in
+ *  the host's XDG_RUNTIME_DIR to share /tmp between instances.
+ */
 gboolean
 flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,
                                   const char      *app_info_path,
@@ -1345,6 +1366,7 @@ flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,
                                   FlatpakContext  *context,
                                   GFile           *app_id_dir,
                                   GPtrArray       *previous_app_id_dirs,
+                                  int              per_app_dir_lock_fd,
                                   FlatpakExports **exports_out,
                                   GCancellable    *cancellable,
                                   GError         **error)
@@ -1356,6 +1378,7 @@ flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,
   gboolean has_wayland = FALSE;
   gboolean allow_x11 = FALSE;
   gboolean home_access = FALSE;
+  gboolean sandboxed = (flags & FLATPAK_RUN_FLAG_SANDBOX) != 0;
 
   if ((context->shares & FLATPAK_CONTEXT_SHARED_IPC) == 0)
     {
@@ -1467,6 +1490,35 @@ flatpak_run_add_environment_args (FlatpakBwrap    *bwrap,
                                               app_id_dir, previous_app_id_dirs,
                                               TRUE, TRUE,
                                               &xdg_dirs_conf, &home_access);
+
+  if (flatpak_exports_path_is_visible (exports, "/tmp"))
+    {
+      /* The original sandbox and any subsandboxes are both already
+       * going to share /tmp with the host, so by transitivity they will
+       * also share it with each other, and with all other instances. */
+    }
+  else if (per_app_dir_lock_fd >= 0 && !sandboxed)
+    {
+      g_autofree char *shared_tmp = NULL;
+
+      /* The host and the original sandbox have separate /tmp,
+       * but we want other instances to be able to share /tmp with the
+       * first sandbox, unless they were created by
+       * flatpak-spawn --sandbox.
+       *
+       * In apply_extra and `flatpak build`, per_app_dir_lock_fd is
+       * negative and we skip this. */
+      if (!flatpak_instance_ensure_per_app_tmp (app_id,
+                                                per_app_dir_lock_fd,
+                                                &shared_tmp,
+                                                error))
+        return FALSE;
+
+      flatpak_bwrap_add_args (bwrap,
+                              "--bind", shared_tmp, "/tmp",
+                              NULL);
+    }
+
   flatpak_context_append_bwrap_filesystem (context, bwrap, app_id, app_id_dir,
                                            exports, xdg_dirs_conf, home_access);
 
@@ -4146,6 +4198,7 @@ flatpak_run_app (FlatpakDecomposed *app_ref,
 
   if (!flatpak_run_add_environment_args (bwrap, app_info_path, flags,
                                          app_id, app_context, app_id_dir, previous_app_id_dirs,
+                                         per_app_dir_lock_fd,
                                          &exports, cancellable, error))
     return FALSE;
 

tests/test-instance.c

@@ -63,7 +63,7 @@ test_gc (void)
   g_autofree char *hold_lock = g_test_build_filename (G_TEST_BUILT, "hold-lock", NULL);
   g_autofree char *alive_app_dir = NULL;
   g_autofree char *alive_app_lock = NULL;
-  g_autofree char *alive_app_test_cleanup = NULL;
+  g_autofree char *alive_app_tmp = NULL;
   g_autofree char *alive_instance_dir = NULL;
   g_autofree char *alive_instance_info = NULL;
   g_autofree char *alive_instance_lock = NULL;
@@ -72,7 +72,7 @@ test_gc (void)
   g_autofree char *alive_dead_instance_lock = NULL;
   g_autofree char *dead_app_dir = NULL;
   g_autofree char *dead_app_lock = NULL;
-  g_autofree char *dead_app_test_cleanup = NULL;
+  g_autofree char *dead_app_tmp = NULL;
   g_autofree char *dead_instance_dir = NULL;
   g_autofree char *dead_instance_info = NULL;
   g_autofree char *dead_instance_lock = NULL;
@@ -96,9 +96,9 @@ test_gc (void)
    * A second instance, #2, was running until recently but has exited. */
   alive_app_dir = g_build_filename (apps_dir, "com.example.Alive", NULL);
   g_assert_no_errno (g_mkdir_with_parents (alive_app_dir, 0700));
-  alive_app_test_cleanup = g_build_filename (alive_app_dir, "test-cleanup", NULL);
-  g_assert_no_errno (g_mkdir_with_parents (alive_app_test_cleanup, 0700));
-  populate_with_files (alive_app_test_cleanup);
+  alive_app_tmp = g_build_filename (alive_app_dir, "tmp", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (alive_app_tmp, 0700));
+  populate_with_files (alive_app_tmp);
   alive_app_lock = g_build_filename (alive_app_dir, ".ref", NULL);
   g_file_set_contents (alive_app_lock, "", 0, &error);
   g_assert_no_error (error);
@@ -153,9 +153,9 @@ test_gc (void)
    * Instance #4 was running until recently but has exited. */
   dead_app_dir = g_build_filename (apps_dir, "com.example.Dead", NULL);
   g_assert_no_errno (g_mkdir_with_parents (dead_app_dir, 0700));
-  dead_app_test_cleanup = g_build_filename (dead_app_dir, "test-cleanup", NULL);
-  g_assert_no_errno (g_mkdir_with_parents (dead_app_test_cleanup, 0700));
-  populate_with_files (dead_app_test_cleanup);
+  dead_app_tmp = g_build_filename (dead_app_dir, "tmp", NULL);
+  g_assert_no_errno (g_mkdir_with_parents (dead_app_tmp, 0700));
+  populate_with_files (dead_app_tmp);
   dead_app_lock = g_build_filename (dead_app_dir, ".ref", NULL);
   g_file_set_contents (dead_app_lock, "", 0, &error);
   g_assert_no_error (error);
@@ -198,12 +198,10 @@ test_gc (void)
   g_assert_no_errno (stat (dead_app_dir, &stat_buf));
   g_assert_no_errno (stat (dead_app_lock, &stat_buf));
 
-  /* Until we implement something that actively uses this directory,
-   * use the test-cleanup subdirectory to check whether GC took place.
-   * We GC the test-cleanup subdirectory if there is no instance alive.
+  /* We GC the tmp subdirectory if there is no instance alive.
    * We do not GC it if there is still an instance holding the lock. */
-  g_assert_no_errno (stat (alive_app_test_cleanup, &stat_buf));
-  g_assert_cmpint (stat (dead_app_test_cleanup, &stat_buf) == 0 ? 0 : errno, ==, ENOENT);
+  g_assert_no_errno (stat (alive_app_tmp, &stat_buf));
+  g_assert_cmpint (stat (dead_app_tmp, &stat_buf) == 0 ? 0 : errno, ==, ENOENT);
 
   g_assert_cmpuint (instances->len, ==, 1);
   instance = g_ptr_array_index (instances, 0);