From 5913b2294490a1f85d4b617fdcc9455ed0bc63a2 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 5 Jun 2017 11:32:52 -0400
Subject: [PATCH] lib/repo: For bare-user, mask content object modes with 0775

Having every object in a bare-user repo (and checkouts) be executable
is ugly.  I can't think of a good reason to do that; they should only
be executable if their input is.  This does
for `bare-user` what we did for `bare-user-only` in
https://github.com/ostreedev/ostree/pull/909
It's also a stronger version of what we do with `checkout -U` in suppressing
suid - here we also strip world-writable files and the sticky bit (even though
that's meaningless today, it might not be in the future).

Closes: https://github.com/ostreedev/ostree/issues/907

Closes: #908
Approved by: alexlarsson
---
 src/libostree/ostree-repo-commit.c | 22 ++++++++---------
 tests/basic-test.sh                |  2 +-
 tests/libtest.sh                   | 18 ++++++++++++++
 tests/test-basic-user.sh           | 38 ++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index 3ecea29d..da0a5cdc 100644
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -283,19 +283,19 @@ commit_loose_object_trusted (OstreeRepo        *self,
       if (objtype == OSTREE_OBJECT_TYPE_FILE &&
           self->mode == OSTREE_REPO_MODE_BARE_USER)
         {
-          if (!object_is_symlink)
-            {
-              /* We need to apply at least some mode bits, because the repo file was created
-                 with mode 644, and we need e.g. exec bits to be right when we do a user-mode
-                 checkout. To make this work we apply all user bits and the read bits for
-                 group/other.  Furthermore, setting user xattrs requires write access, so
-                 this makes sure it's at least writable by us.  (O_TMPFILE uses mode 0 by default) */
-              if (fchmod (fd, mode | 0744) < 0)
-                return glnx_throw_errno (error);
-            }
-
           if (!write_file_metadata_to_xattr (fd, uid, gid, mode, xattrs, error))
             return FALSE;
+
+          if (!object_is_symlink)
+            {
+              /* Note that previously this path added `| 0755` which made every
+               * file executable, see
+               * https://github.com/ostreedev/ostree/issues/907
+               */
+              const mode_t content_mode = (mode & (S_IFREG | 0775));
+              if (fchmod (fd, content_mode) < 0)
+                return glnx_throw_errno_prefix (error, "fchmod");
+            }
         }
       else if (objtype == OSTREE_OBJECT_TYPE_FILE &&
                self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY
diff --git a/tests/basic-test.sh b/tests/basic-test.sh
index f5af93b1..e3e442e9 100644
--- a/tests/basic-test.sh
+++ b/tests/basic-test.sh
@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..66"
+echo "1..$((66 + ${extra_basic_tests:-0}))"
 
 $CMD_PREFIX ostree --version > version.yaml
 python -c 'import yaml; yaml.safe_load(open("version.yaml"))'
diff --git a/tests/libtest.sh b/tests/libtest.sh
index f1fba4fc..1774a7b6 100755
--- a/tests/libtest.sh
+++ b/tests/libtest.sh
@@ -503,3 +503,21 @@ libtest_cleanup_gpg () {
 is_bare_user_only_repo () {
   grep -q 'mode=bare-user-only' $1/config
 }
+
+# Given a path to a file in a repo for a ref, print its checksum
+ostree_file_path_to_checksum() {
+    repo=$1
+    ref=$2
+    path=$3
+    $CMD_PREFIX ostree --repo=$repo ls -C $ref $path | awk '{ print $5 }'
+}
+
+# Given a path to a file in a repo for a ref, print the path to its object
+ostree_file_path_to_object_path() {
+   repo=$1
+   ref=$2
+   path=$3
+   checksum=$(ostree_file_path_to_checksum $repo $ref $path)
+   test -n "${checksum}"
+   echo ${repo}/objects/${checksum:0:2}/${checksum:2}.file
+}
diff --git a/tests/test-basic-user.sh b/tests/test-basic-user.sh
index 3e11545e..fa802df6 100755
--- a/tests/test-basic-user.sh
+++ b/tests/test-basic-user.sh
@@ -25,4 +25,42 @@ skip_without_user_xattrs
 
 setup_test_repository "bare-user"
 
+extra_basic_tests=3
 . $(dirname $0)/basic-test.sh
+
+# Reset things so we don't inherit a lot of state from earlier tests
+rm repo files -rf
+setup_test_repository "bare-user"
+
+cd ${test_tmpdir}
+objpath_nonexec=$(ostree_file_path_to_object_path repo test2 baz/cow)
+# Sigh, umask
+touch testfile
+default_mode=$(stat -c '%a' testfile)
+rm testfile
+assert_file_has_mode ${objpath_nonexec} ${default_mode}
+objpath_ro=$(ostree_file_path_to_object_path repo test2 baz/cowro)
+assert_file_has_mode ${objpath_ro} 600
+objpath_exec=$(ostree_file_path_to_object_path repo test2 baz/deeper/ohyeahx)
+assert_file_has_mode ${objpath_exec} 755
+echo "ok bare-user committed modes"
+
+rm test2-checkout -rf
+$OSTREE checkout -U -H test2 test2-checkout
+cd test2-checkout
+assert_file_has_mode baz/cow ${default_mode}
+assert_file_has_mode baz/cowro 600
+assert_file_has_mode baz/deeper/ohyeahx 755
+echo "ok bare-user checkout modes"
+
+rm test2-checkout -rf
+$OSTREE checkout -U -H test2 test2-checkout
+touch test2-checkout/unwritable
+chmod 0400 test2-checkout/unwritable
+$OSTREE commit -b test2-unwritable --tree=dir=test2-checkout
+chmod 0600 test2-checkout/unwritable
+rm test2-checkout -rf
+$OSTREE checkout -U -H test2-unwritable test2-checkout
+cd test2-checkout
+assert_file_has_mode unwritable 400
+echo "ok bare-user unwritable"