sgid clearing rules?

[Christian Brauner]

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. 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));
 }
 
 static inline int check_sticky(struct user_namespace *mnt_userns,

base-commit: f380367f1811222c3853d942676a451a2353b762
-- 
2.34.1