1. Patchew
  2. linux
  3. View series

[PATCH v3] add group restriction bitmap

Stas Sergeev posted 1 patch 1 month, 4 weeks ago
There is a newer version of this series
fs/namei.c                 | 15 +++++++++++++--
include/linux/cred.h       |  5 +++++
include/uapi/linux/prctl.h |  4 ++++
kernel/groups.c            | 23 ++++++++++++++++++-----
kernel/sys.c               | 18 ++++++++++++++++++
5 files changed, 58 insertions(+), 7 deletions(-)
[PATCH v3] add group restriction bitmap
Posted by Stas Sergeev 1 month, 4 weeks ago 
This patch adds the group restriction bitmap.
This bitmap is normally 0 (all bits clear), which means the normal
handling of the group permission check. When either bit is set, the
corresponding entry in supplementary group list is treated differently:
- if group access denied, then deny, as before
- if group access allowed, then proceed to checking Other perms.

Added 3 prctl calls: PR_GET_GRBITMAP, PR_SET_GRBITMAP and PR_CLR_GRBITMAP
to manipulate the bitmap. This implementation only allows to manipulate
31 bits. PR_CLR_GRBITMAP needs CAP_SETGID, meaning that the user can
only set the restriction bits but never clear (unless capable).

Q: Why is this needed?
A: When you want to lower the privs of your process, you may use
suid/sgid bits to switch to some home-less (no home dir) unprivileged
user that can't touch any files of the original user. But the
supplementary group list ruins that possibility, and you can't drop it.

The ability to drop the group list was proposed by Josh Tripplett:
https://lore.kernel.org/all/0895c1f268bc0b01cc6c8ed4607d7c3953f49728.1416041823.git.josh@joshtriplett.org/

But it wasn't considered secure enough because the group may restrict
an access, not only allow. My solution avoids that problem, as when you
set a bit in the restriction bitmap, the group restriction still
applies - only the permission is withdrawn. Another advantage is that
you can selectively restrict groups from the list, rather than to drop
them all at once.

Changes in v3: add may_setgroups() for !CONFIG_MULTIUSER
  (fixes test bot problem)
Changes in v2: add PR_CLR_GRBITMAP and make the bits otherwise unclearable.

Signed-off-by: Stas Sergeev <stsp2@yandex.ru>

CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jens Axboe <axboe@kernel.dk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Florent Revest <revest@chromium.org>
CC: Kees Cook <kees@kernel.org>
CC: Palmer Dabbelt <palmer@rivosinc.com>
CC: Charlie Jenkins <charlie@rivosinc.com>
CC: Benjamin Gray <bgray@linux.ibm.com>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Helge Deller <deller@gmx.de>
CC: Zev Weiss <zev@bewilderbeest.net> (commit_signer:1/12=8%)
CC: Samuel Holland <samuel.holland@sifive.com>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Eric Biederman <ebiederm@xmission.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Josh Triplett <josh@joshtriplett.org>
---
 fs/namei.c                 | 15 +++++++++++++--
 include/linux/cred.h       |  5 +++++
 include/uapi/linux/prctl.h |  4 ++++
 kernel/groups.c            | 23 ++++++++++++++++++-----
 kernel/sys.c               | 18 ++++++++++++++++++
 5 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 4a4a22a08ac2..44f5571d8f2c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -373,8 +373,19 @@ static int acl_permission_check(struct mnt_idmap *idmap,
 	 */
 	if (mask & (mode ^ (mode >> 3))) {
 		vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
-		if (vfsgid_in_group_p(vfsgid))
-			mode >>= 3;
+		int idx = vfsgid_in_group_p(vfsgid);
+
+		if (idx) {
+			unsigned int mode_grp = mode >> 3;
+
+			if (mask & ~mode_grp)
+				return -EACCES;
+			idx -= 2;
+			if (idx < 0 || idx >= 32 || !((1U << idx) &
+					current_cred()->group_info->restrict_bitmap))
+				return 0;
+			/* If we hit restrict_bitmap, then check Others. */
+		}
 	}
 
 	/* Bits in 'mode' clear that we require? */
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 2976f534a7a3..97fc0a2105dc 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -25,6 +25,7 @@ struct inode;
  */
 struct group_info {
 	refcount_t	usage;
+	unsigned int	restrict_bitmap;
 	int		ngroups;
 	kgid_t		gid[];
 } __randomize_layout;
@@ -83,6 +84,10 @@ static inline int groups_search(const struct group_info *group_info, kgid_t grp)
 {
 	return 1;
 }
+static inline bool may_setgroups(void)
+{
+	return 1;
+}
 #endif
 
 /*
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 35791791a879..2a9f3e0c9845 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -328,4 +328,8 @@ struct prctl_mm_map {
 # define PR_PPC_DEXCR_CTRL_CLEAR_ONEXEC	0x10 /* Clear the aspect on exec */
 # define PR_PPC_DEXCR_CTRL_MASK		0x1f
 
+#define PR_GET_GRBITMAP			74
+#define PR_SET_GRBITMAP			75
+#define PR_CLR_GRBITMAP			76
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/groups.c b/kernel/groups.c
index 9b43da22647d..b7dfd96826e5 100644
--- a/kernel/groups.c
+++ b/kernel/groups.c
@@ -20,6 +20,7 @@ struct group_info *groups_alloc(int gidsetsize)
 		return NULL;
 
 	refcount_set(&gi->usage, 1);
+	gi->restrict_bitmap = 0;
 	gi->ngroups = gidsetsize;
 	return gi;
 }
