fs/read_all: Clear suplementary groups before droping privileges

Message ID 1526721740-8382-1-git-send-email-yangx.jy@cn.fujitsu.com
State Accepted
Headers show
Series
  • fs/read_all: Clear suplementary groups before droping privileges
Related show

Commit Message

Xiao Yang May 19, 2018, 9:22 a.m.
Current user(e.g. root) has its own suplementary group set when logged in.  Which
means that even when a program sets it's user and group ids to nobody the current
group still stays in the list of supplementary groups, which then is matched for
files with the current group ownership and hence we can still access the file.

For example, if /dev/watchdog has root group ownership and rw group permissions,
running read_all_dev can still open /dev/watchdog and reboot system even after
switching user and group ids from root to nobody.

We need to clear suplementary groups before droping privileges and keep the same
rule as commit 1f011e5 if current user doesn't have the capabilities to clear
suplementary groups.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/fs/read_all/read_all.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Richard Palethorpe May 22, 2018, 10:26 a.m. | #1
Hello,

Xiao Yang writes:

> Current user(e.g. root) has its own suplementary group set when logged in.  Which
> means that even when a program sets it's user and group ids to nobody the current
> group still stays in the list of supplementary groups, which then is matched for
> files with the current group ownership and hence we can still access the file.
>
> For example, if /dev/watchdog has root group ownership and rw group permissions,
> running read_all_dev can still open /dev/watchdog and reboot system even after
> switching user and group ids from root to nobody.
>
> We need to clear suplementary groups before droping privileges and keep the same
> rule as commit 1f011e5 if current user doesn't have the capabilities to clear
> suplementary groups.
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  testcases/kernel/fs/read_all/read_all.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
> index a8e1611..acd8e73 100644
> --- a/testcases/kernel/fs/read_all/read_all.c
> +++ b/testcases/kernel/fs/read_all/read_all.c
> @@ -258,6 +258,12 @@ static void maybe_drop_privs(void)
>  	if (!drop_privs)
>  		return;
>  
> +	TEST(setgroups(0, NULL));
> +	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM) {
> +		tst_brk(TBROK | TTERRNO,
> +			"Failed to clear suplementary group set");
> +	}
> +
>  	nobody = SAFE_GETPWNAM("nobody");
>  
>  	TEST(setgid(nobody->pw_gid));

LGTM!
Cyril Hrubis May 22, 2018, 10:54 a.m. | #2
Hi!
I've added #include <grp.h> to the patch so that we get definition for
the setgrp() call and pushed, thanks.
Cyril Hrubis May 22, 2018, 10:56 a.m. | #3
Hi!
> LGTM!

We may as well think about safe groups for the program, looking at my
/dev/ it may sense to add auxiliary groups disk and tty so that we tests
these devices as well.

Patch

diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index a8e1611..acd8e73 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -258,6 +258,12 @@  static void maybe_drop_privs(void)
 	if (!drop_privs)
 		return;
 
+	TEST(setgroups(0, NULL));
+	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM) {
+		tst_brk(TBROK | TTERRNO,
+			"Failed to clear suplementary group set");
+	}
+
 	nobody = SAFE_GETPWNAM("nobody");
 
 	TEST(setgid(nobody->pw_gid));