Group inheritance (SGID bit) does not work in -allow_other mode

[slackner]

The issue noticed in fcaca5f#r31889375 reveals a much larger problem.

Two things are important to make group inheritance work correctly:

• The group of any new file, directory, ... should be set correctly
• For directories, the SGID bit should be preserved

The first point never worked due to the explicit Fchown* we execute when PreserveOwner is enabled. The second point sometimes worked, but this was more pure luck than intentional. For directories created without write/execute permission it never worked due to the explicit Fchmod we executed in that case.

Even with PreserveOwner disabled, group inheritance did not always work (e.g., in the case where we execute an explicit Fchmod on a newly created directory).

[slackner]

Something like this would fix part of the issue (UNTESTED!), but we also need additional code for all other file objects. Are you sure we don't want to juts adjust effective permissions? ;)

http://ix.io/1xWO

[rfjakob]

Are you sure we don't want to juts adjust effective permissions? ;)

Not anymore ;)

[slackner]

For comparison, this would be a fix for Create (fixing the mode race condition + fixing all the SGID problems): http://ix.io/1xX9

[slackner]

The approach in my previous draft patch has the disadvantage that it also affects *.name files. While this might seem nice at first (it would be consistent with reverse mode, where *.name files and gocryptfs.diriv files also inherit the uid/gid of the main file/directory), it has the disadvantage of making other operations like Chown, Rename, ... much more complicated. Whenever WriteLongNameAt is used we have to switch to the correct user, even if it was not necessary before.

A slightly different approach based on changing effective permissions is shown in this patch: http://ix.io/1xY2. By adding new syscallcompat wrappers, we can put all the ugly code in a separate file and minimize the region during which we have to lock OSThread. Similar syscall wrappers would be necessary for other functions that depend on the uid/gid.

What are your thoughts? Does using effective permissions make things sufficiently easier, or should we stick to the previous Fchown approach (and handle SGID inheritance manually)?

[rfjakob]

I believe this was fixed by PR #340 . xfstests generic/314 is happy:

$ sudo ./check-gocryptfs generic/314
gocryptfs v1.7-rc1-37-g711ef81; go-fuse v20170619-75-gf520193; 2019-01-13 go1.11.4
fuse-xfstests gocryptfs-2018-08-18/d8111119
Sun Jan 13 13:34:23 UTC 2019

FSTYP         -- fuse.gocryptfs
PLATFORM      -- Linux/x86_64 brikett 4.19.12-301.fc29.x86_64
MKFS_OPTIONS  -- /var/tmp/check-gocryptfs/scratchdev
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /var/tmp/check-gocryptfs/scratchdev /var/tmp/check-gocryptfs/scratchdir

generic/314 6s ...  8s
Ran: generic/314
Passed all 1 tests

Runtime was 15 seconds