[LTP] [PATCH] read_all: Drop privileges

[LTP] [PATCH] read_all: Drop privileges

[Richard Palethorpe]

The LTP is usually run as root, which allows read_all_dev to read files which
are usually protected from being read at random. This patch introduces the -p
switch to read_all which is used to drop privileges (switch to the nobody
user) for the read_all_dev test.

If -p is set, but the current user does not have the capabilities to change
the uid and gid, then the test will continue under the current user. This
allows the most common scenarios to work as expected, but may cause
difficulties for someone running the LTP under a semi-privileged user.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 runtest/fs                              |  2 +-
 testcases/kernel/fs/read_all/read_all.c | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/runtest/fs b/runtest/fs
index 42a9bfcbf..a66948a43 100644
--- a/runtest/fs
+++ b/runtest/fs
@@ -69,7 +69,7 @@ fs_di fs_di -d $TMPDIR
 # Was not sure why it should reside in runtest/crashme and won´t get tested ever
 proc01 proc01 -m 128
 
-read_all_dev read_all -d /dev -e '/dev/watchdog?(0)' -q -r 10
+read_all_dev read_all -d /dev -p -q -r 10
 read_all_proc read_all -d /proc -q -r 10
 read_all_sys read_all -d /sys -q -r 10
 
diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index add3651c8..9c632c009 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -50,6 +50,7 @@
 #include <fnmatch.h>
 #include <semaphore.h>
 #include <ctype.h>
+#include <pwd.h>
 
 #include "tst_test.h"
 
@@ -88,6 +89,7 @@ static long worker_count;
 static char *str_max_workers;
 static long max_workers = 15;
 static struct worker *workers;
+static char *drop_privs;
 
 static struct tst_option options[] = {
 	{"v", &verbose,
@@ -104,6 +106,8 @@ static struct tst_option options[] = {
 	 "-w count Set the worker count limit, the default is 15."},
 	{"W:", &str_worker_count,
 	 "-W count Override the worker count. Ignores (-w) and the processor count."},
+	{"p", &drop_privs,
+	 "-p       Drop privileges; switch to the nobody user."},
 	{NULL, NULL, NULL}
 };
 
@@ -247,6 +251,24 @@ static int worker_run(struct worker *self)
 	return 0;
 }
 
+static void maybe_drop_privs(void)
+{
+	struct passwd *nobody;
+
+	if (!drop_privs)
+		return;
+
+	nobody = SAFE_GETPWNAM("nobody");
+
+	TEST(setgid(nobody->pw_gid));
+	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
+		tst_res(TBROK | TTERRNO, "Failed to use nobody gid");
+
+	TEST(setuid(nobody->pw_uid));
+	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
+		tst_res(TBROK | TTERRNO, "Failed to use nobody uid");
+}
+
 static void spawn_workers(void)
 {
 	int i;
@@ -257,8 +279,10 @@ static void spawn_workers(void)
 	for (i = 0; i < worker_count; i++) {
 		wa[i].q = queue_init();
 		wa[i].pid = SAFE_FORK();
-		if (!wa[i].pid)
+		if (!wa[i].pid) {
+			maybe_drop_privs();
 			exit(worker_run(wa + i));
+		}
 	}
 }
 
-- 
2.16.3

[LTP] [PATCH] read_all: Drop privileges

[Cyril Hrubis]

