On Thu, Apr 28, 2022 at 03:46:59AM +0000, Al Viro wrote: > On Thu, Apr 28, 2022 at 03:12:30AM +0000, Al Viro wrote: > > > > Note, BTW, that while XFS has inode_fsuid_set() on the non-inode_init_owner() > > > path, it doesn't have inode_fsgid_set() there. Same goes for ext4, while > > > ext2 doesn't bother with either in such case... > > > > > > Let's try to separate the issues here. Jann, could you explain what makes > > > empty sgid files dangerous? > > > > Found the original thread in old mailbox, and the method of avoiding the > > SGID removal on modification is usable. Which answers the question above... > > OK, what do we want for grpid mounts? Aside of "don't forget inode_fsuid_set(), > please", that is. We don't want inode_fsgid_set() there (whatever went for > the parent directory should be the right value for the child). Same "strip Exactly. You sounded puzzled why we don't call that in an earlier mail. > SGID from non-directory child, unless we are in the resulting group"? Honestly, I think we should try and see if we can't use the same setgid inheritance enforcement of the new mode_strip_sgid() helper for the grpid mount option as well. Iow, just don't give the grpid mount option a separate setgid treatment and try it with the current approach. It'll allow us to move things into vfs proper which I think is a robust solution with clear semantics. It also gives us a uniform ordering wrt to umask stripping and POSIX ACLs. Yes, as we've pointed out in the thread this carries a non-zero regression risk. But so does the whole patch series. But this might end up being a big win security wise and makes maintenance way easier going forward. The current setgid situation is thoroughly messy though and we keep plugging holes. Even writing tests for the current situation is an almost herculean task let alone reviewing it.
