sgid clearing rules?

[Christian Brauner]

On Tue, Nov 22, 2022 at 02:21:59PM +0100, Miklos Szeredi wrote:
> On Tue, 22 Nov 2022 at 11:57, Christian Brauner <brauner at kernel.org> wrote:
> >
> > On Mon, Nov 21, 2022 at 02:14:13PM +0100, Miklos Szeredi wrote:
> > > I'm looking at sgid clearing in case of file modification.  Seems like
> > > the intent was:
> > >
> > >  - if not a regular file, then don't clear
> > >  - else if task has CAP_FSETID in init_user_ns, then don't clear
> >
> > This one is a remnant of the past. The code was simply not updated to
> > reflect the new penultimate rule you mention below. This is fixed in
> > -next based on the VFS work we did (It also includes Amirs patches we
> > reviewed a few weeks ago for file_remove_privs() in ovl.).
> >
> > >  - else if group exec is set, then clear
> > >  - else if gid is in task's group list, then don't clear
> > >  - else if gid and uid are mapped in current namespace and task has
> > > CAP_FSETID in current namespace, then don't clear
> > >  - else clear
> > >
> >
> > The setgid stripping series in -next implements these rules.
> >
> > > However behavior seems to deviate from that if group exec is clear and
> > > *suid* bit is not set.  The reason is that inode_has_no_xattr() will
> > > set S_NOSEC and __file_remove_privs() will bail out before even
> > > starting to interpret the rules.
> >
> > Great observation. The dentry_needs_remove_privs() now calls the new
> > setattr_should_drop_sgid() helper which drops the setgid bit according
> > to the rules above. And yes, we should drop the S_IXGRP check from
> > is_sxid() for consistency.
> > The scenario where things get wonky with the S_IXGRP check present must
> > be when setattr_should_drop_sgid() retains the setgid bit.
> 
> Which is exactly what seems to happen in Test 9 and Test 11 in the
> generic/68[3-7].
> 
> > In that case
> > is_sxid() will mark the inode as not being security relevant even though
> > the setgid bit is still set on it. This dates back to mandatory locking
> > when the setgid bit was used for that. But mandatory locks are out of
> > the door for a while now and this is no longer true and also wasn't
> > enforced consistently for countless years even when they were still
> > there. So we should make this helper consistent with the rest.
> >
> > I will run the patch below through xfstests with
> >
> > -g acl,attr,cap,idmapped,io_uring,perms,unlink
> >
> > which should cover all setgid tests (We've added plenty of new tests to
> > the "perms" group.). Could you please review whether this make sense to you?
> >
> > From cbe6cec88c6cfc66e0fb61f602bb2810c3c48578 Mon Sep 17 00:00:00 2001
> > From: Christian Brauner <brauner at kernel.org>
> > Date: Tue, 22 Nov 2022 11:40:32 +0100
> > Subject: [PATCH] fs: use consistent setgid checks in is_sxid()
> >
> > Now that we made the VFS setgid checking consistent an inode can't be
> > marked security irrelevant even if the setgid bit is still set. Make
> > this function consistent with the other helpers.
> >
> > Reported-by: Miklos Szeredi <miklos at szeredi.hu>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner at kernel.org>
> > ---
> >  include/linux/fs.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index b39c5efca180..d07cadac547e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -3527,7 +3527,7 @@ int __init list_bdev_fs_names(char *buf, size_t size);
> >
> >  static inline bool is_sxid(umode_t mode)
> >  {
> > -       return (mode & S_ISUID) || ((mode & S_ISGID) && (mode & S_IXGRP));
> > +       return (mode & S_ISUID) || ((mode & S_ISGID));
> 
> Yes, this is what I meant.  This can be simplified to:
> 
>        return mode & (S_ISUID | S_ISGID);

Thanks, will update accordingly.

Fwiw, enforcing the rules consistently across write operations and
chmod/chown will lead to losing the setgid bit in cases were it might've
been retained before.

I've mentioned this a few times but it's worth repeating. For the sake
of maintainability, consistency, and security this is a risk worth
taking. I've mentioned this already in the last pull request that fixed
issues in this area so Linus is aware that this is a user visible
change we're risking on purpose. I'll mention this again in detail this
time as well.

If this leads to regressions the fix is easy and we simply need to have
special setgid handling in the write path again with different semantics
from chmod/chown. We'll see. I'll update the tests if this second setgid
cleanup gets merged. Before that just be aware that there might be
failures for tests where unprivileged users modify a setgid file.

Thanks!
Christian