Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580277 > unrolled thread

[PATCH v4 0/4] Improved seccomp logging

Started byTyler Hicks <tyhicks@canonical.com>
First post2017-02-14 05:00 +0100
Last post2017-02-16 02:20 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/4] Improved seccomp logging Tyler Hicks <tyhicks@canonical.com> - 2017-02-14 05:00 +0100
    [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG Tyler Hicks <tyhicks@canonical.com> - 2017-02-14 05:00 +0100
      Re: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG Kees Cook <keescook@chromium.org> - 2017-02-16 02:20 +0100
        Re: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG Tyler Hicks <tyhicks@canonical.com> - 2017-02-16 19:50 +0100
    Re: [PATCH v4 0/4] Improved seccomp logging Kees Cook <keescook@chromium.org> - 2017-02-16 02:20 +0100

#1580277 — [PATCH v4 0/4] Improved seccomp logging

FromTyler Hicks <tyhicks@canonical.com>
Date2017-02-14 05:00 +0100
Subject[PATCH v4 0/4] Improved seccomp logging
Message-ID<taCrv-4Mc-3@gated-at.bofh.it>
This patch set is the fourth revision of the following two previously
submitted patch sets:

v1: http://lkml.kernel.org/r/1483375990-14948-1-git-send-email-tyhicks@canonical.com
v1: http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyhicks@canonical.com

v2: http://lkml.kernel.org/r/1486100262-32391-1-git-send-email-tyhicks@canonical.com

v3: Same patches as v4 but I copied and pasted an invalid address for the
    linux-api list when submitting the set.

The patch set aims to address some known deficiencies in seccomp's current
logging capabilities:

  1. Inability to log all filter actions.
  2. Inability to selectively enable filtering; e.g. devs want noisy logging,
     users want relative quiet.
  3. Consistent behavior with audit enabled and disabled.
  4. Inability to easily develop a filter due to the lack of a
     permissive/complain mode.

Changes since v3:
- No code changes. I had to resubmit the patch set after copying and
  pasting a bad address for the linux-api list.

Changes since v2 to address feedback from Kees:
- Patch 1
  + Log a warning when sysctl registration fails
  + Move comment describing SECCOMP_RET_*_NAME from PATCH 2
  + Document the actions_avail sysctl
- Patch 2
  + Inline seccomp_log()
  + Optimize logging for RET_ALLOW hot path
  + Use "{ }" for name buffer initialization
  + Make a copy of the ctl_table and only modify the copy
  + Rename max_action_to_log sysctl to log_max_action
  + Document the log_max_action sysctl
- Patch 3
  + Put some space between RET_LOG and RET_ALLOW for future actions
  + Separate the RET_ALLOW and RET_LOG cases in __seccomp_filter()
- Patch 4
  + Adjust the selftests for the updated RET_LOG value

Tyler


Tyler Hicks (4):
  seccomp: Add sysctl to display available actions
  seccomp: Add sysctl to configure actions that should be logged
  seccomp: Create an action to log before allowing
  seccomp: Add tests for SECCOMP_RET_LOG

 Documentation/prctl/seccomp_filter.txt        |  43 ++++++
 Documentation/sysctl/kernel.txt               |   1 +
 include/linux/audit.h                         |   6 +-
 include/uapi/linux/seccomp.h                  |   1 +
 kernel/seccomp.c                              | 185 +++++++++++++++++++++++++-
 tools/testing/selftests/seccomp/seccomp_bpf.c |  94 +++++++++++++
 6 files changed, 322 insertions(+), 8 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1580279 — [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG

FromTyler Hicks <tyhicks@canonical.com>
Date2017-02-14 05:00 +0100
Subject[PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG
Message-ID<taCrw-4Mc-19@gated-at.bofh.it>
In reply to#1580277
Extend the kernel selftests for seccomp to test the newly added
SECCOMP_RET_LOG action. The added tests follow the example of existing
tests.

Unfortunately, the tests are not capable of inspecting the audit log to
verify that the syscall was logged.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 94 +++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 03f1fa4..5633b42 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -87,6 +87,10 @@ struct seccomp_data {
 };
 #endif
 
+#ifndef SECCOMP_RET_LOG
+#define SECCOMP_RET_LOG       0x7ffc0000U /* allow after logging */
+#endif
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 #define syscall_arg(_n) (offsetof(struct seccomp_data, args[_n]))
 #elif __BYTE_ORDER == __BIG_ENDIAN
@@ -342,6 +346,28 @@ TEST(empty_prog)
 	EXPECT_EQ(EINVAL, errno);
 }
 
+TEST(log_all)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_LOG),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+	long ret;
+	pid_t parent = getppid();
+
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog);
+	ASSERT_EQ(0, ret);
+
+	/* getppid() should succeed and be logged (no check for logging) */
+	EXPECT_EQ(parent, syscall(__NR_getppid));
+}
+
 TEST_SIGNAL(unknown_ret_is_kill_inside, SIGSYS)
 {
 	struct sock_filter filter[] = {
@@ -735,6 +761,7 @@ TEST_F(TRAP, handler)
 
 FIXTURE_DATA(precedence) {
 	struct sock_fprog allow;
+	struct sock_fprog log;
 	struct sock_fprog trace;
 	struct sock_fprog error;
 	struct sock_fprog trap;
@@ -746,6 +773,13 @@ FIXTURE_SETUP(precedence)
 	struct sock_filter allow_insns[] = {
 		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
 	};
+	struct sock_filter log_insns[] = {
+		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+			offsetof(struct seccomp_data, nr)),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 1, 0),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_LOG),
+	};
 	struct sock_filter trace_insns[] = {
 		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
 			offsetof(struct seccomp_data, nr)),
