From e7ad74c3983828efa5aa5dccf95bb639616693e9 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 17 May 2017 14:28:52 +0200 Subject: [PATCH] run: Fix race condition in app identification There is a race condition in how the portals detects the peer app-id. If we manager to open /proc/$pid/root, and then openat(fd, ".flatpak-info"), but the process dies inbetween the two, then the .flatpak-info read-only bind mount (and all other mounts in the namespace except the root one) is unmounted, so we will find and empty .flatpakinfo file. We fix this race by storing the contents in a regular file, but also as a readonly bind mount on top of it. For typical dbus portals the pid is the dbus proxy though, and in that case the app can't modify the file, so we make it just a file there instead of file + bind-mount. --- common/flatpak-run.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/common/flatpak-run.c b/common/flatpak-run.c index 0cef78a9b..bf787aa86 100644 --- a/common/flatpak-run.c +++ b/common/flatpak-run.c @@ -3443,10 +3443,11 @@ flatpak_run_add_app_info_args (GPtrArray *argv_array, GError **error) { g_autofree char *tmp_path = NULL; - int fd; + int fd, fd2; g_autoptr(GKeyFile) keyfile = NULL; g_autofree char *runtime_path = NULL; g_autofree char *fd_str = NULL; + g_autofree char *fd2_str = NULL; g_autofree char *old_dest = g_strdup_printf ("/run/user/%d/flatpak-info", getuid ()); const char *group; @@ -3494,6 +3495,17 @@ flatpak_run_add_app_info_args (GPtrArray *argv_array, if (!g_key_file_save_to_file (keyfile, tmp_path, error)) return FALSE; + /* We want to create a file on /.flatpak-info that the app cannot modify, which + we do by creating a read-only bind mount. This way one can openat() + /proc/$pid/root, and if that succeeds use openat via that to find the + unfakable .flatpak-info file. However, there is a tiny race in that if + you manage to open /proc/$pid/root, but then the pid dies, then + every mount but the root is unmounted in the namespace, so the + .flatpak-info will be empty. We fix this by first creating a real file + with the real info in, then bind-mounting on top of that, the same info. + This way even if the bind-mount is unmounted we can find the real data. + */ + fd = open (tmp_path, O_RDONLY); if (fd == -1) { @@ -3503,14 +3515,29 @@ flatpak_run_add_app_info_args (GPtrArray *argv_array, return FALSE; } + fd2 = open (tmp_path, O_RDONLY); + if (fd2 == -1) + { + close (fd); + int errsv = errno; + g_set_error (error, G_IO_ERROR, g_io_error_from_errno (errsv), + _("Failed to open temp file: %s"), g_strerror (errsv)); + return FALSE; + } + unlink (tmp_path); fd_str = g_strdup_printf ("%d", fd); + fd2_str = g_strdup_printf ("%d", fd2); if (fd_array) - g_array_append_val (fd_array, fd); + { + g_array_append_val (fd_array, fd); + g_array_append_val (fd_array, fd2); + } add_args (argv_array, - "--ro-bind-data", fd_str, "/.flatpak-info", + "--file", fd_str, "/.flatpak-info", + "--ro-bind-data", fd2_str, "/.flatpak-info", "--symlink", "../../../.flatpak-info", old_dest, NULL); @@ -3762,7 +3789,9 @@ prepend_bwrap_argv_wrapper (GPtrArray *argv, g_ptr_array_add (bwrap_args, g_strdup (proxy_socket_dir)); g_ptr_array_add (bwrap_args, g_strdup (proxy_socket_dir)); - g_ptr_array_add (bwrap_args, g_strdup ("--ro-bind-data")); + /* This is a file rather than a bind mount, because it will then + not be unmounted from the namespace when the namespace dies. */ + g_ptr_array_add (bwrap_args, g_strdup ("--file")); g_ptr_array_add (bwrap_args, g_strdup_printf ("%d", app_info_fd)); g_ptr_array_add (bwrap_args, g_strdup ("/.flatpak-info"));