sgid clearing rules?

[Miklos Szeredi]

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,
Miklos