bare-user files are always executable (0755 perms)

[gtristan]

With bare repository this is not an issue, but with bare-user and user mode checkouts, my expectation is that permissions are not munged, only file ownership is left to the active user.

This test case exhibits some strange behavior:

#!/bin/bash

# Test case demonstrating files becoming executable out of thin air
#
# This seems to occur with bare-user repository and checking out in user mode
#
# Usage:
#
#  ./test.sh <work directory>
#
#
set -e
set -x

topdir=${1}
mkdir -p "${topdir}"
topdir="$(cd ${topdir} && pwd)"

repo="${topdir}/repo"
checkin="${topdir}/checkin"
checkout="${topdir}/checkout"

#
# Create test data with some permissions
#
mkdir -p "${checkin}/subdir"
echo "foo" > "${checkin}/foo"
echo "bar" > "${checkin}/bar"
chmod 0600 "${checkin}/foo"
chmod 0755 "${checkin}/bar"
echo "foo" > "${checkin}/subdir/foo"
echo "bar" > "${checkin}/subdir/bar"
chmod 0600 "${checkin}/subdir/foo"
chmod 0755 "${checkin}/subdir/bar"
touch "${checkin}/emptyfile"

#
# Init repo with bare-user
#
ostree init --repo "${repo}" --mode bare-user

#
# Create commit
#
ostree commit --repo "${repo}" --branch "test" -s "test commit" "${checkin}"

#
# Checkout commit, just pass in --disable-cache here because it's default
# when using the API (but should not make a difference, here we are not
# using a compressed repo anyway).
#
ostree checkout --repo "${repo}" --user-mode --disable-cache "test" "${checkout}"

[cgwalters]

Some discussion on list about this https://mail.gnome.org/archives/ostree-list/2017-June/msg00011.html

Honestly, I don't know why we use 0755 always. Looking at a patch.

[cgwalters]

PR in #908 - does that make sense to you?

[gtristan]

Well, sort of :)

First, yes now that I have understood a bit more the purpose of "bare-user" mode is for users to run software in these checkouts inside containers, it makes sense for this use case to strip the suid bits, and certainly this part was previously missing for hardlink user mode checkouts of "bare-user" repositories.

Second, I'm not sure it's necessary to force read-write for user onto the mode bits. Probably there are two opposing things to consider here:

• Why would any commit originally have files without these bits ? Surely if they did, there is a reason, or it just never happens.
• The rationale of forcing these bits may be that we are worried that this may cause a regression to containers which previously expected these bits to be forced, but can probably live with only forcing read-write for owning user ?

Anyway, seems to me that the chance of these bits not already being present at commit time are astronomically low, on my system for example:

# sudo find / ! -perm /u+wr
/dev/pts/ptmx
/run/crond.reboot
/run/systemd/inaccessible
/run/systemd/inaccessible/sock
/run/systemd/inaccessible/fifo
/run/systemd/inaccessible/blk
/run/systemd/inaccessible/chr
/run/systemd/inaccessible/dir
/run/systemd/inaccessible/reg

Not sure what these files are for, also not sure what it buys us to force them to be read-write.

[gtristan]

Note: Another more appropriate invocation of find (find any files which either do not have S_IRUSR or do not have S_IWUSR) sudo find / ! -perm /u+w -o ! -perm /u+r yielded more interesting results.

On my system for instance, there is a huge amount of results from git repositories which store the objects as read-only even for the regular user, since those are never expected to change this seems like a reasonable mode.

[gtristan]

As I am a bit worried about the SUID situation and how I will be able to create filesystem images which do contain setuid binaries, from inside bubblewrap sandboxes based on user mode checkouts that do not contain setuid binaries (which seems a problem I wont be able to escape without horrible hacks like yocto's pseudo or debian's fakeroot)...

I would like to ask that this line in #908:

const mode_t content_mode = (mode & ~(S_ISUID|S_ISGID)) | S_IRUSR | S_IWUSR;

At least be changed to simply be:

const mode_t content_mode = (mode & ~(S_ISUID|S_ISGID));

If the concern is only about setuid & setgid binaries being owned by the checking out user (which is a valid concern, but something I think a user might want to live with for some use cases), then I dont see why we should further be forcing read/write permissions on these files.

[cgwalters]

Ignore /run - it's a dynamic tmpfs, so one wouldn't commit it to ostree.

I think the notion I had of requiring writablity came from directories; see this Red Hat bugzilla.

Oh wait, the rationale for user-writable is in the comment - we need it to write xattrs. However,
But you're clearly right, there's no reason to force writability. And I guess not readability either. We just need to special case the non-writable-by-user by doing the fchmod() twice. Updating the patch.