diff mbox series

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

Message ID 1648718902-2319-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v1,1/2] idmapped-mounts: Add mknodat operation in setgid test | expand

Commit Message

Yang Xu (Fujitsu) March 31, 2022, 9:28 a.m. UTC 
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(-)

Comments

Christian Brauner March 31, 2022, 11:59 a.m. UTC | #1 
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>
Christian Brauner March 31, 2022, 12:10 p.m. UTC | #2 
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.
Yang Xu (Fujitsu) April 1, 2022, 6:08 a.m. UTC | #3 
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>
Yang Xu (Fujitsu) April 1, 2022, 6:11 a.m. UTC | #4 
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
diff mbox series

Patch

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))