On Wed, 21 Jun 2017 at 09:46:21 +0100, Simon McVittie wrote:
> Security team: do you want a backport/DSA for stretch-security, or do
> you consider the mitigations to be sufficient to fix this through
> a stable update instead? I am hoping to get 0.8.7 into stretch r1 as a
> stable update, but 0.8.6 contains unrelated bug fixes that I realise
> you won't necessarily want in stretch-security (proposed-update tracked
> at <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=864028>).
Here is a proposed minimal backport for stretch in case you want one.
I have source and binaries for this ready for upload. Does the security
archive still want source packages built with debuild -sa, and do you
accept source-only uploads for stretch-security?
Thanks,
S
diffstat for flatpak-0.8.5 flatpak-0.8.5
changelog | 14
patches/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch | 178 ++++++++++
patches/dir-Ensure-.local-share-flatpak-is-0700.patch | 79 ++++
patches/series | 2
4 files changed, 273 insertions(+)
diff -Nru flatpak-0.8.5/debian/changelog flatpak-0.8.5/debian/changelog
--- flatpak-0.8.5/debian/changelog 2017-04-24 12:59:09.000000000 +0100
+++ flatpak-0.8.5/debian/changelog 2017-06-21 12:05:49.000000000 +0100
@@ -1,3 +1,17 @@
+flatpak (0.8.5-2+deb9u1) stretch-security; urgency=high
+
+ * d/p/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch:
+ Patch from upstream stable release 0.8.7.
+ Prevent deploying files with inappropriate permissions
+ (world-writable, setuid, etc.) (Closes: #865413)
+ * d/p/dir-Ensure-.local-share-flatpak-is-0700.patch:
+ Patch from upstream stable release 0.8.7.
+ Make ~/.local/share/flatpak private to user to defend against app
+ vendors that might have released files with inappropriate permissions
+ in the past
+
+ -- Simon McVittie <s...@debian.org> Wed, 21 Jun 2017 12:05:49 +0100
+
flatpak (0.8.5-2) unstable; urgency=medium
* flatpak Recommends xdg-desktop-portal-gtk | xdg-desktop-portal-backend,
diff -Nru flatpak-0.8.5/debian/patches/dir-Ensure-.local-share-flatpak-is-0700.patch flatpak-0.8.5/debian/patches/dir-Ensure-.local-share-flatpak-is-0700.patch
--- flatpak-0.8.5/debian/patches/dir-Ensure-.local-share-flatpak-is-0700.patch 1970-01-01 01:00:00.000000000 +0100
+++ flatpak-0.8.5/debian/patches/dir-Ensure-.local-share-flatpak-is-0700.patch 2017-06-21 12:05:49.000000000 +0100
@@ -0,0 +1,79 @@
+From: Colin Walters <walt...@verbum.org>
+Date: Thu, 8 Jun 2017 10:24:48 -0400
+Subject: dir: Ensure ~/.local/share/flatpak is 0700
+
+This goes into a big old topic about Unix homedir permissions; it's not uncommon
+for general purpose OS vendors to have homedirs be 0755. In that case,
+applications need to ensure confidentiality for data requiring it (classically
+e.g. `~/.ssh`) by making the dirs `0700`.
+
+While most of the data in the flatpak per-user dir probably isn't confidential
+(debatably) we have a different issue; if container content includes suid or
+world-writable files/dirs, then having that data accessible to other users
+is obviously problematic.
+
+We're going to fix flatpak/ostree to not create files with those modes
+to begin with, but this simple fix closes off the attack route for
+the per-user directory.
+
+A different fix will be necessary for the system-wide repo.
+
+See: https://github.com/flatpak/flatpak/pull/837
+(cherry picked from commit daf36ba2afab407c488c456952eecba16f79020e)
+
+Origin: upstream, 0.8.7, commit:02a299f5c0321ae807c128812ddaf7d0b71abe1f
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=865413
+---
+ common/flatpak-dir.c | 39 ++++++++++++++++++++++++++++++++++++++-
+ 1 file changed, 38 insertions(+), 1 deletion(-)
+
+diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
+index 4ae1ab1..8c057eb 100644
+--- a/common/flatpak-dir.c
++++ b/common/flatpak-dir.c
+@@ -1306,7 +1306,44 @@ flatpak_dir_ensure_path (FlatpakDir *self,
+ GCancellable *cancellable,
+ GError **error)
+ {
+- return flatpak_mkdir_p (self->basedir, cancellable, error);
++ /* In the system case, we use default perms */
++ if (!self->user)
++ return flatpak_mkdir_p (self->basedir, cancellable, error);
++ else
++ {
++ /* First make the parent */
++ g_autoptr(GFile) parent = g_file_get_parent (self->basedir);
++ if (!flatpak_mkdir_p (parent, cancellable, error))
++ return FALSE;
++ glnx_fd_close int parent_dfd = -1;
++ if (!glnx_opendirat (AT_FDCWD, flatpak_file_get_path_cached (parent), TRUE,
++ &parent_dfd, error))
++ return FALSE;
++ g_autofree char *name = g_file_get_basename (self->basedir);
++ /* Use 0700 in the user case to neuter any suid or world-writable
++ * bits that happen to be in content; see
++ * https://github.com/flatpak/flatpak/pull/837
++ */
++ if (mkdirat (parent_dfd, name, 0700) < 0)
++ {
++ if (errno == EEXIST)
++ {
++ /* And fix up any existing installs that had too-wide perms */
++ struct stat stbuf;
++ if (fstatat (parent_dfd, name, &stbuf, 0) < 0)
++ return flatpak_fail (error, "fstatat");
++ if (stbuf.st_mode & S_IXOTH)
++ {
++ if (fchmodat (parent_dfd, name, 0700, 0) < 0)
++ return flatpak_fail (error, "fchmodat");
++ }
++ }
++ else
++ return flatpak_fail (error, "mkdirat");
++ }
++
++ return TRUE;
++ }
+ }
+
+ /* Warning: This is not threadsafe, don't use in libflatpak */
diff -Nru flatpak-0.8.5/debian/patches/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch flatpak-0.8.5/debian/patches/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch
--- flatpak-0.8.5/debian/patches/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch 1970-01-01 01:00:00.000000000 +0100
+++ flatpak-0.8.5/debian/patches/Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch 2017-06-21 12:05:49.000000000 +0100
@@ -0,0 +1,178 @@
+From: Alexander Larsson <al...@redhat.com>
+Date: Mon, 19 Jun 2017 19:01:08 +0200
+Subject: Ensure we don't install world-writable dirs or setuid files
+
+This is solved in a much nicer way on master, using new ostree
+APIs. However, here we take a brute-force approach of scanning
+all staged files and ensuring we don't have any files or
+directories with invalid modes before committing the transaction.
+
+If any bad permissions were found we delete the entire staging
+directory.
+
+Origin: upstream, 0.8.7, commit:2c8e2417deb6a0d293b15367fcd69080e285da31
+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=865413
+---
+ common/flatpak-dir.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++
+ tests/test-run.sh | 33 ++++++++++++++++++-
+ 2 files changed, 125 insertions(+), 1 deletion(-)
+
+diff --git a/common/flatpak-dir.c b/common/flatpak-dir.c
+index 8c057eb..4dd45c6 100644
+--- a/common/flatpak-dir.c
++++ b/common/flatpak-dir.c
+@@ -2110,6 +2110,96 @@ flatpak_dir_pull_oci (FlatpakDir *self,
+ return TRUE;
+ }
+
++static gboolean
++ensure_safe_objdir (int dir_fd, const char *rel_path, GError **error)
++{
++ g_auto(GLnxDirFdIterator) iter = {0};
++
++ if (!glnx_dirfd_iterator_init_at (dir_fd, rel_path, TRUE, &iter, error))
++ return FALSE;
++
++ while (TRUE)
++ {
++ struct dirent *dent;
++
++ if (!glnx_dirfd_iterator_next_dent_ensure_dtype (&iter, &dent, NULL, error))
++ return FALSE;
++
++ if (dent == NULL)
++ break;
++
++ if (dent->d_type == DT_DIR)
++ {
++ if (!ensure_safe_objdir (iter.fd, dent->d_name, error))
++ return FALSE;
++ }
++ else
++ {
++ struct stat stbuf;
++ if (fstatat (iter.fd, dent->d_name, &stbuf, 0) == 0 &&
++ ((stbuf.st_mode & ~S_IFMT) & ~0775) != 0)
++ return flatpak_fail (error, "Invalid file mode 0%04o", stbuf.st_mode);
++
++ if (g_str_has_suffix (dent->d_name, ".dirmeta"))
++ {
++ glnx_fd_close int dirmeta_fd = -1;
++ g_autoptr(GBytes) data = NULL;
++ g_autoptr(GVariant) variant = NULL;
++ guint32 mode;
++
++ dirmeta_fd = openat (iter.fd, dent->d_name, O_RDONLY | O_CLOEXEC);
++ if (dirmeta_fd < 0)
++ flatpak_fail(error, "Can't read dirmeta");
++
++ data = glnx_fd_readall_bytes (dirmeta_fd, NULL, error);
++ if (!data)
++ return FALSE;
++ variant = g_variant_new_from_bytes (OSTREE_DIRMETA_GVARIANT_FORMAT, data, TRUE);
++ g_variant_ref_sink (variant);
++
++ g_variant_get_child (variant, 2, "u", &mode);
++ mode = GUINT32_FROM_BE (mode);
++ if (((mode & ~S_IFMT) & ~0775) != 0)
++ return flatpak_fail (error, "Invalid directory mode 0%04o", mode);
++ }
++ }
++ }
++
++ return TRUE;
++}
++
++
++static gboolean
++ensure_safe_staging_permissions (OstreeRepo *repo, GError **error)
++{
++ g_auto(GLnxDirFdIterator) tmp_iter = {0};
++
++ /* We don't know which stage dir is in use, so check all */
++
++ if (!glnx_dirfd_iterator_init_at (ostree_repo_get_dfd (repo), "tmp", TRUE, &tmp_iter, error))
++ return FALSE;
++
++ while (TRUE)
++ {
++ struct dirent *dent;
++
++ if (!glnx_dirfd_iterator_next_dent_ensure_dtype (&tmp_iter, &dent, NULL, error))
++ return FALSE;
++
++ if (dent == NULL)
++ break;
++
++ if (dent->d_type == DT_DIR && g_str_has_prefix (dent->d_name, "staging-") &&
++ !ensure_safe_objdir (tmp_iter.fd, dent->d_name, error))
++ {
++ glnx_shutil_rm_rf_at (tmp_iter.fd, dent->d_name, NULL, NULL);
++ return FALSE;
++ }
++ }
++
++ return TRUE;
++}
++
+ gboolean
+ flatpak_dir_pull (FlatpakDir *self,
+ const char *repository,
+@@ -2238,6 +2328,9 @@ flatpak_dir_pull (FlatpakDir *self,
+ goto out;
+
+
++ if (!ensure_safe_staging_permissions (repo, error))
++ goto out;
++
+ if (!ostree_repo_commit_transaction (repo, NULL, cancellable, error))
+ goto out;
+
+diff --git a/tests/test-run.sh b/tests/test-run.sh
+index 0f0d3aa..2b70ff2 100755
+--- a/tests/test-run.sh
++++ b/tests/test-run.sh
+@@ -24,7 +24,7 @@ set -euo pipefail
+ skip_without_bwrap
+ skip_without_user_xattrs
+
+-echo "1..10"
++echo "1..12"
+
+ setup_repo
+ install_repo
+@@ -338,3 +338,34 @@ ${FLATPAK} build-export ${FL_GPGARGS} repos/test ${DIR}
+ ${FLATPAK} ${U} update org.test.OldVersion
+
+ echo "ok version checks"
++
++rm -rf app
++flatpak build-init app org.test.Writable org.test.Platform org.test.Platform
++mkdir -p app/files/a-dir
++chmod a+rwx app/files/a-dir
++flatpak build-finish --command=hello.sh app
++ostree --repo=repos/test commit ${FL_GPGARGS} --branch=app/org.test.Writable/$ARCH/master app
++update_repo
++
++if ${FLATPAK} ${U} install test-repo org.test.Writable &> err.txt; then
++ assert_not_reached "Should not be able to install with world-writable directory"
++fi
++assert_file_has_content err.txt [Ii]nvalid
++
++echo "ok no world writable dir"
++
++rm -rf app
++flatpak build-init app org.test.Setuid org.test.Platform org.test.Platform
++mkdir -p app/files/
++touch app/files/exe
++chmod u+s app/files/exe
++flatpak build-finish --command=hello.sh app
++ostree --repo=repos/test commit ${FL_GPGARGS} --branch=app/org.test.Setuid/$ARCH/master app
++update_repo
++
++if ${FLATPAK} ${U} install test-repo org.test.Setuid &> err2.txt; then
++ assert_not_reached "Should not be able to install with setuid file"
++fi
++assert_file_has_content err2.txt [Ii]nvalid
++
++echo "ok no setuid"
diff -Nru flatpak-0.8.5/debian/patches/series flatpak-0.8.5/debian/patches/series
--- flatpak-0.8.5/debian/patches/series 2017-04-24 12:59:09.000000000 +0100
+++ flatpak-0.8.5/debian/patches/series 2017-06-21 12:05:49.000000000 +0100
@@ -1 +1,3 @@
+dir-Ensure-.local-share-flatpak-is-0700.patch
+Ensure-we-don-t-install-world-writable-dirs-or-setuid-fil.patch
Improve-and-simplify-profile-snippet.patch
