on 2022/4/28 17:34, Christian Brauner wrote: > 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. Sorry for the late reply. I am agree with these. So what should I do in next step? ps:I also think I may send test case to xfstests for posix acl and umask ASAP, then xfstests can merge these test and so more people will notice this problem.
