Bugreport on Ubuntu LTS: not ok - 2 Objects creation does not break ACLs with restrictive umask

> so I just pulled the new git v1.7.10.4 and tried to test it with
>> make test
> This yields this output:
> stderr http://pastebin.com/V8yuZFfi
> stdout http://dl.dropbox.com/u/6520164/git/maketest.txt
>
> In Test 1304 there is
> not ok - 2 Objects creation does not break ACLs with restrictive umask
>
> I am running Ubuntu 12.04 with Linux sb 3.2.0-25-generic #40-Ubuntu
> SMP Wed May 23 20:30:51 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux

> I tried debugging into it:
> In git/t/t1304-default-acl.sh there is:
> check_perms_and_acl () {
>     test -r "$1" &&
>     getfacl "$1" > actual &&
>     grep -q "user:root:rwx" actual &&
>     grep -q "user:${LOGNAME}:rwx" actual &&
>     egrep "mask::?r--" actual > /dev/null 2>&1 &&
>     grep -q "group::---" actual || false
> }
>
> but when I do all the commands as in the test2:
> test_expect_success SETFACL 'Objects creation does not break ACLs with
> restrictive umask' '
>     # SHA1 for empty blob
>     check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
> '
> Now I run the second line in check_perms_and_acl ()  with
> .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
> there is
>
> getfacl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
> # file: .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
> # owner: sb
> # group: sb
> user::r--
> user:root:rwx			#effective:---
> user:sb:rwx			#effective:---
> group::---
> mask::---
> other::---
>
> This command seem to fail 'egrep "mask::?r--" actual' as there is
> mask::---
> but expected is
> mask::r--
>
> That's my understanding of the test case so far.

