[PATCH v1 0/6] seccomp test fixes

[PATCH v1 0/6] seccomp test fixes

[Tycho Andersen]

Hi all,

Here are the fixes I previously mentioned I would send. I previously
assumed that the tests were mostly run as root, but it turns out
everything else besides the stuff I wrote in the seccomp tests either
sets NNP and doesn't require real root, so it all actually works. This
set of fixes should make most of the other tests work unprivileged,
while XFAIL-ing the one that requires real root.

Cheers,

Tycho

Tycho Andersen (6):
  selftests: don't kill child immediately in get_metadata() test
  selftests: fix typo in seccomp_bpf.c
  selftest: include stdio.h in kselftest.h
  selftests: skip seccomp get_metadata test if not real root
  selftests: set NO_NEW_PRIVS bit in seccomp user tests
  selftests: unshare userns in seccomp pidns testcases

 tools/testing/selftests/kselftest.h           |  1 +
 tools/testing/selftests/seccomp/seccomp_bpf.c | 42 ++++++++++++++++---
 2 files changed, 38 insertions(+), 5 deletions(-)

-- 
2.19.1

[PATCH 1/6] selftests: don't kill child immediately in get_metadata() test

[Tycho Andersen]

This this test forks a child, and then the parent waits for a write() to a
pipe signalling the child is ready to be attached to. If something in the
child ASSERTs before it does this write, the test will hang waiting for it.
Instead, let's EXPECT, so that execution continues until we do the write.
Any failure after that is fine and can ASSERT.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 496a9a8c773a..9aba1b904089 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -2943,11 +2943,11 @@ TEST(get_metadata)
 		};
 
 		/* one with log, one without */
-		ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER,
+		EXPECT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER,
 				     SECCOMP_FILTER_FLAG_LOG, &prog));
-		ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog));
+		EXPECT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog));
 
-		ASSERT_EQ(0, close(pipefd[0]));
+		EXPECT_EQ(0, close(pipefd[0]));
 		ASSERT_EQ(1, write(pipefd[1], "1", 1));
 		ASSERT_EQ(0, close(pipefd[1]));
 
-- 
2.19.1

[PATCH 2/6] selftests: fix typo in seccomp_bpf.c

[Tycho Andersen]

There used to be an explanation here because it could trigger lockdep
previously, but now we're not doing recursive locking, so it really is just
for grins.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 9aba1b904089..912a2a5430dc 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3035,7 +3035,7 @@ TEST(user_notification_basic)
 	EXPECT_EQ(true, WIFEXITED(status));
 	EXPECT_EQ(0, WEXITSTATUS(status));
 
-	/* Add some no-op filters so for grins. */
+	/* Add some no-op filters for grins. */
 	EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
 	EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
 	EXPECT_EQ(seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog), 0);
-- 
2.19.1

[PATCH 3/6] selftest: include stdio.h in kselftest.h

[Tycho Andersen]

While playing around with a way to skip the seccomp get_metadata test, I
noticed that this header uses printf() without defining it, leading to,

../kselftest.h: In function ‘ksft_print_header’:
../kselftest.h:61:3: warning: implicit declaration of function ‘printf’ [-Wimplicit-function-declaration]
   printf("TAP version 13\n");
   ^~~~~~
../kselftest.h:61:3: warning: incompatible implicit declaration of built-in function ‘printf’
../kselftest.h:61:3: note: include ‘<stdio.h>’ or provide a declaration of ‘printf’

if user code doesn't also use printf.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/kselftest.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index a3edb2c8e43d..47e1d995c182 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -13,6 +13,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <stdarg.h>
+#include <stdio.h>
 
 /* define kselftest exit codes */
 #define KSFT_PASS  0
-- 
2.19.1

[PATCH 4/6] selftests: skip seccomp get_metadata test if not real root

[Tycho Andersen]

The get_metadata() test requires real root, so let's skip it if we're not
real root.

Note that I used XFAIL here because that's what the test does later if
CONFIG_CHEKCKPOINT_RESTORE happens to not be enabled. After looking at the
code, there doesn't seem to be a nice way to skip tests defined as TEST(),
since there's no return code (I tried exit(KSFT_SKIP), but that didn't work
either...). So let's do it this way to be consistent, and easier to fix
when someone comes along and fixes it.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 912a2a5430dc..ab6b6620f522 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -2929,6 +2929,12 @@ TEST(get_metadata)
 	struct seccomp_metadata md;
 	long ret;
 
+	/* Only real root can get metadata. */
+	if (geteuid()) {
+		XFAIL(return, "get_metadata requires real root");
+		return;
+	}
+
 	ASSERT_EQ(0, pipe(pipefd));
 
 	pid = fork();
-- 
2.19.1

[PATCH 5/6] selftests: set NO_NEW_PRIVS bit in seccomp user tests

[Tycho Andersen]

seccomp() doesn't allow users who aren't root in their userns to attach
filters unless they have the nnp bit set, so let's set it so that these
tests can pass when run as an unprivileged user.

