From cb47d83b7221de7f08a487dd47e69eec57f458e5 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 16 Feb 2021 18:51:23 +0000 Subject: [PATCH] run: Add option to share /dev/shm between instances of an app-ID Similar to /tmp, applications might well use /dev/shm as an IPC rendezvous between instances, which wouldn't have worked without --device=shm until now. Because /dev/shm has specific characteristics (in particular it's meant to always be a tmpfs), we offload the actual storage into a subdirectory of the real /dev/shm. Because /dev/shm is a shared directory between all uids, we have to be extra-careful how we do this, which is why the test coverage here is important. This is done on an opt-in basis because of its extra complexity. Signed-off-by: Simon McVittie --- .github/workflows/check.yml | 2 + common/flatpak-context-private.h | 1 + common/flatpak-context.c | 9 +- common/flatpak-instance-private.h | 12 ++ common/flatpak-instance.c | 286 +++++++++++++++++++++++++++++- common/flatpak-run.c | 87 +++++++-- doc/flatpak-build-finish.xml | 13 +- doc/flatpak-build.xml | 8 +- doc/flatpak-metadata.xml | 13 ++ doc/flatpak-override.xml | 8 +- tests/Makefile.am.inc | 6 +- tests/test-completion.sh | 1 + tests/test-exports.c | 5 +- tests/test-instance.c | 271 ++++++++++++++++++++++++++++ 14 files changed, 699 insertions(+), 23 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index fc293cd6..bc80cbcf 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -41,6 +41,8 @@ jobs: libjson-glib-dev shared-mime-info desktop-file-utils libpolkit-agent-1-dev libpolkit-gobject-1-dev \ libseccomp-dev libsoup2.4-dev libsystemd-dev libxml2-utils libgpgme11-dev gobject-introspection \ libgirepository1.0-dev libappstream-glib-dev libdconf-dev clang socat meson libdbus-1-dev e2fslibs-dev + # One of the tests wants this + sudo mkdir /tmp/flatpak-com.example.App-OwnedByRoot - name: Check out flatpak uses: actions/checkout@v1 with: diff --git a/common/flatpak-context-private.h b/common/flatpak-context-private.h index 1a35793a..7a35bb65 100644 --- a/common/flatpak-context-private.h +++ b/common/flatpak-context-private.h @@ -57,6 +57,7 @@ typedef enum { FLATPAK_CONTEXT_FEATURE_MULTIARCH = 1 << 1, FLATPAK_CONTEXT_FEATURE_BLUETOOTH = 1 << 2, FLATPAK_CONTEXT_FEATURE_CANBUS = 1 << 3, + FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM = 1 << 4, } FlatpakContextFeatures; struct FlatpakContext diff --git a/common/flatpak-context.c b/common/flatpak-context.c index bf693134..f18bac49 100644 --- a/common/flatpak-context.c +++ b/common/flatpak-context.c @@ -78,6 +78,7 @@ const char *flatpak_context_features[] = { "multiarch", "bluetooth", "canbus", + "per-app-dev-shm", NULL }; @@ -2064,6 +2065,11 @@ gboolean flatpak_context_adds_permissions (FlatpakContext *old, FlatpakContext *new) { + /* We allow upgrade to multiarch, that is really not a huge problem. + * Similarly, having sensible semantics for /dev/shm is + * not a security concern. */ + guint32 harmless_features = (FLATPAK_CONTEXT_FEATURE_MULTIARCH | + FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM); guint32 old_sockets; if (adds_flags (old->shares & old->shares_valid, @@ -2085,8 +2091,7 @@ flatpak_context_adds_permissions (FlatpakContext *old, new->devices & new->devices_valid)) return TRUE; - /* We allow upgrade to multiarch, that is really not a huge problem */ - if (adds_flags ((old->features & old->features_valid) | FLATPAK_CONTEXT_FEATURE_MULTIARCH, + if (adds_flags ((old->features & old->features_valid) | harmless_features, new->features & new->features_valid)) return TRUE; diff --git a/common/flatpak-instance-private.h b/common/flatpak-instance-private.h index a84dad38..63c74dde 100644 --- a/common/flatpak-instance-private.h +++ b/common/flatpak-instance-private.h @@ -30,12 +30,24 @@ char *flatpak_instance_get_instances_directory (void); char *flatpak_instance_allocate_id (char **host_dir_out, int *lock_fd_out); +gboolean flatpak_instance_claim_per_app_temp_directory (const char *app_id, + int per_app_dir_lock_fd, + int at_fd, + const char *link_path, + const char *parent, + char **path_out, + GError **error); + void flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances); 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_dev_shm (const char *app_id, + int per_app_dir_lock_fd, + char **shared_dev_shm_out, + GError **error); gboolean flatpak_instance_ensure_per_app_tmp (const char *app_id, int per_app_dir_lock_fd, char **shared_tmp_out, diff --git a/common/flatpak-instance.c b/common/flatpak-instance.c index 123f0a69..64ae4597 100644 --- a/common/flatpak-instance.c +++ b/common/flatpak-instance.c @@ -20,6 +20,10 @@ #include "config.h" +#include +#include + +#include "flatpak-utils-base-private.h" #include "flatpak-utils-private.h" #include "flatpak-run-private.h" #include "flatpak-instance.h" @@ -530,6 +534,102 @@ 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): Used to return the path to the + * shared /dev/shm + * + * Create the per-app /dev/shm. + */ +gboolean +flatpak_instance_ensure_per_app_dev_shm (const char *app_id, + int per_app_dir_lock_fd, + char **shared_dev_shm_out, + GError **error) +{ + /* This function is actually generic, since we might well want to + * offload other directories in the same way - but the only directory + * we do this for right now is /dev/shm. */ + static const char link_name[] = "dev-shm"; + static const char parent[] = "/dev/shm"; + g_autofree gchar *flag_file = NULL; + g_autofree gchar *path = NULL; + g_autofree gchar *per_app_parent = NULL; + g_autofree gchar *per_app_dir = NULL; + glnx_autofd int flag_fd = -1; + glnx_autofd int per_app_dir_fd = -1; + + g_return_val_if_fail (app_id != NULL, FALSE); + g_return_val_if_fail (per_app_dir_lock_fd >= 0, FALSE); + g_return_val_if_fail (shared_dev_shm_out != NULL, FALSE); + g_return_val_if_fail (*shared_dev_shm_out == NULL, FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + per_app_parent = flatpak_instance_get_apps_directory (); + + per_app_dir = g_build_filename (per_app_parent, app_id, NULL); + per_app_dir_fd = openat (AT_FDCWD, per_app_dir, + O_PATH | O_DIRECTORY | O_CLOEXEC); + + /* This can't happen under normal circumstances: if we have the lock, + * then the directory it's in had better exist. */ + if (per_app_dir_fd < 0) + return glnx_throw_errno_prefix (error, + _("Unable to open directory %s"), + per_app_dir); + + /* If there's an existing symlink to a suitable directory, we can + * reuse it (carefully). This gives us the sharing we wanted between + * multiple instances of the same app, and between app and subsandbox. */ + if (flatpak_instance_claim_per_app_temp_directory (app_id, + per_app_dir_lock_fd, + per_app_dir_fd, + link_name, + parent, + &path, + NULL)) + { + *shared_dev_shm_out = g_steal_pointer (&path); + return TRUE; + } + + /* Otherwise create a new directory in @parent, and make @link_name + * a symlink to it. */ + + /* /dev/shm/flatpak-$FLATPAK_ID-XXXXXX */ + path = g_strdup_printf ("%s/flatpak-%s-XXXXXX", parent, app_id); + + if (g_mkdtemp (path) == NULL) + return glnx_throw_errno_prefix (error, + _("Unable to create temporary directory in %s"), + parent); + + /* This marks this directory as an expendable temp directory, and is + * inspired by the use of .testtmp in libostree. */ + flag_file = g_build_filename (path, ".flatpak-tmpdir", NULL); + flag_fd = openat (AT_FDCWD, flag_file, + O_RDONLY | O_CREAT | O_EXCL | O_CLOEXEC | O_NOFOLLOW | O_NOCTTY, + 0600); + + if (flag_fd < 0) + return glnx_throw_errno_prefix (error, + _("Unable to create file %s"), + path); + + /* Replace the symlink */ + if ((unlinkat (per_app_dir_fd, link_name, 0) < 0 && errno != ENOENT) || + symlinkat (path, per_app_dir_fd, link_name) < 0) + return glnx_throw_errno_prefix (error, + _("Unable to update symbolic link %s/%s"), + per_app_dir, link_name); + + *shared_dev_shm_out = g_steal_pointer (&path); + return TRUE; +} + /* * @app_id: $FLATPAK_ID * @per_app_dir_lock_fd: Used to prove that we have already taken out @@ -645,6 +745,130 @@ flatpak_instance_new_for_id (const char *id) return flatpak_instance_new (dir); } +/* + * flatpak_instance_claim_per_app_temp_directory: + * @app_id: $FLATPAK_ID + * @per_app_dir_lock_fd: Lock representing shared or exclusive access + * to directories created for @app_id + * @at_fd: Directory in which to look up @link_path + * @link_path: Path of a symbolic link to a subdirectory of @parent + * @parent: The directory in which we created the temporary directory, + * such as /dev/shm or /tmp + * @path_out: (out) (not optional): Return the path to the directory + * referenced by @link_path + * + * Try to take control of an existing per-app temporary directory + * referenced by @link_path, either for reuse or for deletion. + * Return %TRUE if we can. + * + * This is currently only used for /dev/shm, but it's designed to be + * equally usable for other non-user-owned directories like /tmp. + * + * We have to be careful here, because @link_path might be left over + * from a previous boot, and it probably points into a directory like + * /dev/shm or /tmp, where an attacker might recreate our directories, + * for example as symbolic links to somewhere they control. As a result, + * this function is security-sensitive, and needs to follow a policy of + * failing when an unexpected situation is detected. + * + * @error is not normally user-visible, and is mostly present to support + * debugging and unit testing. + * + * Returns: %TRUE if @link_path points to a suitable directory, + * or %FALSE with @error set if it does not. + */ +gboolean +flatpak_instance_claim_per_app_temp_directory (const char *app_id, + int per_app_dir_lock_fd, + int at_fd, + const char *link_path, + const char *parent, + char **path_out, + GError **error) +{ + g_autofree char *reuse_path = NULL; + glnx_autofd int dfd = -1; + glnx_autofd int flag_fd = -1; + struct stat statbuf; + const char *slash; + const char *rest; + + at_fd = glnx_dirfd_canonicalize (at_fd); + + g_return_val_if_fail (app_id != NULL, FALSE); + g_return_val_if_fail (per_app_dir_lock_fd >= 0, FALSE); + g_return_val_if_fail (at_fd == AT_FDCWD || at_fd >= 0, FALSE); + g_return_val_if_fail (link_path != NULL, FALSE); + g_return_val_if_fail (parent != NULL, FALSE); + g_return_val_if_fail (path_out != NULL, FALSE); + g_return_val_if_fail (*path_out == NULL, FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + reuse_path = glnx_readlinkat_malloc (at_fd, link_path, NULL, error); + + if (reuse_path == NULL) + return FALSE; + + /* If we're going to use it as /dev/shm, the directory on the + * host should match /dev/shm/flatpak-$FLATPAK_ID-XXXXXX */ + if (!g_str_has_prefix (reuse_path, parent)) + return glnx_throw (error, "%s does not start with %s", + reuse_path, parent); + + /* /flatpak-$FLATPAK_ID-XXXXXX */ + slash = reuse_path + strlen (parent); + + if (*slash != '/') + return glnx_throw (error, "%s does not start with %s/", + reuse_path, parent); + + /* flatpak-$FLATPAK_ID-XXXXXX */ + rest = slash + 1; + + if (!g_str_has_prefix (rest, "flatpak-")) + return glnx_throw (error, "%s does not start with %s/flatpak-", + reuse_path, parent); + + if (strchr (rest, '/') != NULL) + return glnx_throw (error, "%s has too many directory separators", + reuse_path); + + if (!g_str_has_prefix (rest + strlen ("flatpak-"), app_id)) + return glnx_throw (error, "%s does not start with %s/flatpak-%s", + reuse_path, parent, app_id); + + if (rest[strlen ("flatpak-") + strlen (app_id)] != '-') + return glnx_throw (error, "%s does not start with %s/flatpak-%s-", + reuse_path, parent, app_id); + + /* Avoid symlink attacks via O_NOFOLLOW */ + dfd = openat (AT_FDCWD, reuse_path, + O_PATH | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + + if (dfd < 0) + return glnx_throw_errno_prefix (error, "opening %s O_DIRECTORY|O_NOFOLLOW", + reuse_path); + + if (fstat (dfd, &statbuf) < 0) + return glnx_throw_errno_prefix (error, "fstat %s", reuse_path); + + /* We certainly don't want to reuse someone else's directory */ + if (statbuf.st_uid != geteuid ()) + return glnx_throw (error, "%s does not belong to this user", reuse_path); + + flag_fd = openat (dfd, ".flatpak-tmpdir", + O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NOCTTY); + + /* If we can't open the flag file, the most likely reason is that it + * isn't a directory that we created */ + if (flag_fd < 0) + return glnx_throw_errno_prefix (error, "opening flag file %s/.flatpak-tmpdir", + reuse_path); + + *path_out = g_steal_pointer (&reuse_path); + return TRUE; +} + /* * The @error is not intended to be user-facing, and is there for * testing/debugging. @@ -726,6 +950,61 @@ flatpak_instance_gc_per_app_dirs (const char *instance_id, g_debug ("Cleaning up per-app-ID state for %s", app_id); + /* /dev/shm is offloaded onto the host's /dev/shm to get consistent + * free space behaviour and make sure it's actually in RAM. It could + * contain relatively large files, so we clean it up. + * + * In principle this could be used for other directories such as /tmp, + * in a loop over an array of paths (hence this indentation), but we + * only do this for /dev/shm right now. */ + do + { + g_autofree char *path = NULL; + + /* /dev/shm is an attacker-controlled namespace, so we need to be + * careful what directories we will delete. We have to assume + * that attackers will create malicious symlinks in /dev/shm to + * try to trick us into opening or deleting the wrong files. */ + if (flatpak_instance_claim_per_app_temp_directory (app_id, + per_app_dir_lock_fd, + per_app_dir_fd, + "dev-shm", + "/dev/shm", + &path, + &local_error)) + { + g_assert (g_str_has_prefix (path, "/dev/shm/")); + + if (unlinkat (per_app_dir_fd, "dev-shm", 0) != 0) + g_debug ("Unable to clean up %s/%s: %s", + per_app_dir, "dev-shm", g_strerror (errno)); + + if (!glnx_shutil_rm_rf_at (AT_FDCWD, path, NULL, &local_error)) + { + g_debug ("Unable to clean up %s: %s", + path, local_error->message); + g_clear_error (&local_error); + } + } + else if (unlinkat (per_app_dir_fd, "dev-shm", 0) < 0 && errno == ENOENT) + { + /* ignore, the symlink wasn't even there anyway */ + g_clear_error (&local_error); + } + else + { + g_debug ("%s/%s no longer points to the expected directory and " + "was removed: %s", + per_app_dir, "dev-shm", local_error->message); + g_clear_error (&local_error); + } + } + while (0); + + /* We currently allocate the app's /tmp directly in the per-app directory + * on the host's XDG_RUNTIME_DIR, instead of offloading it into /tmp + * in a way that's analogous to /dev/shm, so we expect tmp to be a directory + * and not a symlink. If it's a symlink, we'll just unlink it. */ if (!glnx_shutil_rm_rf_at (per_app_dir_fd, "tmp", NULL, &local_error)) { g_debug ("Unable to clean up %s/tmp: %s", per_app_dir, @@ -780,9 +1059,14 @@ flatpak_instance_iterate_all_and_gc (GPtrArray *out_instances) fcntl (lock_fd, F_GETLK, &l) == 0 && l.l_type == F_UNLCK) { + g_autoptr(GError) local_error = NULL; + /* The instance is not used, remove it */ g_debug ("Cleaning up unused container id %s", dent->d_name); - flatpak_instance_gc_per_app_dirs (dent->d_name, NULL); + + if (!flatpak_instance_gc_per_app_dirs (dent->d_name, &local_error)) + flatpak_debug2 ("Not cleaning up per-app dir: %s", local_error->message); + glnx_shutil_rm_rf_at (iter.fd, dent->d_name, NULL, NULL); continue; } diff --git a/common/flatpak-run.c b/common/flatpak-run.c index fb77fa96..52fd9854 100644 --- a/common/flatpak-run.c +++ b/common/flatpak-run.c @@ -1401,10 +1401,39 @@ flatpak_run_add_environment_args (FlatpakBwrap *bwrap, /* Don't expose the host /dev/shm, just the device nodes, unless explicitly allowed */ if (g_file_test ("/dev/shm", G_FILE_TEST_IS_DIR)) { - if ((context->devices & FLATPAK_CONTEXT_DEVICE_SHM) == 0) - flatpak_bwrap_add_args (bwrap, - "--tmpfs", "/dev/shm", - NULL); + if (context->devices & FLATPAK_CONTEXT_DEVICE_SHM) + { + /* Don't do anything special: include shm in the + * shared /dev. The host and all sandboxes and subsandboxes + * all share /dev/shm */ + } + else if ((context->features & FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM) + && per_app_dir_lock_fd >= 0) + { + g_autofree char *shared_dev_shm = NULL; + + /* The host and the original sandbox have separate /dev/shm, + * but we want other instances to be able to share /dev/shm with + * the first sandbox (except for subsandboxes run with + * flatpak-spawn --sandbox, which will have their own). */ + if (!flatpak_instance_ensure_per_app_dev_shm (app_id, + per_app_dir_lock_fd, + &shared_dev_shm, + error)) + return FALSE; + + flatpak_bwrap_add_args (bwrap, + "--bind", shared_dev_shm, "/dev/shm", + NULL); + } + else + { + /* The host, the original sandbox and each subsandbox + * each have a separate /dev/shm. */ + flatpak_bwrap_add_args (bwrap, + "--tmpfs", "/dev/shm", + NULL); + } } else if (g_file_test ("/dev/shm", G_FILE_TEST_IS_SYMLINK)) { @@ -1416,13 +1445,35 @@ flatpak_run_add_environment_args (FlatpakBwrap *bwrap, { if (context->devices & FLATPAK_CONTEXT_DEVICE_SHM && g_file_test ("/run/shm", G_FILE_TEST_IS_DIR)) - flatpak_bwrap_add_args (bwrap, - "--bind", "/run/shm", "/run/shm", - NULL); + { + flatpak_bwrap_add_args (bwrap, + "--bind", "/run/shm", "/run/shm", + NULL); + } + else if ((context->features & FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM) + && per_app_dir_lock_fd >= 0) + { + g_autofree char *shared_dev_shm = NULL; + + /* The host and the original sandbox have separate /dev/shm, + * but we want other instances to be able to share /dev/shm, + * except for flatpak-spawn --subsandbox. */ + if (!flatpak_instance_ensure_per_app_dev_shm (app_id, + per_app_dir_lock_fd, + &shared_dev_shm, + error)) + return FALSE; + + flatpak_bwrap_add_args (bwrap, + "--bind", shared_dev_shm, "/run/shm", + NULL); + } else - flatpak_bwrap_add_args (bwrap, - "--dir", "/run/shm", - NULL); + { + flatpak_bwrap_add_args (bwrap, + "--dir", "/run/shm", + NULL); + } } else g_warning ("Unexpected /dev/shm symlink %s", link); @@ -1484,8 +1535,22 @@ flatpak_run_add_environment_args (FlatpakBwrap *bwrap, if (real_dev_shm != NULL) flatpak_bwrap_add_args (bwrap, "--bind", real_dev_shm, "/dev/shm", NULL); } - } + else if ((context->features & FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM) + && per_app_dir_lock_fd >= 0) + { + g_autofree char *shared_dev_shm = NULL; + if (!flatpak_instance_ensure_per_app_dev_shm (app_id, + per_app_dir_lock_fd, + &shared_dev_shm, + error)) + return FALSE; + + flatpak_bwrap_add_args (bwrap, + "--bind", shared_dev_shm, "/dev/shm", + NULL); + } + } exports = flatpak_context_get_exports_full (context, app_id_dir, previous_app_id_dirs, diff --git a/doc/flatpak-build-finish.xml b/doc/flatpak-build-finish.xml index 0627d0fb..b2b138f1 100644 --- a/doc/flatpak-build-finish.xml +++ b/doc/flatpak-build-finish.xml @@ -184,7 +184,8 @@ Allow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. The devel feature allows the application to @@ -205,6 +206,13 @@ The canbus feature allows the application to use canbus (AF_CAN) sockets. Note, for this work you must also have network access. + + + The per-app-dev-shm feature shares a single + instance of /dev/shm between the + application, any unrestricted subsandboxes that it creates, + and any other instances of the application that are + launched while it is running. @@ -214,7 +222,8 @@ Disallow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. diff --git a/doc/flatpak-build.xml b/doc/flatpak-build.xml index 1bbf9a52..a48e2acd 100644 --- a/doc/flatpak-build.xml +++ b/doc/flatpak-build.xml @@ -194,7 +194,9 @@ Allow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: + devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. See flatpak-build-finish1 @@ -208,7 +210,9 @@ Disallow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: + devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. diff --git a/doc/flatpak-metadata.xml b/doc/flatpak-metadata.xml index f4cd84ce..2d853ade 100644 --- a/doc/flatpak-metadata.xml +++ b/doc/flatpak-metadata.xml @@ -478,6 +478,19 @@ Available since 1.0.3. + + + Share a single instance of + /dev/shm between all + instances of this application run by the same + user ID, including sub-sandboxes. + If the application has the + device permission in its + list, then this + feature flag is ignored. + Available since 1.12.0. + + A feature can be prefixed with to indicate the absence of that feature, for example diff --git a/doc/flatpak-override.xml b/doc/flatpak-override.xml index 306026cb..e2768a42 100644 --- a/doc/flatpak-override.xml +++ b/doc/flatpak-override.xml @@ -176,7 +176,9 @@ Allow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: + devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. See flatpak-build-finish1 @@ -190,7 +192,9 @@ Disallow access to a specific feature. This updates the [Context] group in the metadata. - FEATURE must be one of: devel, multiarch, bluetooth, canbus. + FEATURE must be one of: + devel, multiarch, bluetooth, canbus, + per-app-dev-shm. This option can be used multiple times. diff --git a/tests/Makefile.am.inc b/tests/Makefile.am.inc index 1e303479..283f76c2 100644 --- a/tests/Makefile.am.inc +++ b/tests/Makefile.am.inc @@ -88,7 +88,11 @@ test_exports_SOURCES = tests/test-exports.c test_instance_CFLAGS = $(testcommon_CFLAGS) test_instance_LDADD = $(testcommon_LDADD) libtestlib.la -test_instance_SOURCES = tests/test-instance.c +test_instance_SOURCES = \ + subprojects/libglnx/tests/libglnx-testlib.c \ + subprojects/libglnx/tests/libglnx-testlib.h \ + tests/test-instance.c \ + $(NULL) tests_hold_lock_CFLAGS = $(AM_CFLAGS) $(BASE_CFLAGS) tests_hold_lock_LDADD = $(AM_LDADD) $(BASE_LIBS) libglnx.la diff --git a/tests/test-completion.sh b/tests/test-completion.sh index 4b40ceff..33649660 100755 --- a/tests/test-completion.sh +++ b/tests/test-completion.sh @@ -122,6 +122,7 @@ bluetooth canbus devel multiarch +per-app-dev-shm EOF ok "complete --allow" diff --git a/tests/test-exports.c b/tests/test-exports.c index 8b714096..e89a38cd 100644 --- a/tests/test-exports.c +++ b/tests/test-exports.c @@ -233,7 +233,7 @@ test_full_context (void) g_key_file_set_value (keyfile, FLATPAK_METADATA_GROUP_CONTEXT, FLATPAK_METADATA_KEY_FEATURES, - "devel;multiarch;bluetooth;canbus;"); + "devel;multiarch;bluetooth;canbus;per-app-dev-shm;"); g_key_file_set_value (keyfile, FLATPAK_METADATA_GROUP_CONTEXT, FLATPAK_METADATA_KEY_FILESYSTEMS, @@ -292,7 +292,8 @@ test_full_context (void) (FLATPAK_CONTEXT_FEATURE_DEVEL | FLATPAK_CONTEXT_FEATURE_MULTIARCH | FLATPAK_CONTEXT_FEATURE_BLUETOOTH | - FLATPAK_CONTEXT_FEATURE_CANBUS)); + FLATPAK_CONTEXT_FEATURE_CANBUS | + FLATPAK_CONTEXT_FEATURE_PER_APP_DEV_SHM)); g_assert_cmpuint (context->features_valid, ==, context->features); g_assert_cmpuint (flatpak_context_get_run_flags (context), ==, diff --git a/tests/test-instance.c b/tests/test-instance.c index 2e6e8a37..411602e9 100644 --- a/tests/test-instance.c +++ b/tests/test-instance.c @@ -33,6 +33,7 @@ #include "flatpak-run-private.h" #include "libglnx/libglnx.h" +#include "libglnx/tests/libglnx-testlib.h" #include "testlib.h" @@ -215,6 +216,274 @@ test_gc (void) g_spawn_close_pid (pid); } +static void +test_claim_per_app_temp_directory (void) +{ + /* Run in a temporary directory so we can create a bunch of symlinks */ + _GLNX_TEST_SCOPED_TEMP_DIR; + + gboolean ok; + glnx_autofd int lock_fd = -1; + glnx_autofd int fd = -1; + g_autofree char *result = NULL; + g_autofree char *flag_path = NULL; + g_autofree char *symlink_path = NULL; + g_autofree char *non_directory_path = NULL; + g_autofree char *dir_in_tmp = NULL; + g_autoptr(GError) error = NULL; + struct stat stat_buf; + + /* In real life this would be the per-app-ID lock, but in fact + * we just need some sort of file descriptor - as currently + * implemented, we don't even need to lock it. */ + lock_fd = open ("mock-per-app-id-lock", + O_CLOEXEC | O_CREAT | O_NOCTTY | O_NOFOLLOW, + 0600); + g_assert_no_errno (lock_fd >= 0 ? 0 : -1); + + /* This emulates the sort of directory that we want to reuse. */ + dir_in_tmp = g_strdup ("/tmp/flatpak-com.example.App-XXXXXX"); + g_assert_nonnull (g_mkdtemp (dir_in_tmp)); + + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "doesnt-exist", + "/tmp", + &result, + &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* If link_path is a symlink to a directory not in /tmp, we refuse + * to reuse it */ + g_assert_no_errno (symlink ("/nope", "bad-prefix")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "bad-prefix", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, "/nope does not start with /tmp"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* Similar */ + g_assert_no_errno (symlink ("/tmptation", "bad-prefix2")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "bad-prefix2", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, "/tmptation does not start with /tmp/"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* If link_path points to a subdirectory of /tmp that doesn't match the + * expected pattern, we refuse to reuse it */ + g_assert_no_errno (symlink ("/tmp/nope", "bad-prefix3")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "bad-prefix3", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, "/tmp/nope does not start with /tmp/flatpak-"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* Similar */ + g_assert_no_errno (symlink ("/tmp/flatpak-/nope", "too-many-levels")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "too-many-levels", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, + "/tmp/flatpak-/nope has too many directory separators"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* Similar */ + g_assert_no_errno (symlink ("/tmp/flatpak-abc/", "too-many-levels2")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "too-many-levels2", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, + "/tmp/flatpak-abc/ has too many directory separators"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + g_assert_no_errno (symlink ("/tmp/flatpak-org.example.Other-XXXXXX", "wrong-app")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "wrong-app", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, + "/tmp/flatpak-org.example.Other-XXXXXX does not " + "start with /tmp/flatpak-com.example.App"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + g_assert_no_errno (symlink ("/tmp/flatpak-com.example.ApparentlyNot", "wrong-app2")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "wrong-app2", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, + "/tmp/flatpak-com.example.ApparentlyNot does not " + "start with /tmp/flatpak-com.example.App-"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* If it points to a filesystem object matching the right pattern, but + * that is not a directory, we refuse to reuse it */ + non_directory_path = g_strdup ("/tmp/flatpak-com.example.App-XXXXXX"); + g_assert_no_errno ((fd = g_mkstemp (non_directory_path))); + g_assert_no_errno (symlink (non_directory_path, "not-a-directory")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "not-a-directory", + "/tmp", + &result, + &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_NOT_DIRECTORY); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* Reuse @non_directory_path as the name of a symlink to a directory: + * we consider that to be equally invalid. Create it inside our + * directory in /tmp so that we can rename() it into place, + * because symlink() does not overwrite, but rename() does. */ + symlink_path = g_build_filename (dir_in_tmp, "symlink", NULL); + g_assert_no_errno (symlink (dir_in_tmp, symlink_path)); + /* Overwrite the file with the symlink */ + g_assert_no_errno (rename (symlink_path, non_directory_path)); + + /* We'll refuse to follow the symlink: for all we know it could be + * attacker-controlled. */ + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "not-a-directory", + "/tmp", + &result, + &error); + + /* Either of these would be reasonable */ + if (error->code == G_IO_ERROR_TOO_MANY_LINKS) + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_TOO_MANY_LINKS); + else + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_NOT_DIRECTORY); + + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* If link_path points to a directory owned by someone else, we refuse + * to use it. This part of the test will be skipped unless you pre-create + * this directory as root. */ + if (stat ("/tmp/flatpak-com.example.App-OwnedByRoot", &stat_buf) == 0 + && stat_buf.st_uid == 0 + && geteuid () != 0) + { + g_assert_no_errno (symlink ("/tmp/flatpak-com.example.App-OwnedByRoot", + "not-our-directory")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "not-our-directory", + "/tmp", + &result, + &error); + g_assert_nonnull (error); + g_assert_cmpstr (error->message, ==, + "/tmp/flatpak-com.example.App-OwnedByRoot does not " + "belong to this user"); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + } + + glnx_close_fd (&fd); + g_assert_no_errno (unlink (non_directory_path)); + g_clear_pointer (&non_directory_path, g_free); + + /* Even when we have a symlink to a directory matching the right pattern + * that we own, if it doesn't contain the flag file that indicates that + * it's one of our temp directories, we'll still refuse to use it. */ + g_assert_no_errno (symlink (dir_in_tmp, "good-symlink")); + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "good-symlink", + "/tmp", + &result, + &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND); + g_assert_true (g_str_has_prefix (error->message, + "opening flag file /tmp/flatpak-com.example.App-")); + g_assert_nonnull (strstr (error->message, "/.flatpak-tmpdir:")); + g_assert_null (result); + g_assert_false (ok); + g_clear_error (&error); + + /* Create the flag file (of course in real life this would have happened + * much sooner) */ + flag_path = g_build_filename (dir_in_tmp, ".flatpak-tmpdir", NULL); + g_file_set_contents (flag_path, "", 0, &error); + g_assert_no_error (error); + + /* Now we are finally willing to reuse the directory! A happy ending + * at last. */ + ok = flatpak_instance_claim_per_app_temp_directory ("com.example.App", + lock_fd, + AT_FDCWD, + "good-symlink", + "/tmp", + &result, + &error); + g_assert_no_error (error); + g_assert_cmpstr (result, ==, dir_in_tmp); + g_assert_true (ok); + + g_assert_no_errno (unlink (flag_path)); +} + int main (int argc, char *argv[]) { @@ -225,6 +494,8 @@ main (int argc, char *argv[]) g_test_init (&argc, &argv, NULL); g_test_add_func ("/instance/gc", test_gc); + g_test_add_func ("/instance/claim-per-app-temp-directory", + test_claim_per_app_temp_directory); res = g_test_run ();