Re: [PATCH v2 3/5] attr: use consistent sgid stripping checks

[Christian Brauner <brauner@kernel.org>]

On Tue, Oct 11, 2022 at 02:07:10PM +0300, Amir Goldstein wrote:
> > > > @@ -721,10 +721,10 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
> > > >                 return -EINVAL;
> > > >         if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
> > > >                 return -EINVAL;
> > > > -       if (!S_ISDIR(inode->i_mode))
> > > > -               newattrs.ia_valid |=
> > > > -                       ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> > > >         inode_lock(inode);
> > > > +       if (!S_ISDIR(inode->i_mode))
> > > > +               newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
> > > > +                                    should_remove_sgid(mnt_userns, inode);
> > >
> > > This is making me stop and wonder:
> > > 1. This has !S_ISDIR, should_remove_suid() has S_ISREG and
> > >     setattr_drop_sgid() has neither - is this consistent?
> >
> > I thought about that. It'very likely redundant since we deal with that
> > in other parts but I need to verify all callers before we can remove
> > that.
> >
> > > 2. SUID and PRIV are removed unconditionally and SGID is
> > >     removed conditionally - this is not a change of behavior
> > >     (at least for non-overlayfs), but is it desired???
> >
> > It looks that way but it isn't. The setgid bit was only killed
> > unconditionally for S_IXGRP. We continue to do that. But it was always
> > removed conditionally for ~S_IXGRP. The difference between this patchset
> > and earlier is that it was done in settattr_prepare() or setattr_copy()
> > before this change.
> >
> > IOW, we raised ATTR_KILL_SGID unconditionally but then only
> > conditionally obeyed it in setattr_{prepare,copy}() whereas now we
> > conditionally raise ATTR_KILL_SGID. That's surely a slight change but it
> > just means that we don't cause bugs for filesystems that roll their own
> > prepare or copy helpers and is just nicer overall.
> >
> 
> Yes, that sounds right.
> 
> The point that I was trying to make and failed to articulate myself was
> that chown_common() raises ATTR_KILL_SUID unconditionally,
> while should_remove_suid() raises ATTR_KILL_SUID conditional
> to !capable(CAP_FSETID).
> 
> Is this inconsistency in stripping SUID desired?

I looked at this before and it likely isn't intentional. But I need to
do pre-git archeology to determine that after I'm back from PTO. It
likely is something we can tackle.

> 
> According to man page (I think that) it is:
> 
> "When the owner or group of an executable file is changed by an
>  unprivileged user, the S_ISUID and S_ISGID mode bits are cleared.
>  POSIX does not specify whether this also  should  happen  when  root
>  does the chown(); the Linux behavior depends on the kernel version,
>  and since Linux 2.2.13, root is treated like other users..."
> 
> So special casing SUID stripping in chown() looks intentional,
> but maybe it is worth a comment.

It definitely is worth a comment but I think instead we should in the
future risk changing this for the write path as well. Because right now
losing the S_ISGID bit during chown() for regular files unconditionally
is important to not accidently have root create a situation where they
open a way for an unprivileged user to escalate privileges when chowning
a non-root owned setuid binary to a root-owned setuid binary:

touch aaa
chown 1000:1000
chmod u+s aaa
sudo chown aaa

and if the setuid bit would be retained then an unpriv user can now
abuse the setuid binary - if they can execute ofc. So that's why it's
dropped unconditionally. However, if that is a valid attack scenario
then a write should also drop setuid unconditionally since a non-harmful
setuid binary could be changed to a harmful one.

> 
> The paragraph above *may* be interpreted that chown() should strip
> S_SGID|S_IXGRP regardless of CAP_FSETID, which, as you say,
> has not been the case for a while.

Yeah, for the setgid bit we've been dropping it implicitly currently.

Thanks!
Christian