@@ -88,7 +89,9 @@ void groups_sort(struct group_info *group_info)
 }
 EXPORT_SYMBOL(groups_sort);
 
-/* a simple bsearch */
+/* a simple bsearch
+ * Return: 1-based index of the matched entry, or 0 if not found,
+ */
 int groups_search(const struct group_info *group_info, kgid_t grp)
 {
 	unsigned int left, right;
@@ -105,7 +108,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
 		else if (gid_lt(grp, group_info->gid[mid]))
 			right = mid;
 		else
-			return 1;
+			return mid + 1;
 	}
 	return 0;
 }
@@ -222,15 +225,21 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist)
 }
 
 /*
- * Check whether we're fsgid/egid or in the supplemental group..
+ * Check whether we're fsgid/egid or in the supplemental group.
+ * Return: 1-based index of the matched entry, where 1 means fsgid,
+ * 2..N means 2-based index in group_info.
  */
 int in_group_p(kgid_t grp)
 {
 	const struct cred *cred = current_cred();
 	int retval = 1;
 
-	if (!gid_eq(grp, cred->fsgid))
+	if (!gid_eq(grp, cred->fsgid)) {
 		retval = groups_search(cred->group_info, grp);
+		/* Make it start from 2. */
+		if (retval)
+			retval++;
+	}
 	return retval;
 }
 
@@ -241,8 +250,12 @@ int in_egroup_p(kgid_t grp)
 	const struct cred *cred = current_cred();
 	int retval = 1;
 
-	if (!gid_eq(grp, cred->egid))
+	if (!gid_eq(grp, cred->egid)) {
 		retval = groups_search(cred->group_info, grp);
+		/* Make it start from 2. */
+		if (retval)
+			retval++;
+	}
 	return retval;
 }
 
diff --git a/kernel/sys.c b/kernel/sys.c
index 4da31f28fda8..ed12ac6f5a8a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2784,6 +2784,24 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_RISCV_SET_ICACHE_FLUSH_CTX:
 		error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
 		break;
+	case PR_GET_GRBITMAP:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
+		error = current_cred()->group_info->restrict_bitmap;
+		break;
+	case PR_SET_GRBITMAP:
+		/* Allow 31 bits to avoid setting sign bit. */
+		if (arg2 > (1U << 31) - 1 || arg3 || arg4 || arg5)
+			return -EINVAL;
+		current_cred()->group_info->restrict_bitmap |= arg2;
+		break;
+	case PR_CLR_GRBITMAP:
+		if (arg2 || arg3 || arg4 || arg5)
+			return -EINVAL;
+		if (!may_setgroups())
+			return -EPERM;
+		current_cred()->group_info->restrict_bitmap = 0;
+		break;
 	default:
 		error = -EINVAL;
 		break;
-- 
2.46.2
Re: [PATCH v3] add group restriction bitmap
Posted by Oleg Nesterov 1 month, 4 weeks ago 
I can't comment the intent, just some nits about implementation.

On 09/30, Stas Sergeev wrote:
>
>  struct group_info {
>  	refcount_t	usage;
> +	unsigned int	restrict_bitmap;

Why not unsigned long?

>  int groups_search(const struct group_info *group_info, kgid_t grp)
>  {
>  	unsigned int left, right;
> @@ -105,7 +108,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>  		else if (gid_lt(grp, group_info->gid[mid]))
>  			right = mid;
>  		else
> -			return 1;
> +			return mid + 1;

Suppose we change groups_search()

	--- a/kernel/groups.c
	+++ b/kernel/groups.c
	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
				left = mid + 1;
			else if (gid_lt(grp, group_info->gid[mid]))
				right = mid;
	-		else
	-			return 1;
	+		else {
	+			bool r = mid < BITS_PER_LONG &&
	+				 test_bit(mid, &group_info->restrict_bitmap);
	+			return r ? -1 : 1;
	+		}
		}
		return 0;
	 }

so that it returns, say, -1 if the found grp is restricted.

Then everything else can be greatly simplified, afaics...