> Stefan Beller <stefanbeller@googlemail.com> writes:
>
>> I tried debugging into it:
>> In git/t/t1304-default-acl.sh there is:
>> check_perms_and_acl () {
>>     test -r "$1" &&
>>     getfacl "$1" > actual &&
>>     grep -q "user:root:rwx" actual &&
>>     grep -q "user:${LOGNAME}:rwx" actual &&
>>     egrep "mask::?r--" actual > /dev/null 2>&1 &&
>>     grep -q "group::---" actual || false
>> }

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <stefanbeller@googlemail.com> writes:
>>
>>> I tried debugging into it:
>>> In git/t/t1304-default-acl.sh there is:
>>> check_perms_and_acl () {
>>>     test -r "$1" &&
>>>     getfacl "$1" > actual &&
>>>     grep -q "user:root:rwx" actual &&
>>>     grep -q "user:${LOGNAME}:rwx" actual &&
>>>     egrep "mask::?r--" actual > /dev/null 2>&1 &&
>>>     grep -q "group::---" actual || false
>>> }
>
> Can you run it with --verbose and post the result?
>
>> Any ideas (other than "Your filesystem is broken",
>> that is)?
>
> I'm very tempted to go for the "Your filesystem is broken" indeed.
>
>> As far as I can tell, with 'mask::---', these specific users who are
>> given permissions to read from the objects wouldn't be able to read
>> from them, so...
>
> That's my understanding too.
>
> Stefan, which filesystem are you using in the directory where you run
> tests (type "mount" if you don't know)? Are you running on a virtual
> machine?
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

> Hi,
>
> sb@sb:~/OSS/git/t$ ./t1304-default-acl.sh --verbose
> Initialized empty Git repository in /home/sb/OSS/git/t/trash
> directory.t1304-default-acl/.git/
> expecting success:
>    setfacl -m d:u::rwx,d:g::---,d:o:---,d:m:rwx $dirs_to_set &&
>    setfacl -m m:rwx               $dirs_to_set &&
>    setfacl -m u:root:rwx          $dirs_to_set &&
>    setfacl -m d:u:"${LOGNAME}":rwx  $dirs_to_set &&
>    setfacl -m d:u:root:rwx        $dirs_to_set &&
>
>    touch file.txt &&
>    git add file.txt &&
>    git commit -m "init"
>
> [master (root-commit) 47f54f4] init
>  Author: A U Thor <author@example.com>
>  0 files changed
>  create mode 100644 file.txt
> ok 1 - Setup test repo
>
> expecting success:
>    # SHA1 for empty blob
>    check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
>
> /home/sb/OSS/git/t/trash directory.t1304-default-acl
> not ok - 2 Objects creation does not break ACLs with restrictive umask
> #
> #           # SHA1 for empty blob
> #           check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
> #
>
> expecting success:
>    git gc &&
>    check_perms_and_acl .git/objects/pack/*.pack
>
> Counting objects: 3, done.
> Writing objects: 100% (3/3), done.
> Total 3 (delta 0), reused 0 (delta 0)
> /home/sb/OSS/git/t/trash directory.t1304-default-acl
> not ok - 3 git gc does not break ACLs with restrictive umask
> #
> #           git gc &&
> #           check_perms_and_acl .git/objects/pack/*.pack
> #
>
> # failed 2 among 3 test(s)
> 1..3
> sb@sb:~/OSS/git/t$ cat trash\ directory.t1304-default-acl/actual
> # file: .git/objects/pack/pack-ee77696bcc9be7ef581005ee3706bc17fcba376d.pack
> # owner: sb
> # group: sb
> user::r--
> user:root:rwx   #effective:---
> user:sb:rwx     #effective:---
> group::---
> mask::---
> other::---
>
> sb@sb:~/OSS/git/t$ mount
> /dev/sda3 on / type ext4 (rw,errors=remount-ro)
> proc on /proc type proc (rw,noexec,nosuid,nodev)
> sysfs on /sys type sysfs (rw,noexec,nosuid,nodev)
> none on /sys/fs/fuse/connections type fusectl (rw)
> none on /sys/kernel/debug type debugfs (rw)
> none on /sys/kernel/security type securityfs (rw)
> udev on /dev type devtmpfs (rw,mode=0755)
> devpts on /dev/pts type devpts (rw,noexec,nosuid,gid=5,mode=0620)
> tmpfs on /run type tmpfs (rw,noexec,nosuid,size=10%,mode=0755)
> none on /run/lock type tmpfs (rw,noexec,nosuid,nodev,size=5242880)
> none on /run/shm type tmpfs (rw,nosuid,nodev)
> /dev/sda4 on /home type ext4 (rw)
> binfmt_misc on /proc/sys/fs/binfmt_misc type binfmt_misc
> (rw,noexec,nosuid,nodev)
> /home/sb/.Private on /home/sb type ecryptfs
> (ecryptfs_check_dev_ruid,ecryptfs_cipher=aes,ecryptfs_key_bytes=16,ecryptfs_unlink_sigs,ecryptfs_sig=3b9d213e5c5d5780,ecryptfs_fnek_sig=77bec2da523c0338)
> gvfs-fuse-daemon on /home/sb/.gvfs type fuse.gvfs-fuse-daemon
> (rw,nosuid,nodev,user=sb)
>
>
> So I am using /dev/sda4 on /home type ext4 (rw), but my user account
> has its home directory encrypted via ecryptfs.
> This is not run inside a virtual machine, it's a native computer.
>
>
>
> 2012/6/5 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Stefan Beller <stefanbeller@googlemail.com> writes:
>>>
>>>> I tried debugging into it:
>>>> In git/t/t1304-default-acl.sh there is:
>>>> check_perms_and_acl () {
>>>>     test -r "$1" &&
>>>>     getfacl "$1" > actual &&
>>>>     grep -q "user:root:rwx" actual &&
>>>>     grep -q "user:${LOGNAME}:rwx" actual &&
>>>>     egrep "mask::?r--" actual > /dev/null 2>&1 &&
>>>>     grep -q "group::---" actual || false
>>>> }
>>
>> Can you run it with --verbose and post the result?
>>
>>> Any ideas (other than "Your filesystem is broken",
>>> that is)?
>>
>> I'm very tempted to go for the "Your filesystem is broken" indeed.
>>
>>> As far as I can tell, with 'mask::---', these specific users who are
>>> given permissions to read from the objects wouldn't be able to read
>>> from them, so...
>>
>> That's my understanding too.
>>
>> Stefan, which filesystem are you using in the directory where you run
>> tests (type "mount" if you don't know)? Are you running on a virtual
>> machine?
>>
>> --
>> Matthieu Moy
>> http://www-verimag.imag.fr/~moy/

> +test_expect_success 'checking for a working acl setup' '
> +	if setfacl -m u:root:rwx . &&
> +	   getfacl . | grep user:root:rwx
> +	then
> +		test_set_prereq SETFACL
> +	fi
> +'

> Jeff King <peff@peff.net> writes:
>
>> Subject: t1304: improve setfacl prerequisite setup
>
>> +test_expect_success 'checking for a working acl setup' '
>> +     if setfacl -m u:root:rwx . &&
>> +        getfacl . | grep user:root:rwx
>> +     then
>> +             test_set_prereq SETFACL
>> +     fi
>> +'
>
> Excellent. Thanks.
>
> Tested-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
> > Hi,
> >
> > so I applied that patch with git apply, but here 2 out of 4 tests now
> > still fail.
> 
> Yes, Jeff's patch checks for a "working" setfacl/getfacl, but does not
> check for subtle brokenness of it.
> 
> I'd say a filesystem that accepts setfacl, returns the right line in
> getfacl, and then does not really work is broken, and it's a feature
> that the testsuite shows test failures. A simple fix for you would be to
> disable ACLs, either on your system or in your $HOME.

>   # and create some files with different permissions
>   perl -MFcntl -e '
>     sysopen(X, "a", O_WRONLY|O_CREAT, 0666);
>     sysopen(X, "b", O_WRONLY|O_CREAT, 0600);
>   '
> 
>   # and then check the permissions
>   getfacl a b
> 
> which yields:
> 
>   # file: a
>   # owner: peff
>   # group: peff
>   user::rw-
>   group::---
>   mask::r--
>   other::---
> 
>   # file: b
>   # owner: peff
>   # group: peff
>   user::rw-
>   group::---
>   mask::---
>   other::---
> 
> That just seems wrong to me.

> On Tue, Jun 05, 2012 at 10:04:49AM -0400, Jeff King wrote:
> 
> >   # and create some files with different permissions
> >   perl -MFcntl -e '
> >     sysopen(X, "a", O_WRONLY|O_CREAT, 0666);
> >     sysopen(X, "b", O_WRONLY|O_CREAT, 0600);
> >   '
> Hmm, no I'm wrong. That is OK, as the mask should be restricted by the
> group permission (and it is on ext4, too). But when I traced through
> git, the tmp_obj_* files are created with the right permissions on ext4,
> but not on ecryptfs. Let me trace through it again...<sigh>

> OK, here's the _real_ issue. Git creates with mode 0444, which should
> still allow read in the mask. But it's the restrictive umask at the top
> of the test script that causes the problem. Try this:
> 
>   setfacl -m m:rwx .
>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
>   umask 077
>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
>   getfacl a b
> 
> On ext4, both files will have the read bit set in the mask. On ecryptfs,
> "b" will have an empty mask. I think the wrong thing is that we should
> not be respecting umask at all when default ACLs are in play, and
> ecryptfs is getting that wrong. But I'm having trouble digging up an
> authoritative source.

> [+cc ecryptfs@vger, as I think this is an ecryptfs bug]
>
> On Tue, Jun 05, 2012 at 10:28:13AM -0400, Jeff King wrote:
>
>> OK, here's the _real_ issue. Git creates with mode 0444, which should
>> still allow read in the mask. But it's the restrictive umask at the top
>> of the test script that causes the problem. Try this:
>> 
>>   setfacl -m m:rwx .
>>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
>>   umask 077
>>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
>>   getfacl a b
>> 
>> On ext4, both files will have the read bit set in the mask. On ecryptfs,
>> "b" will have an empty mask. I think the wrong thing is that we should
>> not be respecting umask at all when default ACLs are in play, and
>> ecryptfs is getting that wrong. But I'm having trouble digging up an
>> authoritative source.
>
> Reading the withdrawn posix 1003.1e and "man 5 acl", it seems pretty
> clear that if a default ACL is present, it should be used, and umask
> consulted only if it is not (so the umask should not be making a
> difference in this case).
>
> The reproduction recipe above shows the minimum required to trigger it;
> adding a more realistic default ACL (with actual entries for users) does
> not seem to make a difference.

> >>   setfacl -m m:rwx .
> >>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
> >>   umask 077
> >>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
> >>   getfacl a b
> [...]
> >
> > Reading the withdrawn posix 1003.1e and "man 5 acl", it seems pretty
> > clear that if a default ACL is present, it should be used, and umask
> > consulted only if it is not (so the umask should not be making a
> > difference in this case).
> >
> > The reproduction recipe above shows the minimum required to trigger it;
> > adding a more realistic default ACL (with actual entries for users) does
> > not seem to make a difference.
> 
> Thanks; so combining the above with your earlier patch to 1304 we
> would have a good detection for SETFACL prerequisite?

> On Tue, Jun 05, 2012 at 09:31:54AM -0700, Junio C Hamano wrote:
>
>> >>   setfacl -m m:rwx .
>> >>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
>> >>   umask 077
>> >>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
>> >>   getfacl a b
>> [...]
>> >
>> > Reading the withdrawn posix 1003.1e and "man 5 acl", it seems pretty
>> > clear that if a default ACL is present, it should be used, and umask
>> > consulted only if it is not (so the umask should not be making a
>> > difference in this case).
>> >
>> > The reproduction recipe above shows the minimum required to trigger it;
>> > adding a more realistic default ACL (with actual entries for users) does
>> > not seem to make a difference.
>> 
>> Thanks; so combining the above with your earlier patch to 1304 we
>> would have a good detection for SETFACL prerequisite?
>
> Yes, I think we can detect it reliably. I'd like to hear back from
> ecryptfs folks before making a final patch, though. It may be that there
> is some subtle reason for their behavior, and I want to make sure before
> we write it off as just buggy.

> t1304 first runs setfacl as an experiment to see whether the
> filesystem supports ACLs, and skips the remaining tests if
> it does not. However, our setfacl run did not exercise the
> ACLs very well, and some filesystems may support our initial
> setfacl, but not the rest of the test.
>
> In particular, some versions of ecryptfs will erroneously
> apply the umask on top of an inherited directory ACL,
> causing our tests to fail. Let's be more careful and make
> sure both that we can read back the user ACL we set, and
> that the inherited ACL is propagated correctly. The latter
> catches the ecryptfs bug, but may also catch other bugs
> (e.g., an implementation which does not handle inherited
> ACLs at all).
>
> Since we're making the setup more complex, let's move it
> into its own test. This will hide the output for us unless
> the user wants to run "-v" to see it (and we don't need to
> bother printing anything about setfacl failing; the
> remaining tests will properly print "skip" due to the
> missing prerequisite).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The ecryptfs response was that it is probably a bug, so I think we
> should go with this (especially because it is general enough to
> potentially catch other weird errors).
>
>  t/t1304-default-acl.sh | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
> index 2b962cf..79045ab 100755
> --- a/t/t1304-default-acl.sh
> +++ b/t/t1304-default-acl.sh
> @@ -14,16 +14,15 @@ umask 077
>  # We need an arbitrary other user give permission to using ACLs. root
>  # is a good candidate: exists on all unices, and it has permission
>  # anyway, so we don't create a security hole running the testsuite.
> -
> -setfacl_out="$(setfacl -m u:root:rwx . 2>&1)"
> -setfacl_ret=$?
> -
> -if test $setfacl_ret != 0
> -then
> -       say "Unable to use setfacl (output: '$setfacl_out'; return code: '$setfacl_ret')"
> -else
> -       test_set_prereq SETFACL
> -fi
> +test_expect_success 'checking for a working acl setup' '
> +       if setfacl -m d:m:rwx -m u:root:rwx . &&
> +          getfacl . | grep user:root:rwx &&
> +          touch should-have-readable-acl &&
> +          getfacl should-have-readable-acl | egrep "mask::?rw-"
> +       then
> +               test_set_prereq SETFACL
> +       fi
> +'
>
>  if test -z "$LOGNAME"
>  then
> --
> 1.7.11.rc1.4.g0d3b9b3
>

> t1304 first runs setfacl as an experiment to see whether the
> filesystem supports ACLs, and skips the remaining tests if
> it does not. However, our setfacl run did not exercise the
> ACLs very well, and some filesystems may support our initial
> setfacl, but not the rest of the test.
>
> In particular, some versions of ecryptfs will erroneously
> apply the umask on top of an inherited directory ACL,
> causing our tests to fail. Let's be more careful and make
> sure both that we can read back the user ACL we set, and
> that the inherited ACL is propagated correctly. The latter
> catches the ecryptfs bug, but may also catch other bugs
> (e.g., an implementation which does not handle inherited
> ACLs at all).
>
> Since we're making the setup more complex, let's move it
> into its own test. This will hide the output for us unless
> the user wants to run "-v" to see it (and we don't need to
> bother printing anything about setfacl failing; the
> remaining tests will properly print "skip" due to the
> missing prerequisite).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The ecryptfs response was that it is probably a bug, so I think we
> should go with this (especially because it is general enough to
> potentially catch other weird errors).
>
>  t/t1304-default-acl.sh | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
> index 2b962cf..79045ab 100755
> --- a/t/t1304-default-acl.sh
> +++ b/t/t1304-default-acl.sh
> @@ -14,16 +14,15 @@ umask 077
>  # We need an arbitrary other user give permission to using ACLs. root
>  # is a good candidate: exists on all unices, and it has permission
>  # anyway, so we don't create a security hole running the testsuite.
> -
> -setfacl_out="$(setfacl -m u:root:rwx . 2>&1)"
> -setfacl_ret=$?
> -
> -if test $setfacl_ret != 0
> -then
> -	say "Unable to use setfacl (output: '$setfacl_out'; return code: '$setfacl_ret')"
> -else
> -	test_set_prereq SETFACL
> -fi
> +test_expect_success 'checking for a working acl setup' '
> +	if setfacl -m d:m:rwx -m u:root:rwx . &&
> +	   getfacl . | grep user:root:rwx &&
> +	   touch should-have-readable-acl &&
> +	   getfacl should-have-readable-acl | egrep "mask::?rw-"

> +	then
> +		test_set_prereq SETFACL
> +	fi
> +'
>  
>  if test -z "$LOGNAME"
>  then

> > +test_expect_success 'checking for a working acl setup' '
> > +	if setfacl -m d:m:rwx -m u:root:rwx . &&
> > +	   getfacl . | grep user:root:rwx &&
> > +	   touch should-have-readable-acl &&
> > +	   getfacl should-have-readable-acl | egrep "mask::?rw-"
> 
> At this point of the test, I do not think there is anything that can
> cause this check to be fooled by a user whose name is 'mask', but
> please make it a habit to anchor the pattern at the left.  The tests
> done in check_perms_and_acl do risk getting fooled by a user whose
> name is 'mask', and need to be updated, I think, but that is a
> separate issue.

> Jeff King <peff@peff.net> writes:
>
>> I intentionally did not anchor it because the tests themselves did not,
>> and I worried that that there was some subtle portability issue there
>> (especially because of the magic "let there be two colons" part of the
>> pattern).
>
> OK, fair enough (the "two colons" one I think was for Solaris).

> On Tue, Jun 05, 2012 at 09:31:54AM -0700, Junio C Hamano wrote:
> 
> > >>   setfacl -m m:rwx .
> > >>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
> > >>   umask 077
> > >>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
> > >>   getfacl a b
> > [...]
> > >
> > > Reading the withdrawn posix 1003.1e and "man 5 acl", it seems pretty
> > > clear that if a default ACL is present, it should be used, and umask
> > > consulted only if it is not (so the umask should not be making a
> > > difference in this case).
> > >
> > > The reproduction recipe above shows the minimum required to trigger it;
> > > adding a more realistic default ACL (with actual entries for users) does
> > > not seem to make a difference.
> > 
> > Thanks; so combining the above with your earlier patch to 1304 we
> > would have a good detection for SETFACL prerequisite?
> 
> Yes, I think we can detect it reliably. I'd like to hear back from
> ecryptfs folks before making a final patch, though. It may be that there
> is some subtle reason for their behavior, and I want to make sure before
> we write it off as just buggy.

> On 2012-06-05 12:44:39, Jeff King wrote:
>> On Tue, Jun 05, 2012 at 09:31:54AM -0700, Junio C Hamano wrote:
>>
>> > >>   setfacl -m m:rwx .
>> > >>   perl -MFcntl -e 'sysopen(X, "a", O_WRONLY|O_CREAT, 0444)'
>> > >>   umask 077
>> > >>   perl -MFcntl -e 'sysopen(X, "b", O_WRONLY|O_CREAT, 0444)'
>> > >>   getfacl a b
>> > [...]
>> > >
>> > > Reading the withdrawn posix 1003.1e and "man 5 acl", it seems pretty
>> > > clear that if a default ACL is present, it should be used, and umask
>> > > consulted only if it is not (so the umask should not be making a
>> > > difference in this case).
>> > >
>> > > The reproduction recipe above shows the minimum required to trigger it;
>> > > adding a more realistic default ACL (with actual entries for users) does
>> > > not seem to make a difference.
>> >
>> > Thanks; so combining the above with your earlier patch to 1304 we
>> > would have a good detection for SETFACL prerequisite?
>>
>> Yes, I think we can detect it reliably. I'd like to hear back from
>> ecryptfs folks before making a final patch, though. It may be that there
>> is some subtle reason for their behavior, and I want to make sure before
>> we write it off as just buggy.
>
> It is likely a bug in the eCryptfs filesystem stacking code.
>
> However, using the above script, I get the same results on eCryptfs as I
> do on ext4 in the Ubuntu 12.04 (Precise) LTS:
>
> # file: a
> # owner: tyhicks
> # group: tyhicks
> user::r--
> group::r--
> other::r--
>
> # file: b
> # owner: tyhicks
> # group: tyhicks
> user::r--
> group::---
> other::---
>
> Stefan - can you specify which LTS release you're running as well as the
> output of `cat /proc/version_signature`? Thanks!
>
> Tyler