@@ -782,6 +816,7 @@ FIXTURE_SETUP(precedence)
 	memcpy(self->_x.filter, &_x##_insns, sizeof(_x##_insns)); \
 	self->_x.len = (unsigned short)ARRAY_SIZE(_x##_insns)
 	FILTER_ALLOC(allow);
+	FILTER_ALLOC(log);
 	FILTER_ALLOC(trace);
 	FILTER_ALLOC(error);
 	FILTER_ALLOC(trap);
@@ -792,6 +827,7 @@ FIXTURE_TEARDOWN(precedence)
 {
 #define FILTER_FREE(_x) if (self->_x.filter) free(self->_x.filter)
 	FILTER_FREE(allow);
+	FILTER_FREE(log);
 	FILTER_FREE(trace);
 	FILTER_FREE(error);
 	FILTER_FREE(trap);
@@ -809,6 +845,8 @@ TEST_F(precedence, allow_ok)
 
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
@@ -833,6 +871,8 @@ TEST_F_SIGNAL(precedence, kill_is_highest, SIGSYS)
 
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
@@ -864,6 +904,8 @@ TEST_F_SIGNAL(precedence, kill_is_highest_in_any_order, SIGSYS)
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trap);
@@ -885,6 +927,8 @@ TEST_F_SIGNAL(precedence, trap_is_second, SIGSYS)
 
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
@@ -910,6 +954,8 @@ TEST_F_SIGNAL(precedence, trap_is_second_in_any_order, SIGSYS)
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trap);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
@@ -931,6 +977,8 @@ TEST_F(precedence, errno_is_third)
 
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
@@ -949,6 +997,8 @@ TEST_F(precedence, errno_is_third_in_any_order)
 	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
 	ASSERT_EQ(0, ret);
 
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->error);
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
@@ -971,6 +1021,8 @@ TEST_F(precedence, trace_is_fourth)
 
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->trace);
 	ASSERT_EQ(0, ret);
 	/* Should work just fine. */
@@ -992,12 +1044,54 @@ TEST_F(precedence, trace_is_fourth_in_any_order)
 	ASSERT_EQ(0, ret);
 	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
 	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
 	/* Should work just fine. */
 	EXPECT_EQ(parent, syscall(__NR_getppid));
 	/* No ptracer */
 	EXPECT_EQ(-1, syscall(__NR_getpid));
 }
 
