From b65b3f6eadd51bd6600df2c0d07f902a552163d2 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 7 Apr 2021 19:02:36 +0100 Subject: [PATCH] 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 --- app/flatpak-builtins-build.c | 3 +- common/flatpak-dir.c | 3 +- common/flatpak-instance-private.h | 4 +++ common/flatpak-instance.c | 47 ++++++++++++++++++++++++--- common/flatpak-run-private.h | 1 + common/flatpak-run.c | 53 +++++++++++++++++++++++++++++++ tests/test-instance.c | 24 +++++++------- 7 files changed, 116 insertions(+), 19 deletions(-) diff --git a/app/flatpak-builtins-build.c b/app/flatpak-builtins-build.c index f69db5c7..d98a02ea 100644 --- a/app/flatpak-builtins-build.c +++ b/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++) diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c index f26e50f0..7fbe3493 100644 --- a/common/flatpak-dir.c +++ b/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); diff --git a/common/flatpak-instance-private.h b/common/flatpak-instance-private.h index 0dda174b..a84dad38 100644 --- a/common/flatpak-instance-private.h +++ b/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__ */ diff --git a/common/flatpak-instance.c b/common/flatpak-instance.c index 73f155e2..123f0a69 100644 --- a/common/flatpak-instance.c +++ b/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); } diff --git a/common/flatpak-run-private.h b/common/flatpak-run-private.h index 7dd2ad7a..2bb806e3 100644 --- a/common/flatpak-run-private.h +++ b/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); diff --git a/common/flatpak-run.c b/common/flatpak-run.c index 65af230e..1260113e 100644 --- a/common/flatpak-run.c +++ b/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; diff --git a/tests/test-instance.c b/tests/test-instance.c index 2ea23e3e..2e6e8a37 100644 --- a/tests/test-instance.c +++ b/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);