Oleg.
Re: [PATCH v3] add group restriction bitmap
Posted by stsp 1 month, 4 weeks ago 
01.10.2024 14:15, Oleg Nesterov пишет:
> I can't comment the intent, just some nits about implementation.
>
> On 09/30, Stas Sergeev wrote:
>>   struct group_info {
>>   	refcount_t	usage;
>> +	unsigned int	restrict_bitmap;
> Why not unsigned long?

My impl claims to support 31bit only.
Maybe use "unsigned long long" to
get like 63? Isn't "unsigned long"
arch-dependent? What would be the
benefit of an arch-dependent bitmap?

>>   int groups_search(const struct group_info *group_info, kgid_t grp)
>>   {
>>   	unsigned int left, right;
>> @@ -105,7 +108,7 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>   		else if (gid_lt(grp, group_info->gid[mid]))
>>   			right = mid;
>>   		else
>> -			return 1;
>> +			return mid + 1;
> Suppose we change groups_search()
>
> 	--- a/kernel/groups.c
> 	+++ b/kernel/groups.c
> 	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> 				left = mid + 1;
> 			else if (gid_lt(grp, group_info->gid[mid]))
> 				right = mid;
> 	-		else
> 	-			return 1;
> 	+		else {
> 	+			bool r = mid < BITS_PER_LONG &&
> 	+				 test_bit(mid, &group_info->restrict_bitmap);
> 	+			return r ? -1 : 1;
> 	+		}
> 		}
> 		return 0;
> 	 }
>
> so that it returns, say, -1 if the found grp is restricted.
>
> Then everything else can be greatly simplified, afaics...
This will mean updating all callers
of groups_search(), in_group_p(),
in_egroup_p(), vfsxx_in_group_p()
and so on. Also I am not sure if it really
helps: in_group_p() has also gid, so
if in_group_p() returns -1 for not found
and 0 for gid, then I still need to
increment the retval of groups_search(),
just as I do now.
So unless I am missing your intention,
this isn't really helping.
Re: [PATCH v3] add group restriction bitmap
Posted by Oleg Nesterov 1 month, 4 weeks ago 
On 10/01, stsp wrote:
>
> 01.10.2024 14:15, Oleg Nesterov пишет:
> >Suppose we change groups_search()
> >
> >	--- a/kernel/groups.c
> >	+++ b/kernel/groups.c
> >	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> >				left = mid + 1;
> >			else if (gid_lt(grp, group_info->gid[mid]))
> >				right = mid;
> >	-		else
> >	-			return 1;
> >	+		else {
> >	+			bool r = mid < BITS_PER_LONG &&
> >	+				 test_bit(mid, &group_info->restrict_bitmap);
> >	+			return r ? -1 : 1;
> >	+		}
> >		}
> >		return 0;
> >	 }
> >
> >so that it returns, say, -1 if the found grp is restricted.
> >
> >Then everything else can be greatly simplified, afaics...
> This will mean updating all callers
> of groups_search(), in_group_p(),
> in_egroup_p(), vfsxx_in_group_p()

Why? I think with this change you do not need to touch in_group_p/etc at all.

> if in_group_p() returns -1 for not found
> and 0 for gid,

With the the change above in_group_p() returns 0 if not found, !0 otherwise.
It returns -1 if grp != cred->fsgid and the found grp is restricted.

So acl_permission_check() can simply do

	if (mask & (mode ^ (mode >> 3))) {
		vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
		int xxx = vfsgid_in_group_p(vfsgid);

		if (xxx) {
			if (mask & ~(mode >> 3))
				return -EACCES;
			if (xxx > 0)
				return 0;
			/* If we hit restrict_bitmap, then check Others. */
		}
	}

> So unless I am missing your intention,
> this isn't really helping.

OK, please forget, perhaps I missed something.

Oleg.
Re: [PATCH v3] add group restriction bitmap
Posted by stsp 1 month, 4 weeks ago 
01.10.2024 16:02, Oleg Nesterov пишет:
> On 10/01, stsp wrote:
>> 01.10.2024 14:15, Oleg Nesterov пишет:
>>> Suppose we change groups_search()
>>>
>>> 	--- a/kernel/groups.c
>>> 	+++ b/kernel/groups.c
>>> 	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
>>> 				left = mid + 1;
>>> 			else if (gid_lt(grp, group_info->gid[mid]))
>>> 				right = mid;
>>> 	-		else
>>> 	-			return 1;
>>> 	+		else {
>>> 	+			bool r = mid < BITS_PER_LONG &&
>>> 	+				 test_bit(mid, &group_info->restrict_bitmap);
>>> 	+			return r ? -1 : 1;
>>> 	+		}
>>> 		}
>>> 		return 0;
>>> 	 }
>>>
>>> so that it returns, say, -1 if the found grp is restricted.
>>>
>>> Then everything else can be greatly simplified, afaics...
>> This will mean updating all callers
>> of groups_search(), in_group_p(),
>> in_egroup_p(), vfsxx_in_group_p()
> Why? I think with this change you do not need to touch in_group_p/etc at all.
>
>> if in_group_p() returns -1 for not found
>> and 0 for gid,
> With the the change above in_group_p() returns 0 if not found, !0 otherwise.
> It returns -1 if grp != cred->fsgid and the found grp is restricted.

