[PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[Yang Xu]

Since mknodat can create file, we should also check whether strip S_ISGID.
Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
needs it when using mknod.

Only test mknod with character device in setgid_create function because the another
two functions will hit EPERM error.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 154 ++++++++++++++++++++++++--
 1 file changed, 147 insertions(+), 7 deletions(-)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 4cf6c3bb..1e2f3904 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -241,6 +241,34 @@ static inline bool caps_supported(void)
 	return ret;
 }
 
+static int caps_down_fsetid(void)
+{
+	bool fret = false;
+#ifdef HAVE_SYS_CAPABILITY_H
+	cap_t caps = NULL;
+	cap_value_t cap = CAP_FSETID;
+	int ret = -1;
+
+	caps = cap_get_proc();
+	if (!caps)
+		goto out;
+
+	ret = cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap, 0);
+	if (ret)
+		goto out;
+
+	ret = cap_set_proc(caps);
+	if (ret)
+		goto out;
+
+	fret = true;
+
+out:
+	cap_free(caps);
+#endif
+	return fret;
+}
+
 /* caps_down - lower all effective caps */
 static int caps_down(void)
 {
@@ -7805,8 +7833,8 @@ out_unmap:
 #endif /* HAVE_LIBURING_H */
 
 /* The following tests are concerned with setgid inheritance. These can be
- * filesystem type specific. For xfs, if a new file or directory is created
- * within a setgid directory and irix_sgid_inhiert is set then inherit the
+ * filesystem type specific. For xfs, if a new file or directory or node is
+ * created within a setgid directory and irix_sgid_inhiert is set then inherit the
  * setgid bit if the caller is in the group of the directory.
  */
 static int setgid_create(void)
@@ -7863,15 +7891,41 @@ static int setgid_create(void)
 		if (!is_setgid(t_dir1_fd, DIR1, 0))
 			die("failure: is_setgid");
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, DIR1 "/" FILE1, S_IFREG | S_ISGID | 0755, 0))
+			die("failure: mknodat");
+
+		if (!is_setgid(t_dir1_fd, DIR1 "/" FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (!is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(t_dir1_fd, DIR1 "/" FILE1, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
 		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
 		if (unlinkat(t_dir1_fd, FILE1, 0))
 			die("failure: delete");
 
+		if (unlinkat(t_dir1_fd, DIR1 "/" FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
@@ -7889,8 +7943,8 @@ static int setgid_create(void)
 		if (!switch_ids(0, 10000))
 			die("failure: switch_ids");
 
-		if (!caps_down())
-			die("failure: caps_down");
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
 
 		/* create regular file via open() */
 		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
@@ -7917,6 +7971,19 @@ static int setgid_create(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, DIR1 "/" FILE1, S_IFREG | S_ISGID | 0755, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, DIR1 "/" FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
 		/*
 		 * In setgid directories newly created files always inherit the
 		 * gid from the parent directory. Verify that the file is owned
@@ -7933,6 +8000,24 @@ static int setgid_create(void)
 		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(t_dir1_fd, DIR1 "/" FILE1, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1 "/" FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8051,6 +8136,12 @@ static int setgid_create_idmapped(void)
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 10000, 10000))
 			die("failure: check ownership");
 
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8149,15 +8240,26 @@ static int setgid_create_idmapped_in_userns(void)
 		if (!is_setgid(open_tree_fd, DIR1, 0))
 			die("failure: is_setgid");
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, DIR1 "/" FILE1, S_IFREG | S_ISGID | 755, 0))
+			die("failure: mknodat");
+
+		if (!is_setgid(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, DIR1 "/" FILE1, 0, 0, 0))
+			die("failure: check ownership");
+
 		if (unlinkat(open_tree_fd, FILE1, 0))
 			die("failure: delete");
-
+		if (unlinkat(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: delete");
 		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
@@ -8190,9 +8292,12 @@ static int setgid_create_idmapped_in_userns(void)
 			exit(EXIT_SUCCESS);
 		}
 
-		if (!switch_userns(attr.userns_fd, 0, 0, true))
+		if (!switch_userns(attr.userns_fd, 0, 0, false))
 			die("failure: switch_userns");
 
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
 		/* create regular file via open() */
 		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
 		if (file1_fd < 0)
@@ -8218,6 +8323,13 @@ static int setgid_create_idmapped_in_userns(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, DIR1 "/" FILE1, S_IFREG | S_ISGID | 0755, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: is_setgid");
+
 		/*
 		 * In setgid directories newly created files always inherit the
 		 * gid from the parent directory. Verify that the file is owned
@@ -8234,9 +8346,15 @@ static int setgid_create_idmapped_in_userns(void)
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 1000))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, DIR1 "/" FILE1, 0, 0, 1000))
+			die("failure: check ownership");
+
 		if (unlinkat(open_tree_fd, FILE1, 0))
 			die("failure: delete");
 
+		if (unlinkat(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: delete");
+
 		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
@@ -8266,9 +8384,12 @@ static int setgid_create_idmapped_in_userns(void)
 			exit(EXIT_SUCCESS);
 		}
 
-		if (!switch_userns(attr.userns_fd, 0, 1000, true))
+		if (!switch_userns(attr.userns_fd, 0, 1000, false))
 			die("failure: switch_userns");
 
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
 		/* create regular file via open() */
 		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
 		if (file1_fd < 0)
@@ -8295,12 +8416,31 @@ static int setgid_create_idmapped_in_userns(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, DIR1 "/" FILE1, S_IFREG | S_ISGID | 0755, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, DIR1 "/" FILE1, 0, 0, 0))
+			die("failure: check ownership");
+
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1 "/" FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
-- 
2.27.0

[PATCH v1 2/2] idmapped-mounts: Add umask before test setgid_create

[Yang Xu]

Since stipping S_SIGID should check S_IXGRP, so umask it to check whether
works well.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
If we enable acl on parent directory, then umask is useless, maybe we
also add setfacl on parent directory because we may change the order
about strip S_ISGID and posix_acl setup. Any idea?

 src/idmapped-mounts/idmapped-mounts.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 1e2f3904..30292426 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -7843,6 +7843,7 @@ static int setgid_create(void)
 	int file1_fd = -EBADF;
 	pid_t pid;
 
+	umask(S_IXGRP);
 	if (!caps_supported())
 		return 0;
 
@@ -8040,6 +8041,8 @@ static int setgid_create_idmapped(void)
 	};
 	pid_t pid;
 
+	umask(S_IXGRP);
+
 	if (!caps_supported())
 		return 0;
 
@@ -8166,6 +8169,7 @@ static int setgid_create_idmapped_in_userns(void)
 	};
 	pid_t pid;
 
+	umask(S_IXGRP);
 	if (!caps_supported())
 		return 0;
 
-- 
2.27.0

Re: [PATCH v1 2/2] idmapped-mounts: Add umask before test setgid_create

[Christian Brauner]

On Thu, Mar 31, 2022 at 05:28:22PM +0800, Yang Xu wrote:
> Since stipping S_SIGID should check S_IXGRP, so umask it to check whether
> works well.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

(Sidenote: I really need to rename the test binary to something other
than idmapped-mounts.c as this tests a lot of generic vfs stuff that has
nothing to do with them.)

Tested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

> If we enable acl on parent directory, then umask is useless, maybe we
> also add setfacl on parent directory because we may change the order
> about strip S_ISGID and posix_acl setup. Any idea?

If acls figure into this then this should probably be a new test or
subtest.

Re: [PATCH v1 2/2] idmapped-mounts: Add umask before test setgid_create

[xuyang2018.jy]

on 2022/3/31 20:02, Christian Brauner wrote:
> On Thu, Mar 31, 2022 at 05:28:22PM +0800, Yang Xu wrote:
>> Since stipping S_SIGID should check S_IXGRP, so umask it to check whether
>> works well.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> (Sidenote: I really need to rename the test binary to something other
> than idmapped-mounts.c as this tests a lot of generic vfs stuff that has
> nothing to do with them.)
>
> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>
>> If we enable acl on parent directory, then umask is useless, maybe we
>> also add setfacl on parent directory because we may change the order
>> about strip S_ISGID and posix_acl setup. Any idea?
>
> If acls figure into this then this should probably be a new test or
> subtest.
Will add it on v2

Best Regards
Yang Xu

Re: [PATCH v1 2/2] idmapped-mounts: Add umask before test setgid_create

[xuyang2018.jy]

on 2022/4/1 14:08, xuyang2018.jy@fujitsu.com wrote:
> on 2022/3/31 20:02, Christian Brauner wrote:
>> On Thu, Mar 31, 2022 at 05:28:22PM +0800, Yang Xu wrote:
>>> Since stipping S_SIGID should check S_IXGRP, so umask it to check whether
>>> works well.
>>>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>
>> (Sidenote: I really need to rename the test binary to something other
>> than idmapped-mounts.c as this tests a lot of generic vfs stuff that has
>> nothing to do with them.)
>>
>> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>
>>> If we enable acl on parent directory, then umask is useless, maybe we
>>> also add setfacl on parent directory because we may change the order
>>> about strip S_ISGID and posix_acl setup. Any idea?
>>
>> If acls figure into this then this should probably be a new test or
>> subtest.
> Will add it on v2

I have a holiday (4.2-4.5), so will do this v2 in next week when I come 
back.

Best Regards
Yang Xu
>
> Best Regards
> Yang Xu

Re: [PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[Christian Brauner]

On Thu, Mar 31, 2022 at 05:28:21PM +0800, Yang Xu wrote:
> Since mknodat can create file, we should also check whether strip S_ISGID.
> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
> depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
> needs it when using mknod.
> 
> Only test mknod with character device in setgid_create function because the another
> two functions will hit EPERM error.

Fwiw, it's not allowed to create devices in userns as that would be a
massive attack vector. But it is possible since 5.<some version> to
create whiteouts in userns for the sake of overlayfs. So iirc that
creating a whiteout is just passing 0 as dev_t:

mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, 0)

but you'd need to detect whether the kernel allows this and skip the
test on EPERM when it is a userns test.

> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Sidenote: I really need to rename the test binary to something other
than idmapped-mounts.c as this tests a lot of generic vfs stuff that has
nothing to do with them.

In any case, I pulled and tested this:

Tested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[Christian Brauner]

On Thu, Mar 31, 2022 at 01:59:25PM +0200, Christian Brauner wrote:
> On Thu, Mar 31, 2022 at 05:28:21PM +0800, Yang Xu wrote:
> > Since mknodat can create file, we should also check whether strip S_ISGID.
> > Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
> > depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
> > needs it when using mknod.
> > 
> > Only test mknod with character device in setgid_create function because the another
> > two functions will hit EPERM error.
> 
> Fwiw, it's not allowed to create devices in userns as that would be a
> massive attack vector. But it is possible since 5.<some version> to
> create whiteouts in userns for the sake of overlayfs. So iirc that
> creating a whiteout is just passing 0 as dev_t:
> 
> mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, 0)
> 
> but you'd need to detect whether the kernel allows this and skip the
> test on EPERM when it is a userns test.

Oh, iirc Eryu usually prefers if we don't just extend existing tests but
add new tests so as not to introduce regressions. So instead of adding
this into the existings tests you _could_ add them as new separate 

struct t_idmapped_mounts t_setgid[] = {
};

set of tests and add a new command line switch:

--test-setgid

and create a new

generic/67*

for it. You can use:
d17a88e90956 ("generic: test idmapped mount circular mappings")
as a template for what I mean.

Re: [PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[xuyang2018.jy]

on 2022/3/31 20:10, Christian Brauner wrote:
> On Thu, Mar 31, 2022 at 01:59:25PM +0200, Christian Brauner wrote:
>> On Thu, Mar 31, 2022 at 05:28:21PM +0800, Yang Xu wrote:
>>> Since mknodat can create file, we should also check whether strip S_ISGID.
>>> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
>>> depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
>>> needs it when using mknod.
>>>
>>> Only test mknod with character device in setgid_create function because the another
>>> two functions will hit EPERM error.
>>
>> Fwiw, it's not allowed to create devices in userns as that would be a
>> massive attack vector. But it is possible since 5.<some version>  to
>> create whiteouts in userns for the sake of overlayfs. So iirc that
>> creating a whiteout is just passing 0 as dev_t:
>>
>> mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, 0)
>>
>> but you'd need to detect whether the kernel allows this and skip the
>> test on EPERM when it is a userns test.
>
> Oh, iirc Eryu usually prefers if we don't just extend existing tests but
> add new tests so as not to introduce regressions. So instead of adding
> this into the existings tests you _could_ add them as new separate
>
> struct t_idmapped_mounts t_setgid[] = {
> };
>
> set of tests and add a new command line switch:
>
> --test-setgid
>
> and create a new
>
> generic/67*
>
> for it. You can use:
> d17a88e90956 ("generic: test idmapped mount circular mappings")
> as a template for what I mean.
When I write this patchset, I also think about it. I plan to move setgid 
test from of test-core group and use a new test-segid group(also 
increase its coverage).

Will do it on v2.

Best Regards
Yang Xu

Re: [PATCH v1 1/2] idmapped-mounts: Add mknodat operation in setgid test

[xuyang2018.jy]

on 2022/3/31 19:59, Christian Brauner wrote:
> On Thu, Mar 31, 2022 at 05:28:21PM +0800, Yang Xu wrote:
>> Since mknodat can create file, we should also check whether strip S_ISGID.
>> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
>> depond on this cap and keep other cap(ie CAP_MKNOD) because create character device
>> needs it when using mknod.
>>
>> Only test mknod with character device in setgid_create function because the another
>> two functions will hit EPERM error.
>
> Fwiw, it's not allowed to create devices in userns as that would be a
> massive attack vector. But it is possible since 5.<some version>  to
> create whiteouts in userns for the sake of overlayfs. So iirc that
> creating a whiteout is just passing 0 as dev_t:
>
> mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | 0755, 0)
>
> but you'd need to detect whether the kernel allows this and skip the
> test on EPERM when it is a userns test.

I have found the kernel commit 	a3c751a50 ("vfs: allow unprivileged 
whiteout creation") in v5.8-rc1.
Thanks. Will create whiteout instead of actual character device on v2.


>
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Sidenote: I really need to rename the test binary to something other
> than idmapped-mounts.c as this tests a lot of generic vfs stuff that has
> nothing to do with them.
Agree.

Best Regards
Yang Xu
>
> In any case, I pulled and tested this:
>
> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>