This idea stolen from the other seccomp tests, which use this trick :)

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index ab6b6620f522..a4a7dce1a91b 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3026,6 +3026,11 @@ TEST(user_notification_basic)
 		.filter = filter,
 	};
 
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
 	pid = fork();
 	ASSERT_GE(pid, 0);
 
@@ -3107,6 +3112,11 @@ TEST(user_notification_kill_in_middle)
 	struct seccomp_notif req = {};
 	struct seccomp_notif_resp resp = {};
 
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
 	listener = user_trap_syscall(__NR_getpid,
 				     SECCOMP_FILTER_FLAG_NEW_LISTENER);
 	ASSERT_GE(listener, 0);
@@ -3154,6 +3164,11 @@ TEST(user_notification_signal)
 	struct seccomp_notif_resp resp = {};
 	char c;
 
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
 	ASSERT_EQ(socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair), 0);
 
 	listener = user_trap_syscall(__NR_gettid,
@@ -3219,6 +3234,11 @@ TEST(user_notification_closed_listener)
 	long ret;
 	int status, listener;
 
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
 	listener = user_trap_syscall(__NR_getpid,
 				     SECCOMP_FILTER_FLAG_NEW_LISTENER);
 	ASSERT_GE(listener, 0);
@@ -3350,6 +3370,10 @@ TEST(user_notification_fault_recv)
 	struct seccomp_notif req = {};
 	struct seccomp_notif_resp resp = {};
 
+	ASSERT_EQ(prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0), 0) {
+		TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+	}
+
 	listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
 	ASSERT_GE(listener, 0);
 
-- 
2.19.1

[PATCH 6/6] selftests: unshare userns in seccomp pidns testcases

[Tycho Andersen]

The pid ns cannot be unshare()d as an unprivileged user without owning the
userns as well. Let's unshare the userns so that we can subsequently
unshare the pidns.

This also means that we don't need to set the no new privs bit as in the
other test cases, since we're unsharing the userns.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index a4a7dce1a91b..8f6e95773225 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3271,7 +3271,7 @@ TEST(user_notification_child_pid_ns)
 	struct seccomp_notif req = {};
 	struct seccomp_notif_resp resp = {};
 
-	ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+	ASSERT_EQ(unshare(CLONE_NEWUSER | CLONE_NEWPID), 0);
 
 	listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
 	ASSERT_GE(listener, 0);
@@ -3308,6 +3308,8 @@ TEST(user_notification_sibling_pid_ns)
 	struct seccomp_notif req = {};
 	struct seccomp_notif_resp resp = {};
 
+	ASSERT_EQ(unshare(CLONE_NEWUSER), 0);
+
 	listener = user_trap_syscall(__NR_getpid, SECCOMP_FILTER_FLAG_NEW_LISTENER);
 	ASSERT_GE(listener, 0);
 
-- 
2.19.1

Re: [PATCH v1 0/6] seccomp test fixes

[Kees Cook]

On Fri, Jan 18, 2019 at 4:12 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> Hi all,
>
> Here are the fixes I previously mentioned I would send. I previously
> assumed that the tests were mostly run as root, but it turns out
> everything else besides the stuff I wrote in the seccomp tests either
> sets NNP and doesn't require real root, so it all actually works. This
> set of fixes should make most of the other tests work unprivileged,
> while XFAIL-ing the one that requires real root.

Awesome. This all looks good to me. :)

Acked-by: Kees Cook <keescook@chromium.org>

Shuah, can you take this series?

-Kees

>
> Cheers,
>
> Tycho
>
> Tycho Andersen (6):
>   selftests: don't kill child immediately in get_metadata() test
>   selftests: fix typo in seccomp_bpf.c
>   selftest: include stdio.h in kselftest.h
>   selftests: skip seccomp get_metadata test if not real root
>   selftests: set NO_NEW_PRIVS bit in seccomp user tests
>   selftests: unshare userns in seccomp pidns testcases
>
>  tools/testing/selftests/kselftest.h           |  1 +
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 42 ++++++++++++++++---
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> --
> 2.19.1
>


-- 
Kees Cook

Re: [PATCH v1 0/6] seccomp test fixes

[shuah]

On 1/19/19 5:43 PM, Kees Cook wrote:
> On Fri, Jan 18, 2019 at 4:12 PM Tycho Andersen <tycho@tycho.ws> wrote:
>>
>> Hi all,
>>
>> Here are the fixes I previously mentioned I would send. I previously
>> assumed that the tests were mostly run as root, but it turns out
>> everything else besides the stuff I wrote in the seccomp tests either
>> sets NNP and doesn't require real root, so it all actually works. This
>> set of fixes should make most of the other tests work unprivileged,
>> while XFAIL-ing the one that requires real root.

Tycho, Thanks for a quick response in fixing the problems.
> 
> Awesome. This all looks good to me. :)
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> Shuah, can you take this series?
> 


Yes. I will take these in for rc5.

thanks,
-- Shuah