On Tue, 22 Nov 2022 at 11:57, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> ---
> 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