in_group_p() doesn't check if the
group is restricted or not.
acl_permission_check() does, but
in your example it doesn't as well.
I think you mean to move the
restrict_bitmap check upwards to
in_group_p()?
Anyway, suppose you don't mean that.
In this case:
1. in_group_p() and in_egroup_p()
   should be changed:
-  int retval = 1;
+ int retval = -1;

But their callers should not.
There are also the callers of groups_search()
in kernel/auditsc.c and they should
be updated. But they are few.
Just to be clear, is this what you suggest?

> So acl_permission_check() can simply do
>
> 	if (mask & (mode ^ (mode >> 3))) {
> 		vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> 		int xxx = vfsgid_in_group_p(vfsgid);
>
> 		if (xxx) {
> 			if (mask & ~(mode >> 3))
> 				return -EACCES;
> 			if (xxx > 0)
> 				return 0;
> 			/* If we hit restrict_bitmap, then check Others. */
> 		}
> 	}

Well, in my impl it should check
the bitmap right here, but you removed
that. Maybe you want the check elsewhere?
Re: [PATCH v3] add group restriction bitmap
Posted by Oleg Nesterov 1 month, 3 weeks ago 
We can't understand each other. I guess I missed something...

On 10/01, stsp wrote:
>
> 01.10.2024 16:02, Oleg Nesterov пишет:
> >On 10/01, stsp wrote:
> >>01.10.2024 14:15, Oleg Nesterov пишет:
> >>>Suppose we change groups_search()
> >>>
> >>>	--- a/kernel/groups.c
> >>>	+++ b/kernel/groups.c
> >>>	@@ -104,8 +104,11 @@ int groups_search(const struct group_info *group_info, kgid_t grp)
> >>>				left = mid + 1;
> >>>			else if (gid_lt(grp, group_info->gid[mid]))
> >>>				right = mid;
> >>>	-		else
> >>>	-			return 1;
> >>>	+		else {
> >>>	+			bool r = mid < BITS_PER_LONG &&
> >>>	+				 test_bit(mid, &group_info->restrict_bitmap);
> >>>	+			return r ? -1 : 1;
> >>>	+		}
> >>>		}
> >>>		return 0;
> >>>	 }
> >>>
> >>>so that it returns, say, -1 if the found grp is restricted.
> >>>
> >>>Then everything else can be greatly simplified, afaics...
> >>This will mean updating all callers
> >>of groups_search(), in_group_p(),
> >>in_egroup_p(), vfsxx_in_group_p()
> >Why? I think with this change you do not need to touch in_group_p/etc at all.
> >
> >>if in_group_p() returns -1 for not found
> >>and 0 for gid,
> >With the the change above in_group_p() returns 0 if not found, !0 otherwise.
> >It returns -1 if grp != cred->fsgid and the found grp is restricted.
>
> in_group_p() doesn't check if the
> group is restricted or not.

And it shouldn't. It returns the result of groups_search() if this
grp is supplementary or "not found".

> acl_permission_check() does, but
> in your example it doesn't as well.

But it does??? see below...

> I think you mean to move the
> restrict_bitmap check upwards to
> in_group_p()?

No, I meant to move the restrict_bitmap check to groups_search(), see the patch
above.

> Anyway, suppose you don't mean that.
> In this case:
> 1. in_group_p() and in_egroup_p()
>   should be changed:
> -  int retval = 1;
> + int retval = -1;

Why? -1 means that the grp is supplementary and restricted.

> There are also the callers of groups_search()
> in kernel/auditsc.c and they should
> be updated.

Why? I don't think so. audit_filter_rules() uses the result of groups_search()
as a boolean.

> >So acl_permission_check() can simply do
> >
> >	if (mask & (mode ^ (mode >> 3))) {
> >		vfsgid_t vfsgid = i_gid_into_vfsgid(idmap, inode);
> >		int xxx = vfsgid_in_group_p(vfsgid);
> >
> >		if (xxx) {
> >			if (mask & ~(mode >> 3))
> >				return -EACCES;
> >			if (xxx > 0)
> >				return 0;
> >			/* If we hit restrict_bitmap, then check Others. */
> >		}
> >	}
>
> Well, in my impl it should check
> the bitmap right here, but you removed
> that.

No, I didn't remove the check, this code relies on the change in
groups_search(). Note the "xxx > 0" check.

I must have missed something :/

Oleg.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.