+TEST_F(precedence, log_is_fifth)
+{
+	pid_t mypid, parent;
+	long ret;
+
+	mypid = getpid();
+	parent = getppid();
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
+	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
+	/* Should work just fine. */
+	EXPECT_EQ(parent, syscall(__NR_getppid));
+	/* Should also work just fine */
+	EXPECT_EQ(mypid, syscall(__NR_getpid));
+}
+
+TEST_F(precedence, log_is_fifth_in_any_order)
+{
+	pid_t mypid, parent;
+	long ret;
+
+	mypid = getpid();
+	parent = getppid();
+	ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
+	ASSERT_EQ(0, ret);
+
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->log);
+	ASSERT_EQ(0, ret);
+	ret = prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &self->allow);
+	ASSERT_EQ(0, ret);
+	/* Should work just fine. */
+	EXPECT_EQ(parent, syscall(__NR_getppid));
+	/* Should also work just fine */
+	EXPECT_EQ(mypid, syscall(__NR_getpid));
+}
+
 #ifndef PTRACE_O_TRACESECCOMP
 #define PTRACE_O_TRACESECCOMP	0x00000080
 #endif
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582208 — Re: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG

FromKees Cook <keescook@chromium.org>
Date2017-02-16 02:20 +0100
SubjectRe: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG
Message-ID<tbiTL-7zZ-9@gated-at.bofh.it>
In reply to#1580279
On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> Extend the kernel selftests for seccomp to test the newly added
> SECCOMP_RET_LOG action. The added tests follow the example of existing
> tests.
>
> Unfortunately, the tests are not capable of inspecting the audit log to
> verify that the syscall was logged.

It could try to read dmesg... :P But I don't need that for this patch,
though maybe add it to the test's TODO list?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1582856 — Re: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG

FromTyler Hicks <tyhicks@canonical.com>
Date2017-02-16 19:50 +0100
SubjectRe: [PATCH v4 4/4] seccomp: Add tests for SECCOMP_RET_LOG
Message-ID<tbzhU-1Tc-17@gated-at.bofh.it>
In reply to#1582208

[Multipart message — attachments visible in raw view] — view raw

On 02/15/2017 07:13 PM, Kees Cook wrote:
> On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Extend the kernel selftests for seccomp to test the newly added
>> SECCOMP_RET_LOG action. The added tests follow the example of existing
>> tests.
>>
>> Unfortunately, the tests are not capable of inspecting the audit log to
>> verify that the syscall was logged.
> 
> It could try to read dmesg... :P But I don't need that for this patch,
> though maybe add it to the test's TODO list?

Sure thing!

Tyler

> 
> -Kees
>

[toc] | [prev] | [next] | [standalone]

#1582210

FromKees Cook <keescook@chromium.org>
Date2017-02-16 02:20 +0100
Message-ID<tbiTM-7zZ-11@gated-at.bofh.it>
In reply to#1580277
On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set is the fourth revision of the following two previously
> submitted patch sets:
>
> v1: http://lkml.kernel.org/r/1483375990-14948-1-git-send-email-tyhicks@canonical.com
> v1: http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyhicks@canonical.com
>
> v2: http://lkml.kernel.org/r/1486100262-32391-1-git-send-email-tyhicks@canonical.com
>
> v3: Same patches as v4 but I copied and pasted an invalid address for the
>     linux-api list when submitting the set.
>
> The patch set aims to address some known deficiencies in seccomp's current
> logging capabilities:
>
>   1. Inability to log all filter actions.
>   2. Inability to selectively enable filtering; e.g. devs want noisy logging,
>      users want relative quiet.
>   3. Consistent behavior with audit enabled and disabled.
>   4. Inability to easily develop a filter due to the lack of a
>      permissive/complain mode.

I'm pretty happy with the series! If you can address the minor nits I
pointed out and send a v4, I'll get it queued for -next. It won't make
v4.11; we're almost at the merge window and I want to make sure this
gets as much testing time as possible. It should be good for v4.12,
though.

Thanks for the series; I know a lot of people have wanted the functionality. :)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web