Hi!
> +static void maybe_drop_privs(void)
> +{
> +	struct passwd *nobody;
> +
> +	if (!drop_privs)
> +		return;
> +
> +	nobody = SAFE_GETPWNAM("nobody");
> +
> +	TEST(setgid(nobody->pw_gid));
> +	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
> +		tst_res(TBROK | TTERRNO, "Failed to use nobody gid");
                ^
		Shouldn't this be tst_brk()?

> +	TEST(setuid(nobody->pw_uid));
> +	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
> +		tst_res(TBROK | TTERRNO, "Failed to use nobody uid");
                ^
		And here as well?

Otherwise it looks fine.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all: Drop privileges

[Richard Palethorpe]

Hello,

Cyril Hrubis writes:

> Hi!
>> +static void maybe_drop_privs(void)
>> +{
>> +	struct passwd *nobody;
>> +
>> +	if (!drop_privs)
>> +		return;
>> +
>> +	nobody = SAFE_GETPWNAM("nobody");
>> +
>> +	TEST(setgid(nobody->pw_gid));
>> +	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
>> +		tst_res(TBROK | TTERRNO, "Failed to use nobody gid");
>                 ^
> 		Shouldn't this be tst_brk()?
>
>> +	TEST(setuid(nobody->pw_uid));
>> +	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
>> +		tst_res(TBROK | TTERRNO, "Failed to use nobody uid");
>                 ^
> 		And here as well?
>
> Otherwise it looks fine.

Well spotted, yes it should.

-- 
Thank you,
Richard.

[LTP] [PATCH] read_all: Drop privileges

[Cyril Hrubis]

Hi!
> > Otherwise it looks fine.
> 
> Well spotted, yes it should.

I will push it with that change then, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] read_all: Drop privileges

[Richard Palethorpe]

The LTP is usually run as root, which allows read_all_dev to read files which
are usually protected from being read at random. This patch introduces the -p
switch to read_all which is used to drop privileges (switch to the nobody
user) for the read_all_dev test.

If -p is set, but the current user does not have the capabilities to change
the uid and gid, then the test will continue under the current user. This
allows the most common scenarios to work as expected, but may cause
difficulties for someone running the LTP under a semi-privileged user.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 runtest/fs                              |  2 +-
 testcases/kernel/fs/read_all/read_all.c | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/runtest/fs b/runtest/fs
index 42a9bfcbf..a66948a43 100644
--- a/runtest/fs
+++ b/runtest/fs
@@ -69,7 +69,7 @@ fs_di fs_di -d $TMPDIR
 # Was not sure why it should reside in runtest/crashme and won´t get tested ever
 proc01 proc01 -m 128
 
-read_all_dev read_all -d /dev -e '/dev/watchdog?(0)' -q -r 10
+read_all_dev read_all -d /dev -p -q -r 10
 read_all_proc read_all -d /proc -q -r 10
 read_all_sys read_all -d /sys -q -r 10
 
diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
index add3651c8..a8e161129 100644
--- a/testcases/kernel/fs/read_all/read_all.c
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -50,6 +50,7 @@
 #include <fnmatch.h>
 #include <semaphore.h>
 #include <ctype.h>
+#include <pwd.h>
 
 #include "tst_test.h"
 
@@ -88,6 +89,7 @@ static long worker_count;
 static char *str_max_workers;
 static long max_workers = 15;
 static struct worker *workers;
+static char *drop_privs;
 
 static struct tst_option options[] = {
 	{"v", &verbose,
@@ -104,6 +106,8 @@ static struct tst_option options[] = {
 	 "-w count Set the worker count limit, the default is 15."},
 	{"W:", &str_worker_count,
 	 "-W count Override the worker count. Ignores (-w) and the processor count."},
+	{"p", &drop_privs,
+	 "-p       Drop privileges; switch to the nobody user."},
 	{NULL, NULL, NULL}
 };
 
@@ -247,6 +251,24 @@ static int worker_run(struct worker *self)
 	return 0;
 }
 
+static void maybe_drop_privs(void)
+{
+	struct passwd *nobody;
+
+	if (!drop_privs)
+		return;
+
+	nobody = SAFE_GETPWNAM("nobody");
+
+	TEST(setgid(nobody->pw_gid));
+	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
+		tst_brk(TBROK | TTERRNO, "Failed to use nobody gid");
+
+	TEST(setuid(nobody->pw_uid));
+	if (TEST_RETURN < 0 && TEST_ERRNO != EPERM)
+		tst_brk(TBROK | TTERRNO, "Failed to use nobody uid");
+}
+
 static void spawn_workers(void)
 {
 	int i;
@@ -257,8 +279,10 @@ static void spawn_workers(void)
 	for (i = 0; i < worker_count; i++) {
 		wa[i].q = queue_init();
 		wa[i].pid = SAFE_FORK();
-		if (!wa[i].pid)
+		if (!wa[i].pid) {
+			maybe_drop_privs();
 			exit(worker_run(wa + i));
+		}
 	}
 }
 
-- 
2.16.3

[LTP] [PATCH] read_all: Drop privileges

[Punit Agrawal]

Hi Richard, Cyril,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > Otherwise it looks fine.
>> 
>> Well spotted, yes it should.
>
> I will push it with that change then, thanks.

I can confirm that with the update, the system no longer generates
aborts due to /dev/port access.

Thanks for the quick turn around.

Punit

[LTP] [PATCH] read_all: Drop privileges

[Punit Agrawal]

Hi Richard, Cyril,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > Otherwise it looks fine.
>> 
>> Well spotted, yes it should.
>
> I will push it with that change then, thanks.

I can confirm that with the change the system no longer generates aborts
while reading nodes in /dev.

Thanks for pushing this through before the release.

Punit

[LTP] [PATCH] read_all: Drop privileges

[Richard Palethorpe]

Hello,

Punit Agrawal writes:

> Hi Richard, Cyril,
>
> Cyril Hrubis <chrubis@suse.cz> writes:
>
>> Hi!
>>> > Otherwise it looks fine.
>>> 
>>> Well spotted, yes it should.
>>
>> I will push it with that change then, thanks.
>
> I can confirm that with the update, the system no longer generates
> aborts due to /dev/port access.
>
> Thanks for the quick turn around.
>
> Punit

Great, thanks for testing it.

-- 
Thank you,
Richard.

[LTP] [PATCH] read_all: Drop privileges

[Xiao Yang]

Hi Richard,

If the permission of /dev/watchdog was 0600(default permission on RHEL7), we could not read /dev/watchdog
as nobody user and returned EACCES as expected.

If the permission of /dev/watchdog was 0660(default permission on RHEL6), Reading /dev/watchdog as nobody
user failed, but still led to system reboot.

I think reading /dev/watchdog as nobody user should get EACCES even if the permission is 0660, but i am not
sure whether this is a watchdog bug in kernel or not.

Thanks,
Xiao Yang

On 2018/05/15 18:55, Richard Palethorpe wrote:
> Hello,
>
> Cyril Hrubis writes:
>
>> Hi!
>>> +static void maybe_drop_privs(void)
>>> +{
>>> +	struct passwd *nobody;
>>> +
>>> +	if (!drop_privs)
>>> +		return;
>>> +
>>> +	nobody = SAFE_GETPWNAM("nobody");
>>> +
>>> +	TEST(setgid(nobody->pw_gid));
>>> +	if (TEST_RETURN<  0&&  TEST_ERRNO != EPERM)
>>> +		tst_res(TBROK | TTERRNO, "Failed to use nobody gid");
>>                  ^
>> 		Shouldn't this be tst_brk()?
>>
>>> +	TEST(setuid(nobody->pw_uid));
>>> +	if (TEST_RETURN<  0&&  TEST_ERRNO != EPERM)
>>> +		tst_res(TBROK | TTERRNO, "Failed to use nobody uid");
>>                  ^
>> 		And here as well?
>>
>> Otherwise it looks fine.
> Well spotted, yes it should.
>

[LTP] [PATCH] read_all: Drop privileges

[Cyril Hrubis]

Hi!
> If the permission of /dev/watchdog was 0660(default permission on RHEL6), Reading /dev/watchdog as nobody
> user failed, but still led to system reboot.

If unprivileged user can reboot the system it's a bug.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all: Drop privileges

[Xiao Yang]

On 2018/05/16 19:44, Cyril Hrubis wrote:
> Hi!
>> If the permission of /dev/watchdog was 0660(default permission on RHEL6), Reading /dev/watchdog as nobody
>> user failed, but still led to system reboot.
> If unprivileged user can reboot the system it's a bug.
Hi Cyril,

Sorry, it seems a bug in open(2) instead of watchdog.

You can reproduce the issue by running the following test.c:
----------------------------------------------------------------------------------------------------------
#include <errno.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <pwd.h>
#include <unistd.h>
#include <stdlib.h>

static void switch_privs(void)
{
         struct passwd *nobody;
         int ret;

         nobody = getpwnam("nobody");
         if (nobody == NULL) {
                 printf("getpwnam(nobody) failed with errno %d\n", errno);
                 exit(1);
         }

         ret = setgid(nobody->pw_gid);
         if (ret < 0) {
                 printf("Failed to use nobody gid with errno %d\n", errno);
                 exit(1);
         }

         ret = setuid(nobody->pw_uid);
         if (ret < 0) {
                 printf("Failed to use nobody uid with errno %d\n", errno);
                 exit(1);
         }
}

int main(void)
{
         int fd;

         umask(0);

         fd = open("testfile", O_RDWR | O_CREAT, 0660);
         if (fd < 0) {
                 printf("open(testfile) failed with errno %d\n", errno);
                 return 1;
         }

         close(fd);

         switch_privs();

         fd = open("testfile", O_RDWR);
         if (fd < 0) {
                 printf("open(testfile) failed with errno %d\n", errno);
                 return 1;
         }

         printf("open(testfile) succeeded unexpectedly\n");

         close(fd);
}
------------------------------------------------------------------------------------------------------------
# gcc -o test test.c
# ./test
open(testfile) succeeded unexpectedly

We created a test file with 0660 mode as root user, and opened the test 
file as nobody user switched by setuid() and setgid().
Running this test got success rather than EACCES.  Do you think this is 
a bug or i misunderstand the permissions of file?

Thanks,
Xiao Yang

[LTP] [PATCH] read_all: Drop privileges

[Cyril Hrubis]

Hi!
> Sorry, it seems a bug in open(2) instead of watchdog.

Looks like the list of supplementary groups is at fault here.

On my system I do have in /etc/group:

root:x:0:root

Which means that among other groups root has root suplementary group set
when logged in.

Which means that even when a program sets it's user and group ids to
nobody the root still stays in the list of supplementary groups, which
then is matched for files with root group ownership and hence we can
stil open the file.

Adding setgroups(0, NULL); to switch_privs() in your program "fixes" the
behavior and we get EPERM as expected. And I guess that we should patch
the read_all to do the same, which should fix your problem. I will apply
the fix.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] read_all: Drop privileges

[Xiao Yang]

On 2018/05/19 1:09, Cyril Hrubis wrote:
> Hi!
>> Sorry, it seems a bug in open(2) instead of watchdog.
> Looks like the list of supplementary groups is at fault here.
>
> On my system I do have in /etc/group:
>
> root:x:0:root
>
> Which means that among other groups root has root suplementary group set
> when logged in.
>
> Which means that even when a program sets it's user and group ids to
> nobody the root still stays in the list of supplementary groups, which
> then is matched for files with root group ownership and hence we can
> stil open the file.
>
> Adding setgroups(0, NULL); to switch_privs() in your program "fixes" the
> behavior and we get EPERM as expected. And I guess that we should patch
> the read_all to do the same, which should fix your problem. I will apply
> the fix.
Hi Cyril,

Thanks for your detailed explanation.
I will send the fix patch as you suggested.

Thanks,
Xiao Yang

[LTP] [PATCH] fs/read_all: Clear suplementary groups before droping privileges

[Xiao Yang]

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

[LTP] [PATCH] fs/read_all: Clear suplementary groups before droping privileges

[Richard Palethorpe]

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!

-- 
Thank you,
Richard.

[LTP] [PATCH] fs/read_all: Clear suplementary groups before droping privileges

[Cyril Hrubis]

Hi!
I've added #include <grp.h> to the patch so that we get definition for
the setgrp() call and pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] fs/read_all: Clear suplementary groups before droping privileges

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz