[PATCH v2 0/4] Improved seccomp logging

[PATCH v2 0/4] Improved seccomp logging

[Tyler Hicks]

This patch set is the second revision of the following two previously
submitted patch sets:

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

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.

The first three items were outlined by Paul Moore and are issues that I agree
with. The last one is one that I'm particularly interested in.

I deviated a little from the plan that he laid out to address the third issue.
Looking back at Paul's feedback, he wanted a way to log seccomp actions even
when the audit subsystem is disabled at build time. I felt like the bigger
problem is that, while it is common for kernels to be built with audit support,
it is far less common to actually have auditd running. Therefore, my approach
was to improve the situation when kernel audit support is enabled at build time
but audit_enabled is false at runtime. The audit subsystem forwards messages
onto syslog in that situation.

Tyler

[PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Tyler Hicks]

This patch creates a read-only sysctl containing an ordered list of
seccomp actions that the kernel supports. The ordering, from left to
right, is the lowest action value (kill) to the highest action value
(allow). Currently, a read of the sysctl file would return "kill trap
errno trace allow". The contents of this sysctl file can be useful for
userspace code as well as the system administrator.

The path to the sysctl is:

  /proc/sys/kernel/seccomp/actions_avail

libseccomp and other userspace code can easily determine which actions
the current kernel supports. The set of actions supported by the current
kernel may be different than the set of action macros found in kernel
headers that were installed where the userspace code was built.

In addition, this sysctl will allow system administrators to know which
actions are supported by the kernel and make it easier to configure
exactly what seccomp logs through the audit subsystem. Support for this
level of logging configuration will come in a future patch.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 kernel/seccomp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f7ce79a..919ad9f 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -16,10 +16,12 @@
 #include <linux/atomic.h>
 #include <linux/audit.h>
 #include <linux/compat.h>
+#include <linux/kmemleak.h>
 #include <linux/sched.h>
 #include <linux/seccomp.h>
 #include <linux/slab.h>
 #include <linux/syscalls.h>
+#include <linux/sysctl.h>
 
 #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
 #include <asm/syscall.h>
@@ -905,3 +907,51 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 	return ret;
 }
 #endif
+
+#ifdef CONFIG_SYSCTL
+
+#define SECCOMP_RET_KILL_NAME		"kill"
+#define SECCOMP_RET_TRAP_NAME		"trap"
+#define SECCOMP_RET_ERRNO_NAME		"errno"
+#define SECCOMP_RET_TRACE_NAME		"trace"
+#define SECCOMP_RET_ALLOW_NAME		"allow"
+
+static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME	" "
+				      SECCOMP_RET_TRAP_NAME	" "
+				      SECCOMP_RET_ERRNO_NAME	" "
+				      SECCOMP_RET_TRACE_NAME	" "
+				      SECCOMP_RET_ALLOW_NAME;
+
+static struct ctl_path seccomp_sysctl_path[] = {
+	{ .procname = "kernel", },
+	{ .procname = "seccomp", },
+	{ }
+};
+
+static struct ctl_table seccomp_sysctl_table[] = {
+	{
+		.procname	= "actions_avail",
+		.data		= &seccomp_actions_avail,
+		.maxlen		= sizeof(seccomp_actions_avail),
+		.mode		= 0444,
+		.proc_handler	= proc_dostring,
+	},
+	{ }
+};
+
+static int __init seccomp_sysctl_init(void)
+{
+	struct ctl_table_header *hdr;
+
+	hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
+	kmemleak_not_leak(hdr);
+	return 0;
+}
+
+#else /* CONFIG_SYSCTL */
+
+static __init int seccomp_sysctl_init(void) { return 0; }
+
+#endif /* CONFIG_SYSCTL */
+
+device_initcall(seccomp_sysctl_init)
-- 
2.7.4

[PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged

[Tyler Hicks]

Administrators can write to this sysctl to set the maximum seccomp
action that should be logged. Any actions with values greater than
what's written to the sysctl will not be logged.

For example, all SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
SECCOMP_RET_ERRNO actions would be logged if "errno" were written to the
sysctl. SECCOMP_RET_TRACE and SECCOMP_RET_ALLOW actions would not be
logged since their values are higher than SECCOMP_RET_ERRNO.

The path to the sysctl is:

 /proc/sys/kernel/seccomp/max_action_to_log

The actions_avail sysctl can be read to discover the valid action names
that can be written to the max_action_to_log sysctl. The actions_avail
sysctl is also useful in understanding the ordering of actions used when
deciding the maximum action to log.

The default setting for the sysctl is to only log SECCOMP_RET_KILL
actions which matches the existing behavior.

There's one important exception to this sysctl. If a task is
specifically being audited, meaning that an audit context has been
allocated for the task, seccomp will log all actions other than
SECCOMP_RET_ALLOW despite the value of max_action_to_log. This exception
preserves the existing auditing behavior of tasks with an allocated
audit context.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 include/linux/audit.h |   6 +--
 kernel/seccomp.c      | 114 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index f51fca8d..e0d95fc 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -315,11 +315,7 @@ void audit_core_dumps(long signr);
 
 static inline void audit_seccomp(unsigned long syscall, long signr, int code)
 {
-	if (!audit_enabled)
-		return;
-
-	/* Force a record to be reported if a signal was delivered. */
-	if (signr || unlikely(!audit_dummy_context()))
+	if (audit_enabled && unlikely(!audit_dummy_context()))
 		__audit_seccomp(syscall, signr, code);
 }
 
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 919ad9f..548fb89 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -509,6 +509,24 @@ static void seccomp_send_sigsys(int syscall, int reason)
 }
 #endif	/* CONFIG_SECCOMP_FILTER */
 
+static u32 seccomp_max_action_to_log = SECCOMP_RET_KILL;
+
+static void seccomp_log(unsigned long syscall, long signr, u32 action)
+{
+	/* Force an audit message to be emitted when the action is not greater
+	 * than the configured maximum action.
+	 */
+	if (action <= seccomp_max_action_to_log)
+		return __audit_seccomp(syscall, signr, action);
+
+	/* If the action is not an ALLOW action, let the audit subsystem decide
+	 * if it should be audited based on whether the current task itself is
+	 * being audited.
+	 */
+	if (action != SECCOMP_RET_ALLOW)
+		return audit_seccomp(syscall, signr, action);
+}
+
 /*
  * Secure computing mode 1 allows only read/write/exit/sigreturn.
  * To be fully secure this must be combined with rlimit
@@ -534,7 +552,7 @@ static void __secure_computing_strict(int this_syscall)
 #ifdef SECCOMP_DEBUG
 	dump_stack();
 #endif
-	audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
+	seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
 	do_exit(SIGKILL);
 }
 
@@ -633,18 +651,19 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		return 0;
 
 	case SECCOMP_RET_ALLOW:
+		seccomp_log(this_syscall, 0, action);
 		return 0;
 
 	case SECCOMP_RET_KILL:
 	default:
-		audit_seccomp(this_syscall, SIGSYS, action);
+		seccomp_log(this_syscall, SIGSYS, action);
 		do_exit(SIGSYS);
 	}
 
 	unreachable();
 
 skip:
-	audit_seccomp(this_syscall, 0, action);
+	seccomp_log(this_syscall, 0, action);
 	return -1;
 }
 #else
@@ -910,18 +929,102 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 
 #ifdef CONFIG_SYSCTL
 
+/* Human readable action names for friendly sysctl interaction */
 #define SECCOMP_RET_KILL_NAME		"kill"
 #define SECCOMP_RET_TRAP_NAME		"trap"
 #define SECCOMP_RET_ERRNO_NAME		"errno"
 #define SECCOMP_RET_TRACE_NAME		"trace"
 #define SECCOMP_RET_ALLOW_NAME		"allow"
 
+/* Largest strlen() of all action names */
+#define SECCOMP_RET_MAX_NAME_LEN	5
+
 static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME	" "
 				      SECCOMP_RET_TRAP_NAME	" "
 				      SECCOMP_RET_ERRNO_NAME	" "
 				      SECCOMP_RET_TRACE_NAME	" "
 				      SECCOMP_RET_ALLOW_NAME;
 
+struct seccomp_action_name {
+	u32		action;
+	const char	*name;
+};
+
+static struct seccomp_action_name seccomp_action_names[] = {
+	{ SECCOMP_RET_KILL, SECCOMP_RET_KILL_NAME },
+	{ SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
+	{ SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
+	{ SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
+	{ SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
+	{ }
+};
+
+static bool seccomp_name_from_action(const char **namep, u32 action)
+{
+	struct seccomp_action_name *cur;
+
+	for (cur = seccomp_action_names; cur->name; cur++) {
+		if (cur->action == action) {
+			*namep = cur->name;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static bool seccomp_action_from_name(u32 *action, const char *name)
+{
+	struct seccomp_action_name *cur;
+
+	for (cur = seccomp_action_names; cur->name; cur++) {
+		if (!strcmp(cur->name, name)) {
+			*action = cur->action;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static int seccomp_max_action_to_log_handler(struct ctl_table *table, int write,
+					     void __user *buffer, size_t *lenp,
+					     loff_t *ppos)
+{
+	char name[SECCOMP_RET_MAX_NAME_LEN + 1] = {0};
+	int ret;
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!write) {
+		const char *namep;
+
+		if (!seccomp_name_from_action(&namep,
+					      seccomp_max_action_to_log))
+			return -EINVAL;
+
+		strncpy(name, namep, sizeof(name) - 1);
+	}
+
+	table->data = name;
+	table->maxlen = sizeof(name);
+	ret = proc_dostring(table, write, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+
+	if (write) {
+		u32 action;
+
+		if (!seccomp_action_from_name(&action, table->data))
+			return -EINVAL;
+
+		seccomp_max_action_to_log = action;
+	}
+
+	return 0;
+}
+
 static struct ctl_path seccomp_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "seccomp", },
@@ -936,6 +1039,11 @@ static struct ctl_table seccomp_sysctl_table[] = {
 		.mode		= 0444,
 		.proc_handler	= proc_dostring,
 	},
+	{
+		.procname	= "max_action_to_log",
+		.mode		= 0644,
+		.proc_handler	= seccomp_max_action_to_log_handler,
+	},
 	{ }
 };
 
-- 
2.7.4

[PATCH v2 3/4] seccomp: Create an action to log before allowing

[Tyler Hicks]

Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
the syscall. At the implementation level, this action is identical to
the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
initially developing a seccomp filter for an application. The developer
can set the default action to be SECCOMP_RET_LOG, maybe mark any
obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
application through its paces. A list of syscalls that triggered the
default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
that list can be used to build the syscall whitelist. Finally, the
developer can change the default action to the desired value.

This provides a more friendly experience than seeing the application get
killed, then updating the filter and rebuilding the app, seeing the
application get killed due to a different syscall, then updating the
filter and rebuilding the app, etc.

The functionality is similar to what's supported by the various LSMs.
SELinux has permissive mode, AppArmor has complain mode, SMACK has
bring-up mode, etc.

SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
"allow" can be written to the max_action_to_log sysctl in order to get a
list of logged actions without the, potentially larger, set of allowed
actions.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 Documentation/prctl/seccomp_filter.txt | 6 ++++++
 include/uapi/linux/seccomp.h           | 1 +
 kernel/seccomp.c                       | 4 ++++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
index 1e469ef..ba55a91 100644
--- a/Documentation/prctl/seccomp_filter.txt
+++ b/Documentation/prctl/seccomp_filter.txt
@@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
 	allow use of ptrace, even of other sandboxed processes, without
 	extreme care; ptracers can use this mechanism to escape.)
 
+SECCOMP_RET_LOG:
+	Results in the system call being executed after it is logged. This
+	should be used by application developers to learn which syscalls their
+	application needs without having to iterate through multiple test and
+	development cycles to build the list.
+
 SECCOMP_RET_ALLOW:
 	Results in the system call being executed.
 
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 0f238a4..67f72cd 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -29,6 +29,7 @@
 #define SECCOMP_RET_TRAP	0x00030000U /* disallow and force a SIGSYS */
 #define SECCOMP_RET_ERRNO	0x00050000U /* returns an errno */
 #define SECCOMP_RET_TRACE	0x7ff00000U /* pass to a tracer or disallow */
+#define SECCOMP_RET_LOG		0x7ffe0000U /* allow after logging */
 #define SECCOMP_RET_ALLOW	0x7fff0000U /* allow */
 
 /* Masks for the return value sections. */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 548fb89..8627481 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 
 		return 0;
 
+	case SECCOMP_RET_LOG:
 	case SECCOMP_RET_ALLOW:
 		seccomp_log(this_syscall, 0, action);
 		return 0;
@@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 #define SECCOMP_RET_TRAP_NAME		"trap"
 #define SECCOMP_RET_ERRNO_NAME		"errno"
 #define SECCOMP_RET_TRACE_NAME		"trace"
+#define SECCOMP_RET_LOG_NAME		"log"
 #define SECCOMP_RET_ALLOW_NAME		"allow"
 
 /* Largest strlen() of all action names */
@@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME	" "
 				      SECCOMP_RET_TRAP_NAME	" "
 				      SECCOMP_RET_ERRNO_NAME	" "
 				      SECCOMP_RET_TRACE_NAME	" "
+				      SECCOMP_RET_LOG_NAME	" "
 				      SECCOMP_RET_ALLOW_NAME;
 
 struct seccomp_action_name {
@@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
 	{ SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
 	{ SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
 	{ SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
+	{ SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
 	{ SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
 	{ }
 };
-- 
2.7.4

[PATCH v2 4/4] seccomp: Add tests for SECCOMP_RET_LOG

[Tyler Hicks]

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..a39f620 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       0x7ffe0000U /* 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

Re: [PATCH v2 0/4] Improved seccomp logging

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch set is the second revision of the following two previously
> submitted patch sets:
>
> http://lkml.kernel.org/r/1483375990-14948-1-git-send-email-tyhicks@canonical.com
> http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyhicks@canonical.com
>
> 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.
>
> The first three items were outlined by Paul Moore and are issues that I agree
> with. The last one is one that I'm particularly interested in.
>
> I deviated a little from the plan that he laid out to address the third issue.
> Looking back at Paul's feedback, he wanted a way to log seccomp actions even
> when the audit subsystem is disabled at build time. I felt like the bigger
> problem is that, while it is common for kernels to be built with audit support,
> it is far less common to actually have auditd running. Therefore, my approach
> was to improve the situation when kernel audit support is enabled at build time
> but audit_enabled is false at runtime. The audit subsystem forwards messages
> onto syslog in that situation.

I'm pretty happy with this series; it's pretty close to something I'd
Ack. :) I think this will get us a lot of what people have asked for
without too much pain. I'll add some thoughts on each of the specific
patches...

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> This patch creates a read-only sysctl containing an ordered list of
> seccomp actions that the kernel supports. The ordering, from left to
> right, is the lowest action value (kill) to the highest action value
> (allow). Currently, a read of the sysctl file would return "kill trap
> errno trace allow". The contents of this sysctl file can be useful for
> userspace code as well as the system administrator.
>
> The path to the sysctl is:
>
>   /proc/sys/kernel/seccomp/actions_avail
>
> libseccomp and other userspace code can easily determine which actions
> the current kernel supports. The set of actions supported by the current
> kernel may be different than the set of action macros found in kernel
> headers that were installed where the userspace code was built.

This is certainly good: having a discoverable way to detect filter
capabilities. I do wonder if it'd still be easier to just expose the
max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
all part of uapi, so we can't escape their values...



>
> In addition, this sysctl will allow system administrators to know which
> actions are supported by the kernel and make it easier to configure
> exactly what seccomp logs through the audit subsystem. Support for this
> level of logging configuration will come in a future patch.
>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  kernel/seccomp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f7ce79a..919ad9f 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -16,10 +16,12 @@
>  #include <linux/atomic.h>
>  #include <linux/audit.h>
>  #include <linux/compat.h>
> +#include <linux/kmemleak.h>
>  #include <linux/sched.h>
>  #include <linux/seccomp.h>
>  #include <linux/slab.h>
>  #include <linux/syscalls.h>
> +#include <linux/sysctl.h>
>
>  #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
>  #include <asm/syscall.h>
> @@ -905,3 +907,51 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>         return ret;
>  }
>  #endif
> +
> +#ifdef CONFIG_SYSCTL
> +
> +#define SECCOMP_RET_KILL_NAME          "kill"
> +#define SECCOMP_RET_TRAP_NAME          "trap"
> +#define SECCOMP_RET_ERRNO_NAME         "errno"
> +#define SECCOMP_RET_TRACE_NAME         "trace"
> +#define SECCOMP_RET_ALLOW_NAME         "allow"
> +
> +static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
> +                                     SECCOMP_RET_TRAP_NAME     " "
> +                                     SECCOMP_RET_ERRNO_NAME    " "
> +                                     SECCOMP_RET_TRACE_NAME    " "
> +                                     SECCOMP_RET_ALLOW_NAME;
> +
> +static struct ctl_path seccomp_sysctl_path[] = {
> +       { .procname = "kernel", },
> +       { .procname = "seccomp", },
> +       { }
> +};
> +
> +static struct ctl_table seccomp_sysctl_table[] = {
> +       {
> +               .procname       = "actions_avail",
> +               .data           = &seccomp_actions_avail,
> +               .maxlen         = sizeof(seccomp_actions_avail),
> +               .mode           = 0444,
> +               .proc_handler   = proc_dostring,
> +       },
> +       { }
> +};
> +
> +static int __init seccomp_sysctl_init(void)
> +{
> +       struct ctl_table_header *hdr;
> +
> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
> +       kmemleak_not_leak(hdr);

Will kmemleak complain about this if hdr is saved to a global (or not
saved at all)? Also, something should be reported in the failure
case...

> +       return 0;
> +}
> +
> +#else /* CONFIG_SYSCTL */
> +
> +static __init int seccomp_sysctl_init(void) { return 0; }
> +
> +#endif /* CONFIG_SYSCTL */
> +
> +device_initcall(seccomp_sysctl_init)
> --
> 2.7.4
>

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> Administrators can write to this sysctl to set the maximum seccomp
> action that should be logged. Any actions with values greater than
> what's written to the sysctl will not be logged.
>
> For example, all SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
> SECCOMP_RET_ERRNO actions would be logged if "errno" were written to the
> sysctl. SECCOMP_RET_TRACE and SECCOMP_RET_ALLOW actions would not be
> logged since their values are higher than SECCOMP_RET_ERRNO.
>
> The path to the sysctl is:
>
>  /proc/sys/kernel/seccomp/max_action_to_log

/me looks for new bikeshed paint.

How about .../seccomp/action_log ? (And a corresponding
s/max_action_to_log/action_log/, if that looks readable...) I think
four words is just too long. :)

> The actions_avail sysctl can be read to discover the valid action names
> that can be written to the max_action_to_log sysctl. The actions_avail
> sysctl is also useful in understanding the ordering of actions used when
> deciding the maximum action to log.
>
> The default setting for the sysctl is to only log SECCOMP_RET_KILL
> actions which matches the existing behavior.
>
> There's one important exception to this sysctl. If a task is
> specifically being audited, meaning that an audit context has been
> allocated for the task, seccomp will log all actions other than
> SECCOMP_RET_ALLOW despite the value of max_action_to_log. This exception
> preserves the existing auditing behavior of tasks with an allocated
> audit context.
>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  include/linux/audit.h |   6 +--
>  kernel/seccomp.c      | 114 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 112 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f51fca8d..e0d95fc 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -315,11 +315,7 @@ void audit_core_dumps(long signr);
>
>  static inline void audit_seccomp(unsigned long syscall, long signr, int code)
>  {
> -       if (!audit_enabled)
> -               return;
> -
> -       /* Force a record to be reported if a signal was delivered. */
> -       if (signr || unlikely(!audit_dummy_context()))
> +       if (audit_enabled && unlikely(!audit_dummy_context()))
>                 __audit_seccomp(syscall, signr, code);
>  }
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 919ad9f..548fb89 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -509,6 +509,24 @@ static void seccomp_send_sigsys(int syscall, int reason)
>  }
>  #endif /* CONFIG_SECCOMP_FILTER */
>
> +static u32 seccomp_max_action_to_log = SECCOMP_RET_KILL;
> +
> +static void seccomp_log(unsigned long syscall, long signr, u32 action)

Please mark this inline...

> +{
> +       /* Force an audit message to be emitted when the action is not greater
> +        * than the configured maximum action.
> +        */
> +       if (action <= seccomp_max_action_to_log)
> +               return __audit_seccomp(syscall, signr, action);
> +
> +       /* If the action is not an ALLOW action, let the audit subsystem decide
> +        * if it should be audited based on whether the current task itself is
> +        * being audited.
> +        */
> +       if (action != SECCOMP_RET_ALLOW)
> +               return audit_seccomp(syscall, signr, action);

Based on my thoughts below, this test can actually be removed (making
the audit_seccomp() call unconditional), since callers will always be
!= RET_ALLOW.

> +}
> +
>  /*
>   * Secure computing mode 1 allows only read/write/exit/sigreturn.
>   * To be fully secure this must be combined with rlimit
> @@ -534,7 +552,7 @@ static void __secure_computing_strict(int this_syscall)
>  #ifdef SECCOMP_DEBUG
>         dump_stack();
>  #endif
> -       audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
> +       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
>         do_exit(SIGKILL);
>  }
>
> @@ -633,18 +651,19 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>                 return 0;
>
>         case SECCOMP_RET_ALLOW:
> +               seccomp_log(this_syscall, 0, action);
>                 return 0;

I am extremely sensitive about anything appearing in the RET_ALLOW
case, since it's the hot path for seccomp. This adds a full function
call (which also contains a redundant test: the action IS RET_ALLOW,
so we'll never call audit_seccomp() in seccomp_log()).

While the inline request above removes the function call, it's not
clear to me if gcc is going to do the right thing here, and I'd like
to assist the branch predictor (likely separate from the other case),
so I think I'd like an open-coded test here instead:

case SECCOMP_RET_ALLOW:
    /* Open-coded seccomp_log(), optimized for RET_ALLOW. */
    if (unlikely(seccomp_max_action_to_log == 0))
        __audit_seccomp(syscall, signr, action);
    return 0;

>         case SECCOMP_RET_KILL:
>         default:
> -               audit_seccomp(this_syscall, SIGSYS, action);
> +               seccomp_log(this_syscall, SIGSYS, action);
>                 do_exit(SIGSYS);
>         }
>
>         unreachable();
>
>  skip:
> -       audit_seccomp(this_syscall, 0, action);
> +       seccomp_log(this_syscall, 0, action);
>         return -1;
>  }
>  #else
> @@ -910,18 +929,102 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>
>  #ifdef CONFIG_SYSCTL
>
> +/* Human readable action names for friendly sysctl interaction */

This line should be in patch 1.

>  #define SECCOMP_RET_KILL_NAME          "kill"
>  #define SECCOMP_RET_TRAP_NAME          "trap"
>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>  #define SECCOMP_RET_TRACE_NAME         "trace"
>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>
> +/* Largest strlen() of all action names */
> +#define SECCOMP_RET_MAX_NAME_LEN       5

This feels fragile... though I don't have a good suggestion yet. :P

> +
>  static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>                                       SECCOMP_RET_TRAP_NAME     " "
>                                       SECCOMP_RET_ERRNO_NAME    " "
>                                       SECCOMP_RET_TRACE_NAME    " "
>                                       SECCOMP_RET_ALLOW_NAME;
>
> +struct seccomp_action_name {
> +       u32             action;
> +       const char      *name;
> +};
> +
> +static struct seccomp_action_name seccomp_action_names[] = {
> +       { SECCOMP_RET_KILL, SECCOMP_RET_KILL_NAME },
> +       { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
> +       { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
> +       { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
> +       { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
> +       { }
> +};
> +
> +static bool seccomp_name_from_action(const char **namep, u32 action)
> +{
> +       struct seccomp_action_name *cur;
> +
> +       for (cur = seccomp_action_names; cur->name; cur++) {
> +               if (cur->action == action) {
> +                       *namep = cur->name;
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static bool seccomp_action_from_name(u32 *action, const char *name)
> +{
> +       struct seccomp_action_name *cur;
> +
> +       for (cur = seccomp_action_names; cur->name; cur++) {
> +               if (!strcmp(cur->name, name)) {
> +                       *action = cur->action;
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static int seccomp_max_action_to_log_handler(struct ctl_table *table, int write,
> +                                            void __user *buffer, size_t *lenp,
> +                                            loff_t *ppos)
> +{
> +       char name[SECCOMP_RET_MAX_NAME_LEN + 1] = {0};

{ } preferred over { 0 }

> +       int ret;
> +
> +       if (write && !capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if (!write) {
> +               const char *namep;
> +
> +               if (!seccomp_name_from_action(&namep,
> +                                             seccomp_max_action_to_log))
> +                       return -EINVAL;
> +
> +               strncpy(name, namep, sizeof(name) - 1);
> +       }
> +
> +       table->data = name;
> +       table->maxlen = sizeof(name);

In the hopes of some day making the sysctl table entirely read-only,
can you add some fancy crap here for me? :) See
security/yama/yama_lsm.c's yama_dointvec_minmax(), which uses a copy
of the sysctl table on the stack.

> +       ret = proc_dostring(table, write, buffer, lenp, ppos);
> +       if (ret)
> +               return ret;
> +
> +       if (write) {
> +               u32 action;
> +
> +               if (!seccomp_action_from_name(&action, table->data))
> +                       return -EINVAL;
> +
> +               seccomp_max_action_to_log = action;
> +       }
> +
> +       return 0;
> +}
> +
>  static struct ctl_path seccomp_sysctl_path[] = {
>         { .procname = "kernel", },
>         { .procname = "seccomp", },
> @@ -936,6 +1039,11 @@ static struct ctl_table seccomp_sysctl_table[] = {
>                 .mode           = 0444,
>                 .proc_handler   = proc_dostring,
>         },
> +       {
> +               .procname       = "max_action_to_log",
> +               .mode           = 0644,
> +               .proc_handler   = seccomp_max_action_to_log_handler,
> +       },
>         { }
>  };
>
> --
> 2.7.4
>

(Though I still wonder if a numeric would be easier...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> Administrators can write to this sysctl to set the maximum seccomp
> action that should be logged. Any actions with values greater than
> what's written to the sysctl will not be logged.
>
> For example, all SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
> SECCOMP_RET_ERRNO actions would be logged if "errno" were written to the
> sysctl. SECCOMP_RET_TRACE and SECCOMP_RET_ALLOW actions would not be
> logged since their values are higher than SECCOMP_RET_ERRNO.
>
> The path to the sysctl is:
>
>  /proc/sys/kernel/seccomp/max_action_to_log

/me looks for new bikeshed paint.

How about .../seccomp/action_log ? (And a corresponding
s/max_action_to_log/action_log/, if that looks readable...) I think
four words is just too long. :)

> The actions_avail sysctl can be read to discover the valid action names
> that can be written to the max_action_to_log sysctl. The actions_avail
> sysctl is also useful in understanding the ordering of actions used when
> deciding the maximum action to log.
>
> The default setting for the sysctl is to only log SECCOMP_RET_KILL
> actions which matches the existing behavior.
>
> There's one important exception to this sysctl. If a task is
> specifically being audited, meaning that an audit context has been
> allocated for the task, seccomp will log all actions other than
> SECCOMP_RET_ALLOW despite the value of max_action_to_log. This exception
> preserves the existing auditing behavior of tasks with an allocated
> audit context.
>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  include/linux/audit.h |   6 +--
>  kernel/seccomp.c      | 114 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 112 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f51fca8d..e0d95fc 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -315,11 +315,7 @@ void audit_core_dumps(long signr);
>
>  static inline void audit_seccomp(unsigned long syscall, long signr, int code)
>  {
> -       if (!audit_enabled)
> -               return;
> -
> -       /* Force a record to be reported if a signal was delivered. */
> -       if (signr || unlikely(!audit_dummy_context()))
> +       if (audit_enabled && unlikely(!audit_dummy_context()))
>                 __audit_seccomp(syscall, signr, code);
>  }
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 919ad9f..548fb89 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -509,6 +509,24 @@ static void seccomp_send_sigsys(int syscall, int reason)
>  }
>  #endif /* CONFIG_SECCOMP_FILTER */
>
> +static u32 seccomp_max_action_to_log = SECCOMP_RET_KILL;
> +
> +static void seccomp_log(unsigned long syscall, long signr, u32 action)

Please mark this inline...

> +{
> +       /* Force an audit message to be emitted when the action is not greater
> +        * than the configured maximum action.
> +        */
> +       if (action <= seccomp_max_action_to_log)
> +               return __audit_seccomp(syscall, signr, action);
> +
> +       /* If the action is not an ALLOW action, let the audit subsystem decide
> +        * if it should be audited based on whether the current task itself is
> +        * being audited.
> +        */
> +       if (action != SECCOMP_RET_ALLOW)
> +               return audit_seccomp(syscall, signr, action);

Based on my thoughts below, this test can actually be removed (making
the audit_seccomp() call unconditional), since callers will always be
!= RET_ALLOW.

> +}
> +
>  /*
>   * Secure computing mode 1 allows only read/write/exit/sigreturn.
>   * To be fully secure this must be combined with rlimit
> @@ -534,7 +552,7 @@ static void __secure_computing_strict(int this_syscall)
>  #ifdef SECCOMP_DEBUG
>         dump_stack();
>  #endif
> -       audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
> +       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
>         do_exit(SIGKILL);
>  }
>
> @@ -633,18 +651,19 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>                 return 0;
>
>         case SECCOMP_RET_ALLOW:
> +               seccomp_log(this_syscall, 0, action);
>                 return 0;

I am extremely sensitive about anything appearing in the RET_ALLOW
case, since it's the hot path for seccomp. This adds a full function
call (which also contains a redundant test: the action IS RET_ALLOW,
so we'll never call audit_seccomp() in seccomp_log()).

While the inline request above removes the function call, it's not
clear to me if gcc is going to do the right thing here, and I'd like
to assist the branch predictor (likely separate from the other case),
so I think I'd like an open-coded test here instead:

case SECCOMP_RET_ALLOW:
    /* Open-coded seccomp_log(), optimized for RET_ALLOW. */
    if (unlikely(seccomp_max_action_to_log == 0))
        __audit_seccomp(syscall, signr, action);
    return 0;

>         case SECCOMP_RET_KILL:
>         default:
> -               audit_seccomp(this_syscall, SIGSYS, action);
> +               seccomp_log(this_syscall, SIGSYS, action);
>                 do_exit(SIGSYS);
>         }
>
>         unreachable();
>
>  skip:
> -       audit_seccomp(this_syscall, 0, action);
> +       seccomp_log(this_syscall, 0, action);
>         return -1;
>  }
>  #else
> @@ -910,18 +929,102 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>
>  #ifdef CONFIG_SYSCTL
>
> +/* Human readable action names for friendly sysctl interaction */

This line should be in patch 1.

>  #define SECCOMP_RET_KILL_NAME          "kill"
>  #define SECCOMP_RET_TRAP_NAME          "trap"
>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>  #define SECCOMP_RET_TRACE_NAME         "trace"
>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>
> +/* Largest strlen() of all action names */
> +#define SECCOMP_RET_MAX_NAME_LEN       5

This feels fragile... though I don't have a good suggestion yet. :P

> +
>  static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>                                       SECCOMP_RET_TRAP_NAME     " "
>                                       SECCOMP_RET_ERRNO_NAME    " "
>                                       SECCOMP_RET_TRACE_NAME    " "
>                                       SECCOMP_RET_ALLOW_NAME;
>
> +struct seccomp_action_name {
> +       u32             action;
> +       const char      *name;
> +};
> +
> +static struct seccomp_action_name seccomp_action_names[] = {
> +       { SECCOMP_RET_KILL, SECCOMP_RET_KILL_NAME },
> +       { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
> +       { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
> +       { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
> +       { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
> +       { }
> +};
> +
> +static bool seccomp_name_from_action(const char **namep, u32 action)
> +{
> +       struct seccomp_action_name *cur;
> +
> +       for (cur = seccomp_action_names; cur->name; cur++) {
> +               if (cur->action == action) {
> +                       *namep = cur->name;
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static bool seccomp_action_from_name(u32 *action, const char *name)
> +{
> +       struct seccomp_action_name *cur;
> +
> +       for (cur = seccomp_action_names; cur->name; cur++) {
> +               if (!strcmp(cur->name, name)) {
> +                       *action = cur->action;
> +                       return true;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static int seccomp_max_action_to_log_handler(struct ctl_table *table, int write,
> +                                            void __user *buffer, size_t *lenp,
> +                                            loff_t *ppos)
> +{
> +       char name[SECCOMP_RET_MAX_NAME_LEN + 1] = {0};

{ } preferred over { 0 }

> +       int ret;
> +
> +       if (write && !capable(CAP_SYS_ADMIN))
> +               return -EPERM;
> +
> +       if (!write) {
> +               const char *namep;
> +
> +               if (!seccomp_name_from_action(&namep,
> +                                             seccomp_max_action_to_log))
> +                       return -EINVAL;
> +
> +               strncpy(name, namep, sizeof(name) - 1);
> +       }
> +
> +       table->data = name;
> +       table->maxlen = sizeof(name);

In the hopes of some day making the sysctl table entirely read-only,
can you add some fancy crap here for me? :) See
security/yama/yama_lsm.c's yama_dointvec_minmax(), which uses a copy
of the sysctl table on the stack.

> +       ret = proc_dostring(table, write, buffer, lenp, ppos);
> +       if (ret)
> +               return ret;
> +
> +       if (write) {
> +               u32 action;
> +
> +               if (!seccomp_action_from_name(&action, table->data))
> +                       return -EINVAL;
> +
> +               seccomp_max_action_to_log = action;
> +       }
> +
> +       return 0;
> +}
> +
>  static struct ctl_path seccomp_sysctl_path[] = {
>         { .procname = "kernel", },
>         { .procname = "seccomp", },
> @@ -936,6 +1039,11 @@ static struct ctl_table seccomp_sysctl_table[] = {
>                 .mode           = 0444,
>                 .proc_handler   = proc_dostring,
>         },
> +       {
> +               .procname       = "max_action_to_log",
> +               .mode           = 0644,
> +               .proc_handler   = seccomp_max_action_to_log_handler,
> +       },
>         { }
>  };
>
> --
> 2.7.4
>

(Though I still wonder if a numeric would be easier...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 4912 bytes --]

On 02/07/2017 06:03 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch creates a read-only sysctl containing an ordered list of
>> seccomp actions that the kernel supports. The ordering, from left to
>> right, is the lowest action value (kill) to the highest action value
>> (allow). Currently, a read of the sysctl file would return "kill trap
>> errno trace allow". The contents of this sysctl file can be useful for
>> userspace code as well as the system administrator.
>>
>> The path to the sysctl is:
>>
>>   /proc/sys/kernel/seccomp/actions_avail
>>
>> libseccomp and other userspace code can easily determine which actions
>> the current kernel supports. The set of actions supported by the current
>> kernel may be different than the set of action macros found in kernel
>> headers that were installed where the userspace code was built.
> 
> This is certainly good: having a discoverable way to detect filter
> capabilities. I do wonder if it'd still be easier to just expose the
> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
> all part of uapi, so we can't escape their values...

I was very torn on whether to use a numeric or string representation
here. The reason I decided on string representation is because I think
these sysctls are mostly aimed for admins and numeric representations
wouldn't be easy to use. I considered added a utility to libseccomp but,
since the kernel code to do a string representation was so simple, I
went with doing it in the kernel.

Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
the uapi.

> 
> 
> 
>>
>> In addition, this sysctl will allow system administrators to know which
>> actions are supported by the kernel and make it easier to configure
>> exactly what seccomp logs through the audit subsystem. Support for this
>> level of logging configuration will come in a future patch.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  kernel/seccomp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index f7ce79a..919ad9f 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -16,10 +16,12 @@
>>  #include <linux/atomic.h>
>>  #include <linux/audit.h>
>>  #include <linux/compat.h>
>> +#include <linux/kmemleak.h>
>>  #include <linux/sched.h>
>>  #include <linux/seccomp.h>
>>  #include <linux/slab.h>
>>  #include <linux/syscalls.h>
>> +#include <linux/sysctl.h>
>>
>>  #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
>>  #include <asm/syscall.h>
>> @@ -905,3 +907,51 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>         return ret;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_SYSCTL
>> +
>> +#define SECCOMP_RET_KILL_NAME          "kill"
>> +#define SECCOMP_RET_TRAP_NAME          "trap"
>> +#define SECCOMP_RET_ERRNO_NAME         "errno"
>> +#define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_ALLOW_NAME         "allow"
>> +
>> +static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>> +                                     SECCOMP_RET_TRAP_NAME     " "
>> +                                     SECCOMP_RET_ERRNO_NAME    " "
>> +                                     SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_ALLOW_NAME;
>> +
>> +static struct ctl_path seccomp_sysctl_path[] = {
>> +       { .procname = "kernel", },
>> +       { .procname = "seccomp", },
>> +       { }
>> +};
>> +
>> +static struct ctl_table seccomp_sysctl_table[] = {
>> +       {
>> +               .procname       = "actions_avail",
>> +               .data           = &seccomp_actions_avail,
>> +               .maxlen         = sizeof(seccomp_actions_avail),
>> +               .mode           = 0444,
>> +               .proc_handler   = proc_dostring,
>> +       },
>> +       { }
>> +};
>> +
>> +static int __init seccomp_sysctl_init(void)
>> +{
>> +       struct ctl_table_header *hdr;
>> +
>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>> +       kmemleak_not_leak(hdr);
> 
> Will kmemleak complain about this if hdr is saved to a global (or not
> saved at all)? Also, something should be reported in the failure
> case...

I have to admit to blindly following the example set by sysctl_init() in
kernel/sysctl.c. I can test what kmemleak will/won't complain about and
report back (tomorrow at the earliest).

Tyler

> 
>> +       return 0;
>> +}
>> +
>> +#else /* CONFIG_SYSCTL */
>> +
>> +static __init int seccomp_sysctl_init(void) { return 0; }
>> +
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +device_initcall(seccomp_sysctl_init)
>> --
>> 2.7.4
>>
> 
> -Kees
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Tyler Hicks]

[-- Attachment #1.1.1: Type: text/plain, Size: 4912 bytes --]

On 02/07/2017 06:03 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> This patch creates a read-only sysctl containing an ordered list of
>> seccomp actions that the kernel supports. The ordering, from left to
>> right, is the lowest action value (kill) to the highest action value
>> (allow). Currently, a read of the sysctl file would return "kill trap
>> errno trace allow". The contents of this sysctl file can be useful for
>> userspace code as well as the system administrator.
>>
>> The path to the sysctl is:
>>
>>   /proc/sys/kernel/seccomp/actions_avail
>>
>> libseccomp and other userspace code can easily determine which actions
>> the current kernel supports. The set of actions supported by the current
>> kernel may be different than the set of action macros found in kernel
>> headers that were installed where the userspace code was built.
> 
> This is certainly good: having a discoverable way to detect filter
> capabilities. I do wonder if it'd still be easier to just expose the
> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
> all part of uapi, so we can't escape their values...

I was very torn on whether to use a numeric or string representation
here. The reason I decided on string representation is because I think
these sysctls are mostly aimed for admins and numeric representations
wouldn't be easy to use. I considered added a utility to libseccomp but,
since the kernel code to do a string representation was so simple, I
went with doing it in the kernel.

Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
the uapi.

> 
> 
> 
>>
>> In addition, this sysctl will allow system administrators to know which
>> actions are supported by the kernel and make it easier to configure
>> exactly what seccomp logs through the audit subsystem. Support for this
>> level of logging configuration will come in a future patch.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  kernel/seccomp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index f7ce79a..919ad9f 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -16,10 +16,12 @@
>>  #include <linux/atomic.h>
>>  #include <linux/audit.h>
>>  #include <linux/compat.h>
>> +#include <linux/kmemleak.h>
>>  #include <linux/sched.h>
>>  #include <linux/seccomp.h>
>>  #include <linux/slab.h>
>>  #include <linux/syscalls.h>
>> +#include <linux/sysctl.h>
>>
>>  #ifdef CONFIG_HAVE_ARCH_SECCOMP_FILTER
>>  #include <asm/syscall.h>
>> @@ -905,3 +907,51 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>         return ret;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_SYSCTL
>> +
>> +#define SECCOMP_RET_KILL_NAME          "kill"
>> +#define SECCOMP_RET_TRAP_NAME          "trap"
>> +#define SECCOMP_RET_ERRNO_NAME         "errno"
>> +#define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_ALLOW_NAME         "allow"
>> +
>> +static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>> +                                     SECCOMP_RET_TRAP_NAME     " "
>> +                                     SECCOMP_RET_ERRNO_NAME    " "
>> +                                     SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_ALLOW_NAME;
>> +
>> +static struct ctl_path seccomp_sysctl_path[] = {
>> +       { .procname = "kernel", },
>> +       { .procname = "seccomp", },
>> +       { }
>> +};
>> +
>> +static struct ctl_table seccomp_sysctl_table[] = {
>> +       {
>> +               .procname       = "actions_avail",
>> +               .data           = &seccomp_actions_avail,
>> +               .maxlen         = sizeof(seccomp_actions_avail),
>> +               .mode           = 0444,
>> +               .proc_handler   = proc_dostring,
>> +       },
>> +       { }
>> +};
>> +
>> +static int __init seccomp_sysctl_init(void)
>> +{
>> +       struct ctl_table_header *hdr;
>> +
>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>> +       kmemleak_not_leak(hdr);
> 
> Will kmemleak complain about this if hdr is saved to a global (or not
> saved at all)? Also, something should be reported in the failure
> case...

I have to admit to blindly following the example set by sysctl_init() in
kernel/sysctl.c. I can test what kmemleak will/won't complain about and
report back (tomorrow at the earliest).

Tyler

> 
>> +       return 0;
>> +}
>> +
>> +#else /* CONFIG_SYSCTL */
>> +
>> +static __init int seccomp_sysctl_init(void) { return 0; }
>> +
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +device_initcall(seccomp_sysctl_init)
>> --
>> 2.7.4
>>
> 
> -Kees
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
> the syscall. At the implementation level, this action is identical to
> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
> initially developing a seccomp filter for an application. The developer
> can set the default action to be SECCOMP_RET_LOG, maybe mark any
> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
> application through its paces. A list of syscalls that triggered the
> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
> that list can be used to build the syscall whitelist. Finally, the
> developer can change the default action to the desired value.
>
> This provides a more friendly experience than seeing the application get
> killed, then updating the filter and rebuilding the app, seeing the
> application get killed due to a different syscall, then updating the
> filter and rebuilding the app, etc.
>
> The functionality is similar to what's supported by the various LSMs.
> SELinux has permissive mode, AppArmor has complain mode, SMACK has
> bring-up mode, etc.
>
> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
> "allow" can be written to the max_action_to_log sysctl in order to get a
> list of logged actions without the, potentially larger, set of allowed
> actions.
>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  Documentation/prctl/seccomp_filter.txt | 6 ++++++
>  include/uapi/linux/seccomp.h           | 1 +
>  kernel/seccomp.c                       | 4 ++++
>  3 files changed, 11 insertions(+)
>
> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
> index 1e469ef..ba55a91 100644
> --- a/Documentation/prctl/seccomp_filter.txt
> +++ b/Documentation/prctl/seccomp_filter.txt
> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
>         allow use of ptrace, even of other sandboxed processes, without
>         extreme care; ptracers can use this mechanism to escape.)
>
> +SECCOMP_RET_LOG:
> +       Results in the system call being executed after it is logged. This
> +       should be used by application developers to learn which syscalls their
> +       application needs without having to iterate through multiple test and
> +       development cycles to build the list.
> +
>  SECCOMP_RET_ALLOW:
>         Results in the system call being executed.
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 0f238a4..67f72cd 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -29,6 +29,7 @@
>  #define SECCOMP_RET_TRAP       0x00030000U /* disallow and force a SIGSYS */
>  #define SECCOMP_RET_ERRNO      0x00050000U /* returns an errno */
>  #define SECCOMP_RET_TRACE      0x7ff00000U /* pass to a tracer or disallow */
> +#define SECCOMP_RET_LOG                0x7ffe0000U /* allow after logging */

This adds to UAPI, so it'd be good to think for a moment about how
this would work on older kernels: right now, if someone tried to use
this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
this sane?

I'm also trying to figure out if there is some other solution to this,
but they all involve tests against an otherwise RET_ALLOW case, which
I want to avoid. :)

So, I think, for now, this looks good, but I'd prefer this be
0x7ffc0000U, just to make sure we have not painted ourselves into a
numerical corner if we for some reason ever need to put something
between RET_ALLOW and RET_LOG.

>  #define SECCOMP_RET_ALLOW      0x7fff0000U /* allow */
>
>  /* Masks for the return value sections. */
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 548fb89..8627481 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>
>                 return 0;
>
> +       case SECCOMP_RET_LOG:

Given my protective feelings about the RET_ALLOW case, can you make
this a fully separate case statement? I'd rather have RET_ALLOW be
distinctly separate.

>         case SECCOMP_RET_ALLOW:
>                 seccomp_log(this_syscall, 0, action);
>                 return 0;
> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>  #define SECCOMP_RET_TRAP_NAME          "trap"
>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>  #define SECCOMP_RET_TRACE_NAME         "trace"
> +#define SECCOMP_RET_LOG_NAME           "log"
>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>
>  /* Largest strlen() of all action names */
> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
>                                       SECCOMP_RET_TRAP_NAME     " "
>                                       SECCOMP_RET_ERRNO_NAME    " "
>                                       SECCOMP_RET_TRACE_NAME    " "
> +                                     SECCOMP_RET_LOG_NAME      " "
>                                       SECCOMP_RET_ALLOW_NAME;
>
>  struct seccomp_action_name {
> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
>         { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>         { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>         { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
> +       { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
>         { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>         { }
>  };
> --
> 2.7.4
>

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 4/4] seccomp: Add tests for SECCOMP_RET_LOG

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 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.
>
> 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..a39f620 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       0x7ffe0000U /* allow after logging */

Except changing this to match my suggested tweak, this all looks
great. (Though it would be fun to find a clean way to actually examine
the dmesg buffer...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 4/4] seccomp: Add tests for SECCOMP_RET_LOG

[Kees Cook]

On Thu, Feb 2, 2017 at 9:37 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.
>
> 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..a39f620 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       0x7ffe0000U /* allow after logging */

Except changing this to match my suggested tweak, this all looks
great. (Though it would be fun to find a clean way to actually examine
the dmesg buffer...)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Kees Cook]

On Tue, Feb 7, 2017 at 4:25 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 02/07/2017 06:03 PM, Kees Cook wrote:
>> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> This patch creates a read-only sysctl containing an ordered list of
>>> seccomp actions that the kernel supports. The ordering, from left to
>>> right, is the lowest action value (kill) to the highest action value
>>> (allow). Currently, a read of the sysctl file would return "kill trap
>>> errno trace allow". The contents of this sysctl file can be useful for
>>> userspace code as well as the system administrator.
>>>
>>> The path to the sysctl is:
>>>
>>>   /proc/sys/kernel/seccomp/actions_avail
>>>
>>> libseccomp and other userspace code can easily determine which actions
>>> the current kernel supports. The set of actions supported by the current
>>> kernel may be different than the set of action macros found in kernel
>>> headers that were installed where the userspace code was built.
>>
>> This is certainly good: having a discoverable way to detect filter
>> capabilities. I do wonder if it'd still be easier to just expose the
>> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
>> all part of uapi, so we can't escape their values...
>
> I was very torn on whether to use a numeric or string representation
> here. The reason I decided on string representation is because I think
> these sysctls are mostly aimed for admins and numeric representations
> wouldn't be easy to use. I considered added a utility to libseccomp but,
> since the kernel code to do a string representation was so simple, I
> went with doing it in the kernel.

Yeah, I think I like it just because it gives a way to discover the
UAPI "level"... I will think more about this. For v3, let's keep the
string stuff.

> Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
> the uapi.

I like keeping the UAPI minimal. ;)

>>> +static int __init seccomp_sysctl_init(void)
>>> +{
>>> +       struct ctl_table_header *hdr;
>>> +
>>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>>> +       kmemleak_not_leak(hdr);
>>
>> Will kmemleak complain about this if hdr is saved to a global (or not
>> saved at all)? Also, something should be reported in the failure
>> case...
>
> I have to admit to blindly following the example set by sysctl_init() in
> kernel/sysctl.c. I can test what kmemleak will/won't complain about and
> report back (tomorrow at the earliest).

Cool, no rush. I'm backlogged on reviews anyway. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Kees Cook]

On Tue, Feb 7, 2017 at 4:25 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 02/07/2017 06:03 PM, Kees Cook wrote:
>> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> This patch creates a read-only sysctl containing an ordered list of
>>> seccomp actions that the kernel supports. The ordering, from left to
>>> right, is the lowest action value (kill) to the highest action value
>>> (allow). Currently, a read of the sysctl file would return "kill trap
>>> errno trace allow". The contents of this sysctl file can be useful for
>>> userspace code as well as the system administrator.
>>>
>>> The path to the sysctl is:
>>>
>>>   /proc/sys/kernel/seccomp/actions_avail
>>>
>>> libseccomp and other userspace code can easily determine which actions
>>> the current kernel supports. The set of actions supported by the current
>>> kernel may be different than the set of action macros found in kernel
>>> headers that were installed where the userspace code was built.
>>
>> This is certainly good: having a discoverable way to detect filter
>> capabilities. I do wonder if it'd still be easier to just expose the
>> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
>> all part of uapi, so we can't escape their values...
>
> I was very torn on whether to use a numeric or string representation
> here. The reason I decided on string representation is because I think
> these sysctls are mostly aimed for admins and numeric representations
> wouldn't be easy to use. I considered added a utility to libseccomp but,
> since the kernel code to do a string representation was so simple, I
> went with doing it in the kernel.

Yeah, I think I like it just because it gives a way to discover the
UAPI "level"... I will think more about this. For v3, let's keep the
string stuff.

> Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
> the uapi.

I like keeping the UAPI minimal. ;)

>>> +static int __init seccomp_sysctl_init(void)
>>> +{
>>> +       struct ctl_table_header *hdr;
>>> +
>>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>>> +       kmemleak_not_leak(hdr);
>>
>> Will kmemleak complain about this if hdr is saved to a global (or not
>> saved at all)? Also, something should be reported in the failure
>> case...
>
> I have to admit to blindly following the example set by sysctl_init() in
> kernel/sysctl.c. I can test what kmemleak will/won't complain about and
> report back (tomorrow at the earliest).

Cool, no rush. I'm backlogged on reviews anyway. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Kees Cook]

On Tue, Feb 7, 2017 at 4:33 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
>> the syscall. At the implementation level, this action is identical to
>> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
>> initially developing a seccomp filter for an application. The developer
>> can set the default action to be SECCOMP_RET_LOG, maybe mark any
>> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
>> application through its paces. A list of syscalls that triggered the
>> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
>> that list can be used to build the syscall whitelist. Finally, the
>> developer can change the default action to the desired value.
>>
>> This provides a more friendly experience than seeing the application get
>> killed, then updating the filter and rebuilding the app, seeing the
>> application get killed due to a different syscall, then updating the
>> filter and rebuilding the app, etc.
>>
>> The functionality is similar to what's supported by the various LSMs.
>> SELinux has permissive mode, AppArmor has complain mode, SMACK has
>> bring-up mode, etc.
>>
>> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
>> "allow" can be written to the max_action_to_log sysctl in order to get a
>> list of logged actions without the, potentially larger, set of allowed
>> actions.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

It came to my attention that OpenWRT has actually been running with a
SECCOMP_RET_LOG on at least the 3.18 kernels (added John to CC):

https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.18/999-seccomp_log.patch?rev=45093

This patch appears to have SECCOMP_RET_LOG perform a "skip" (rather
than allow, which contradicts the comment). (And since it's closer to
the KILL end, I assume the comment is wrong... well, inaccurate, it's
allowed in the sense that the process isn't killed, but the syscall
isn't executed, so it's not really allowed.)

Tyler's SECCOMP_RET_LOG performs the syscall and logs.

I'd like to make sure Tyler's series will meaningfully address what
OpenWRT needs, since it appears OpenWRT has a similar logging desire.
:)

John, what's the intention for this filter value? It looks like you
want to perform RET_ERRNO but get a log message? Would Tyler's global
log level API work for OpenWRT?
https://lkml.org/lkml/2017/2/3/13

i.e. change your default filter return to SECCOMP_RET_ERRNO, and set
the sysctl to ERRNO?

Thanks!

-Kees

>> ---
>>  Documentation/prctl/seccomp_filter.txt | 6 ++++++
>>  include/uapi/linux/seccomp.h           | 1 +
>>  kernel/seccomp.c                       | 4 ++++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
>> index 1e469ef..ba55a91 100644
>> --- a/Documentation/prctl/seccomp_filter.txt
>> +++ b/Documentation/prctl/seccomp_filter.txt
>> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
>>         allow use of ptrace, even of other sandboxed processes, without
>>         extreme care; ptracers can use this mechanism to escape.)
>>
>> +SECCOMP_RET_LOG:
>> +       Results in the system call being executed after it is logged. This
>> +       should be used by application developers to learn which syscalls their
>> +       application needs without having to iterate through multiple test and
>> +       development cycles to build the list.
>> +
>>  SECCOMP_RET_ALLOW:
>>         Results in the system call being executed.
>>
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a4..67f72cd 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -29,6 +29,7 @@
>>  #define SECCOMP_RET_TRAP       0x00030000U /* disallow and force a SIGSYS */
>>  #define SECCOMP_RET_ERRNO      0x00050000U /* returns an errno */
>>  #define SECCOMP_RET_TRACE      0x7ff00000U /* pass to a tracer or disallow */
>> +#define SECCOMP_RET_LOG                0x7ffe0000U /* allow after logging */
>
> This adds to UAPI, so it'd be good to think for a moment about how
> this would work on older kernels: right now, if someone tried to use
> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
> this sane?
>
> I'm also trying to figure out if there is some other solution to this,
> but they all involve tests against an otherwise RET_ALLOW case, which
> I want to avoid. :)
>
> So, I think, for now, this looks good, but I'd prefer this be
> 0x7ffc0000U, just to make sure we have not painted ourselves into a
> numerical corner if we for some reason ever need to put something
> between RET_ALLOW and RET_LOG.
>
>>  #define SECCOMP_RET_ALLOW      0x7fff0000U /* allow */
>>
>>  /* Masks for the return value sections. */
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 548fb89..8627481 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>
>>                 return 0;
>>
>> +       case SECCOMP_RET_LOG:
>
> Given my protective feelings about the RET_ALLOW case, can you make
> this a fully separate case statement? I'd rather have RET_ALLOW be
> distinctly separate.
>
>>         case SECCOMP_RET_ALLOW:
>>                 seccomp_log(this_syscall, 0, action);
>>                 return 0;
>> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>  #define SECCOMP_RET_TRAP_NAME          "trap"
>>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>>  #define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_LOG_NAME           "log"
>>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>>
>>  /* Largest strlen() of all action names */
>> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
>>                                       SECCOMP_RET_TRAP_NAME     " "
>>                                       SECCOMP_RET_ERRNO_NAME    " "
>>                                       SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_LOG_NAME      " "
>>                                       SECCOMP_RET_ALLOW_NAME;
>>
>>  struct seccomp_action_name {
>> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
>>         { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>>         { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>>         { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
>> +       { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
>>         { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>>         { }
>>  };
>> --
>> 2.7.4
>>
>
> -Kees
>
> --
> Kees Cook
> Pixel Security


-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Kees Cook]

On Tue, Feb 7, 2017 at 4:33 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
>> the syscall. At the implementation level, this action is identical to
>> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
>> initially developing a seccomp filter for an application. The developer
>> can set the default action to be SECCOMP_RET_LOG, maybe mark any
>> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
>> application through its paces. A list of syscalls that triggered the
>> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
>> that list can be used to build the syscall whitelist. Finally, the
>> developer can change the default action to the desired value.
>>
>> This provides a more friendly experience than seeing the application get
>> killed, then updating the filter and rebuilding the app, seeing the
>> application get killed due to a different syscall, then updating the
>> filter and rebuilding the app, etc.
>>
>> The functionality is similar to what's supported by the various LSMs.
>> SELinux has permissive mode, AppArmor has complain mode, SMACK has
>> bring-up mode, etc.
>>
>> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
>> "allow" can be written to the max_action_to_log sysctl in order to get a
>> list of logged actions without the, potentially larger, set of allowed
>> actions.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

It came to my attention that OpenWRT has actually been running with a
SECCOMP_RET_LOG on at least the 3.18 kernels (added John to CC):

https://dev.openwrt.org/browser/trunk/target/linux/generic/patches-3.18/999-seccomp_log.patch?rev=45093

This patch appears to have SECCOMP_RET_LOG perform a "skip" (rather
than allow, which contradicts the comment). (And since it's closer to
the KILL end, I assume the comment is wrong... well, inaccurate, it's
allowed in the sense that the process isn't killed, but the syscall
isn't executed, so it's not really allowed.)

Tyler's SECCOMP_RET_LOG performs the syscall and logs.

I'd like to make sure Tyler's series will meaningfully address what
OpenWRT needs, since it appears OpenWRT has a similar logging desire.
:)

John, what's the intention for this filter value? It looks like you
want to perform RET_ERRNO but get a log message? Would Tyler's global
log level API work for OpenWRT?
https://lkml.org/lkml/2017/2/3/13

i.e. change your default filter return to SECCOMP_RET_ERRNO, and set
the sysctl to ERRNO?

Thanks!

-Kees

>> ---
>>  Documentation/prctl/seccomp_filter.txt | 6 ++++++
>>  include/uapi/linux/seccomp.h           | 1 +
>>  kernel/seccomp.c                       | 4 ++++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
>> index 1e469ef..ba55a91 100644
>> --- a/Documentation/prctl/seccomp_filter.txt
>> +++ b/Documentation/prctl/seccomp_filter.txt
>> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
>>         allow use of ptrace, even of other sandboxed processes, without
>>         extreme care; ptracers can use this mechanism to escape.)
>>
>> +SECCOMP_RET_LOG:
>> +       Results in the system call being executed after it is logged. This
>> +       should be used by application developers to learn which syscalls their
>> +       application needs without having to iterate through multiple test and
>> +       development cycles to build the list.
>> +
>>  SECCOMP_RET_ALLOW:
>>         Results in the system call being executed.
>>
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a4..67f72cd 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -29,6 +29,7 @@
>>  #define SECCOMP_RET_TRAP       0x00030000U /* disallow and force a SIGSYS */
>>  #define SECCOMP_RET_ERRNO      0x00050000U /* returns an errno */
>>  #define SECCOMP_RET_TRACE      0x7ff00000U /* pass to a tracer or disallow */
>> +#define SECCOMP_RET_LOG                0x7ffe0000U /* allow after logging */
>
> This adds to UAPI, so it'd be good to think for a moment about how
> this would work on older kernels: right now, if someone tried to use
> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
> this sane?
>
> I'm also trying to figure out if there is some other solution to this,
> but they all involve tests against an otherwise RET_ALLOW case, which
> I want to avoid. :)
>
> So, I think, for now, this looks good, but I'd prefer this be
> 0x7ffc0000U, just to make sure we have not painted ourselves into a
> numerical corner if we for some reason ever need to put something
> between RET_ALLOW and RET_LOG.
>
>>  #define SECCOMP_RET_ALLOW      0x7fff0000U /* allow */
>>
>>  /* Masks for the return value sections. */
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 548fb89..8627481 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>
>>                 return 0;
>>
>> +       case SECCOMP_RET_LOG:
>
> Given my protective feelings about the RET_ALLOW case, can you make
> this a fully separate case statement? I'd rather have RET_ALLOW be
> distinctly separate.
>
>>         case SECCOMP_RET_ALLOW:
>>                 seccomp_log(this_syscall, 0, action);
>>                 return 0;
>> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>  #define SECCOMP_RET_TRAP_NAME          "trap"
>>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>>  #define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_LOG_NAME           "log"
>>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>>
>>  /* Largest strlen() of all action names */
>> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
>>                                       SECCOMP_RET_TRAP_NAME     " "
>>                                       SECCOMP_RET_ERRNO_NAME    " "
>>                                       SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_LOG_NAME      " "
>>                                       SECCOMP_RET_ALLOW_NAME;
>>
>>  struct seccomp_action_name {
>> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
>>         { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>>         { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>>         { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
>> +       { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
>>         { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>>         { }
>>  };
>> --
>> 2.7.4
>>
>
> -Kees
>
> --
> Kees Cook
> Pixel Security


-- 
Kees Cook
Pixel Security

Re: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 10795 bytes --]

On 02/07/2017 06:24 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Administrators can write to this sysctl to set the maximum seccomp
>> action that should be logged. Any actions with values greater than
>> what's written to the sysctl will not be logged.
>>
>> For example, all SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
>> SECCOMP_RET_ERRNO actions would be logged if "errno" were written to the
>> sysctl. SECCOMP_RET_TRACE and SECCOMP_RET_ALLOW actions would not be
>> logged since their values are higher than SECCOMP_RET_ERRNO.
>>
>> The path to the sysctl is:
>>
>>  /proc/sys/kernel/seccomp/max_action_to_log
> 
> /me looks for new bikeshed paint.
> 
> How about .../seccomp/action_log ? (And a corresponding
> s/max_action_to_log/action_log/, if that looks readable...) I think
> four words is just too long. :)

Kees and I discussed this a bit over IRC today. We settled on
log_max_action for v3 of the patch set.

> 
>> The actions_avail sysctl can be read to discover the valid action names
>> that can be written to the max_action_to_log sysctl. The actions_avail
>> sysctl is also useful in understanding the ordering of actions used when
>> deciding the maximum action to log.
>>
>> The default setting for the sysctl is to only log SECCOMP_RET_KILL
>> actions which matches the existing behavior.
>>
>> There's one important exception to this sysctl. If a task is
>> specifically being audited, meaning that an audit context has been
>> allocated for the task, seccomp will log all actions other than
>> SECCOMP_RET_ALLOW despite the value of max_action_to_log. This exception
>> preserves the existing auditing behavior of tasks with an allocated
>> audit context.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  include/linux/audit.h |   6 +--
>>  kernel/seccomp.c      | 114 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 112 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index f51fca8d..e0d95fc 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -315,11 +315,7 @@ void audit_core_dumps(long signr);
>>
>>  static inline void audit_seccomp(unsigned long syscall, long signr, int code)
>>  {
>> -       if (!audit_enabled)
>> -               return;
>> -
>> -       /* Force a record to be reported if a signal was delivered. */
>> -       if (signr || unlikely(!audit_dummy_context()))
>> +       if (audit_enabled && unlikely(!audit_dummy_context()))
>>                 __audit_seccomp(syscall, signr, code);
>>  }
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 919ad9f..548fb89 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -509,6 +509,24 @@ static void seccomp_send_sigsys(int syscall, int reason)
>>  }
>>  #endif /* CONFIG_SECCOMP_FILTER */
>>
>> +static u32 seccomp_max_action_to_log = SECCOMP_RET_KILL;
>> +
>> +static void seccomp_log(unsigned long syscall, long signr, u32 action)
> 
> Please mark this inline...

Will do.

> 
>> +{
>> +       /* Force an audit message to be emitted when the action is not greater
>> +        * than the configured maximum action.
>> +        */
>> +       if (action <= seccomp_max_action_to_log)
>> +               return __audit_seccomp(syscall, signr, action);
>> +
>> +       /* If the action is not an ALLOW action, let the audit subsystem decide
>> +        * if it should be audited based on whether the current task itself is
>> +        * being audited.
>> +        */
>> +       if (action != SECCOMP_RET_ALLOW)
>> +               return audit_seccomp(syscall, signr, action);
> 
> Based on my thoughts below, this test can actually be removed (making
> the audit_seccomp() call unconditional), since callers will always be
> != RET_ALLOW.

Agreed.

> 
>> +}
>> +
>>  /*
>>   * Secure computing mode 1 allows only read/write/exit/sigreturn.
>>   * To be fully secure this must be combined with rlimit
>> @@ -534,7 +552,7 @@ static void __secure_computing_strict(int this_syscall)
>>  #ifdef SECCOMP_DEBUG
>>         dump_stack();
>>  #endif
>> -       audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
>> +       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
>>         do_exit(SIGKILL);
>>  }
>>
>> @@ -633,18 +651,19 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>                 return 0;
>>
>>         case SECCOMP_RET_ALLOW:
>> +               seccomp_log(this_syscall, 0, action);
>>                 return 0;
> 
> I am extremely sensitive about anything appearing in the RET_ALLOW
> case, since it's the hot path for seccomp. This adds a full function
> call (which also contains a redundant test: the action IS RET_ALLOW,
> so we'll never call audit_seccomp() in seccomp_log()).
> 
> While the inline request above removes the function call, it's not
> clear to me if gcc is going to do the right thing here, and I'd like
> to assist the branch predictor (likely separate from the other case),
> so I think I'd like an open-coded test here instead:
> 
> case SECCOMP_RET_ALLOW:
>     /* Open-coded seccomp_log(), optimized for RET_ALLOW. */
>     if (unlikely(seccomp_max_action_to_log == 0))
>         __audit_seccomp(syscall, signr, action);
>     return 0;

That makes sense.

> 
>>         case SECCOMP_RET_KILL:
>>         default:
>> -               audit_seccomp(this_syscall, SIGSYS, action);
>> +               seccomp_log(this_syscall, SIGSYS, action);
>>                 do_exit(SIGSYS);
>>         }
>>
>>         unreachable();
>>
>>  skip:
>> -       audit_seccomp(this_syscall, 0, action);
>> +       seccomp_log(this_syscall, 0, action);
>>         return -1;
>>  }
>>  #else
>> @@ -910,18 +929,102 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>
>>  #ifdef CONFIG_SYSCTL
>>
>> +/* Human readable action names for friendly sysctl interaction */
> 
> This line should be in patch 1.
> 
>>  #define SECCOMP_RET_KILL_NAME          "kill"
>>  #define SECCOMP_RET_TRAP_NAME          "trap"
>>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>>  #define SECCOMP_RET_TRACE_NAME         "trace"
>>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>>
>> +/* Largest strlen() of all action names */
>> +#define SECCOMP_RET_MAX_NAME_LEN       5
> 
> This feels fragile... though I don't have a good suggestion yet. :P

I agree and I also don't have a good solution. I didn't like having to
hard code it.

> 
>> +
>>  static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
>>                                       SECCOMP_RET_TRAP_NAME     " "
>>                                       SECCOMP_RET_ERRNO_NAME    " "
>>                                       SECCOMP_RET_TRACE_NAME    " "
>>                                       SECCOMP_RET_ALLOW_NAME;
>>
>> +struct seccomp_action_name {
>> +       u32             action;
>> +       const char      *name;
>> +};
>> +
>> +static struct seccomp_action_name seccomp_action_names[] = {
>> +       { SECCOMP_RET_KILL, SECCOMP_RET_KILL_NAME },
>> +       { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>> +       { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>> +       { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
>> +       { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>> +       { }
>> +};
>> +
>> +static bool seccomp_name_from_action(const char **namep, u32 action)
>> +{
>> +       struct seccomp_action_name *cur;
>> +
>> +       for (cur = seccomp_action_names; cur->name; cur++) {
>> +               if (cur->action == action) {
>> +                       *namep = cur->name;
>> +                       return true;
>> +               }
>> +       }
>> +
>> +       return false;
>> +}
>> +
>> +static bool seccomp_action_from_name(u32 *action, const char *name)
>> +{
>> +       struct seccomp_action_name *cur;
>> +
>> +       for (cur = seccomp_action_names; cur->name; cur++) {
>> +               if (!strcmp(cur->name, name)) {
>> +                       *action = cur->action;
>> +                       return true;
>> +               }
>> +       }
>> +
>> +       return false;
>> +}
>> +
>> +static int seccomp_max_action_to_log_handler(struct ctl_table *table, int write,
>> +                                            void __user *buffer, size_t *lenp,
>> +                                            loff_t *ppos)
>> +{
>> +       char name[SECCOMP_RET_MAX_NAME_LEN + 1] = {0};
> 
> { } preferred over { 0 }

No problem.

> 
>> +       int ret;
>> +
>> +       if (write && !capable(CAP_SYS_ADMIN))
>> +               return -EPERM;
>> +
>> +       if (!write) {
>> +               const char *namep;
>> +
>> +               if (!seccomp_name_from_action(&namep,
>> +                                             seccomp_max_action_to_log))
>> +                       return -EINVAL;
>> +
>> +               strncpy(name, namep, sizeof(name) - 1);
>> +       }
>> +
>> +       table->data = name;
>> +       table->maxlen = sizeof(name);
> 
> In the hopes of some day making the sysctl table entirely read-only,
> can you add some fancy crap here for me? :) See
> security/yama/yama_lsm.c's yama_dointvec_minmax(), which uses a copy
> of the sysctl table on the stack.

Will do. I'll deviate slightly from yama_dointvec_minmax(). To make it
clear that the ctl_table param shouldn't be modified, I'm going to name
it ro_table and then the stack variable will be named table.

Tyler

> 
>> +       ret = proc_dostring(table, write, buffer, lenp, ppos);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (write) {
>> +               u32 action;
>> +
>> +               if (!seccomp_action_from_name(&action, table->data))
>> +                       return -EINVAL;
>> +
>> +               seccomp_max_action_to_log = action;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  static struct ctl_path seccomp_sysctl_path[] = {
>>         { .procname = "kernel", },
>>         { .procname = "seccomp", },
>> @@ -936,6 +1039,11 @@ static struct ctl_table seccomp_sysctl_table[] = {
>>                 .mode           = 0444,
>>                 .proc_handler   = proc_dostring,
>>         },
>> +       {
>> +               .procname       = "max_action_to_log",
>> +               .mode           = 0644,
>> +               .proc_handler   = seccomp_max_action_to_log_handler,
>> +       },
>>         { }
>>  };
>>
>> --
>> 2.7.4
>>
> 
> (Though I still wonder if a numeric would be easier...)
> 
> -Kees
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 6460 bytes --]

On 02/07/2017 06:33 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
>> the syscall. At the implementation level, this action is identical to
>> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
>> initially developing a seccomp filter for an application. The developer
>> can set the default action to be SECCOMP_RET_LOG, maybe mark any
>> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
>> application through its paces. A list of syscalls that triggered the
>> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
>> that list can be used to build the syscall whitelist. Finally, the
>> developer can change the default action to the desired value.
>>
>> This provides a more friendly experience than seeing the application get
>> killed, then updating the filter and rebuilding the app, seeing the
>> application get killed due to a different syscall, then updating the
>> filter and rebuilding the app, etc.
>>
>> The functionality is similar to what's supported by the various LSMs.
>> SELinux has permissive mode, AppArmor has complain mode, SMACK has
>> bring-up mode, etc.
>>
>> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
>> "allow" can be written to the max_action_to_log sysctl in order to get a
>> list of logged actions without the, potentially larger, set of allowed
>> actions.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  Documentation/prctl/seccomp_filter.txt | 6 ++++++
>>  include/uapi/linux/seccomp.h           | 1 +
>>  kernel/seccomp.c                       | 4 ++++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
>> index 1e469ef..ba55a91 100644
>> --- a/Documentation/prctl/seccomp_filter.txt
>> +++ b/Documentation/prctl/seccomp_filter.txt
>> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
>>         allow use of ptrace, even of other sandboxed processes, without
>>         extreme care; ptracers can use this mechanism to escape.)
>>
>> +SECCOMP_RET_LOG:
>> +       Results in the system call being executed after it is logged. This
>> +       should be used by application developers to learn which syscalls their
>> +       application needs without having to iterate through multiple test and
>> +       development cycles to build the list.
>> +
>>  SECCOMP_RET_ALLOW:
>>         Results in the system call being executed.
>>
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a4..67f72cd 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -29,6 +29,7 @@
>>  #define SECCOMP_RET_TRAP       0x00030000U /* disallow and force a SIGSYS */
>>  #define SECCOMP_RET_ERRNO      0x00050000U /* returns an errno */
>>  #define SECCOMP_RET_TRACE      0x7ff00000U /* pass to a tracer or disallow */
>> +#define SECCOMP_RET_LOG                0x7ffe0000U /* allow after logging */
> 
> This adds to UAPI, so it'd be good to think for a moment about how
> this would work on older kernels: right now, if someone tried to use
> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
> this sane?

It is not sane for userspace code to blindly attempt to use a new
feature on an old kernel. One of the main motivations of the
actions_avail sysctl is to allow userspace to be smart about what the
current kernel supports.

I'll be adding logic (requested by Paul) to libseccomp that checks this
sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
use libseccomp will have to do something similar.

> 
> I'm also trying to figure out if there is some other solution to this,
> but they all involve tests against an otherwise RET_ALLOW case, which
> I want to avoid. :)
> 
> So, I think, for now, this looks good, but I'd prefer this be
> 0x7ffc0000U, just to make sure we have not painted ourselves into a
> numerical corner if we for some reason ever need to put something
> between RET_ALLOW and RET_LOG.

That makes sense. I'll do that in v3.
> 
>>  #define SECCOMP_RET_ALLOW      0x7fff0000U /* allow */
>>
>>  /* Masks for the return value sections. */
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 548fb89..8627481 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>
>>                 return 0;
>>
>> +       case SECCOMP_RET_LOG:
> 
> Given my protective feelings about the RET_ALLOW case, can you make
> this a fully separate case statement? I'd rather have RET_ALLOW be
> distinctly separate.

Sure! It actually has to be two different cases now that we're doing the
hot path approach for SECCOMP_RET_ALLOW.

Tyler

> 
>>         case SECCOMP_RET_ALLOW:
>>                 seccomp_log(this_syscall, 0, action);
>>                 return 0;
>> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>  #define SECCOMP_RET_TRAP_NAME          "trap"
>>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>>  #define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_LOG_NAME           "log"
>>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>>
>>  /* Largest strlen() of all action names */
>> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
>>                                       SECCOMP_RET_TRAP_NAME     " "
>>                                       SECCOMP_RET_ERRNO_NAME    " "
>>                                       SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_LOG_NAME      " "
>>                                       SECCOMP_RET_ALLOW_NAME;
>>
>>  struct seccomp_action_name {
>> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
>>         { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>>         { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>>         { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
>> +       { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
>>         { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>>         { }
>>  };
>> --
>> 2.7.4
>>
> 
> -Kees
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Tyler Hicks]

[-- Attachment #1.1.1: Type: text/plain, Size: 6460 bytes --]

On 02/07/2017 06:33 PM, Kees Cook wrote:
> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing
>> the syscall. At the implementation level, this action is identical to
>> the existing SECCOMP_RET_ALLOW action. However, it can be very useful when
>> initially developing a seccomp filter for an application. The developer
>> can set the default action to be SECCOMP_RET_LOG, maybe mark any
>> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the
>> application through its paces. A list of syscalls that triggered the
>> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs and
>> that list can be used to build the syscall whitelist. Finally, the
>> developer can change the default action to the desired value.
>>
>> This provides a more friendly experience than seeing the application get
>> killed, then updating the filter and rebuilding the app, seeing the
>> application get killed due to a different syscall, then updating the
>> filter and rebuilding the app, etc.
>>
>> The functionality is similar to what's supported by the various LSMs.
>> SELinux has permissive mode, AppArmor has complain mode, SMACK has
>> bring-up mode, etc.
>>
>> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that
>> "allow" can be written to the max_action_to_log sysctl in order to get a
>> list of logged actions without the, potentially larger, set of allowed
>> actions.
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>>  Documentation/prctl/seccomp_filter.txt | 6 ++++++
>>  include/uapi/linux/seccomp.h           | 1 +
>>  kernel/seccomp.c                       | 4 ++++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
>> index 1e469ef..ba55a91 100644
>> --- a/Documentation/prctl/seccomp_filter.txt
>> +++ b/Documentation/prctl/seccomp_filter.txt
>> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE:
>>         allow use of ptrace, even of other sandboxed processes, without
>>         extreme care; ptracers can use this mechanism to escape.)
>>
>> +SECCOMP_RET_LOG:
>> +       Results in the system call being executed after it is logged. This
>> +       should be used by application developers to learn which syscalls their
>> +       application needs without having to iterate through multiple test and
>> +       development cycles to build the list.
>> +
>>  SECCOMP_RET_ALLOW:
>>         Results in the system call being executed.
>>
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 0f238a4..67f72cd 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -29,6 +29,7 @@
>>  #define SECCOMP_RET_TRAP       0x00030000U /* disallow and force a SIGSYS */
>>  #define SECCOMP_RET_ERRNO      0x00050000U /* returns an errno */
>>  #define SECCOMP_RET_TRACE      0x7ff00000U /* pass to a tracer or disallow */
>> +#define SECCOMP_RET_LOG                0x7ffe0000U /* allow after logging */
> 
> This adds to UAPI, so it'd be good to think for a moment about how
> this would work on older kernels: right now, if someone tried to use
> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
> this sane?

It is not sane for userspace code to blindly attempt to use a new
feature on an old kernel. One of the main motivations of the
actions_avail sysctl is to allow userspace to be smart about what the
current kernel supports.

I'll be adding logic (requested by Paul) to libseccomp that checks this
sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
use libseccomp will have to do something similar.

> 
> I'm also trying to figure out if there is some other solution to this,
> but they all involve tests against an otherwise RET_ALLOW case, which
> I want to avoid. :)
> 
> So, I think, for now, this looks good, but I'd prefer this be
> 0x7ffc0000U, just to make sure we have not painted ourselves into a
> numerical corner if we for some reason ever need to put something
> between RET_ALLOW and RET_LOG.

That makes sense. I'll do that in v3.
> 
>>  #define SECCOMP_RET_ALLOW      0x7fff0000U /* allow */
>>
>>  /* Masks for the return value sections. */
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index 548fb89..8627481 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>
>>                 return 0;
>>
>> +       case SECCOMP_RET_LOG:
> 
> Given my protective feelings about the RET_ALLOW case, can you make
> this a fully separate case statement? I'd rather have RET_ALLOW be
> distinctly separate.

Sure! It actually has to be two different cases now that we're doing the
hot path approach for SECCOMP_RET_ALLOW.

Tyler

> 
>>         case SECCOMP_RET_ALLOW:
>>                 seccomp_log(this_syscall, 0, action);
>>                 return 0;
>> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>  #define SECCOMP_RET_TRAP_NAME          "trap"
>>  #define SECCOMP_RET_ERRNO_NAME         "errno"
>>  #define SECCOMP_RET_TRACE_NAME         "trace"
>> +#define SECCOMP_RET_LOG_NAME           "log"
>>  #define SECCOMP_RET_ALLOW_NAME         "allow"
>>
>>  /* Largest strlen() of all action names */
>> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME " "
>>                                       SECCOMP_RET_TRAP_NAME     " "
>>                                       SECCOMP_RET_ERRNO_NAME    " "
>>                                       SECCOMP_RET_TRACE_NAME    " "
>> +                                     SECCOMP_RET_LOG_NAME      " "
>>                                       SECCOMP_RET_ALLOW_NAME;
>>
>>  struct seccomp_action_name {
>> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_names[] = {
>>         { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME },
>>         { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME },
>>         { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME },
>> +       { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME },
>>         { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME },
>>         { }
>>  };
>> --
>> 2.7.4
>>
> 
> -Kees
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged

[Kees Cook]

On Fri, Feb 10, 2017 at 3:56 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 02/07/2017 06:24 PM, Kees Cook wrote:
>> case SECCOMP_RET_ALLOW:
>>     /* Open-coded seccomp_log(), optimized for RET_ALLOW. */
>>     if (unlikely(seccomp_max_action_to_log == 0))
>>         __audit_seccomp(syscall, signr, action);
>>     return 0;
>
> That makes sense.

And, heh, reading it again now, my example should be ==
SECCOMP_RET_ALLOW (which is 0, yes, but eek raw number, bad me).

>>> +/* Largest strlen() of all action names */
>>> +#define SECCOMP_RET_MAX_NAME_LEN       5
>>
>> This feels fragile... though I don't have a good suggestion yet. :P
>
> I agree and I also don't have a good solution. I didn't like having to
> hard code it.

Yeah. Hrmpf. I mean, it could be sizeof(seccomp_actions_avail) ...
that'll always be long enough. :) But it's a bit of stack over-kill,
but ... is that so bad? I dunno.

>> In the hopes of some day making the sysctl table entirely read-only,
>> can you add some fancy crap here for me? :) See
>> security/yama/yama_lsm.c's yama_dointvec_minmax(), which uses a copy
>> of the sysctl table on the stack.
>
> Will do. I'll deviate slightly from yama_dointvec_minmax(). To make it
> clear that the ctl_table param shouldn't be modified, I'm going to name
> it ro_table and then the stack variable will be named table.

Sounds great, thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Kees Cook]

On Fri, Feb 10, 2017 at 4:01 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 02/07/2017 06:33 PM, Kees Cook wrote:
>> This adds to UAPI, so it'd be good to think for a moment about how
>> this would work on older kernels: right now, if someone tried to use
>> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
>> this sane?
>
> It is not sane for userspace code to blindly attempt to use a new
> feature on an old kernel. One of the main motivations of the
> actions_avail sysctl is to allow userspace to be smart about what the
> current kernel supports.

Yeah, agreed. I mean, userspace could also build a little test
program, toss in RET_LOG, run it and see if it get SIGSYS. But that's
so much more pain that checking in /proc.

> I'll be adding logic (requested by Paul) to libseccomp that checks this
> sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
> use libseccomp will have to do something similar.

Excellent, I had been meaning to ask if you'd chatted with Paul at
all, since this is an API addition for libseccomp. Speaking of which,
can you CC linux-api@ on the next version too?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Kees Cook]

On Fri, Feb 10, 2017 at 4:01 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 02/07/2017 06:33 PM, Kees Cook wrote:
>> This adds to UAPI, so it'd be good to think for a moment about how
>> this would work on older kernels: right now, if someone tried to use
>> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
>> this sane?
>
> It is not sane for userspace code to blindly attempt to use a new
> feature on an old kernel. One of the main motivations of the
> actions_avail sysctl is to allow userspace to be smart about what the
> current kernel supports.

Yeah, agreed. I mean, userspace could also build a little test
program, toss in RET_LOG, run it and see if it get SIGSYS. But that's
so much more pain that checking in /proc.

> I'll be adding logic (requested by Paul) to libseccomp that checks this
> sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
> use libseccomp will have to do something similar.

Excellent, I had been meaning to ask if you'd chatted with Paul at
all, since this is an API addition for libseccomp. Speaking of which,
can you CC linux-api@ on the next version too?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 1488 bytes --]

On 02/10/2017 06:08 PM, Kees Cook wrote:
> On Fri, Feb 10, 2017 at 4:01 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> On 02/07/2017 06:33 PM, Kees Cook wrote:
>>> This adds to UAPI, so it'd be good to think for a moment about how
>>> this would work on older kernels: right now, if someone tried to use
>>> this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is
>>> this sane?
>>
>> It is not sane for userspace code to blindly attempt to use a new
>> feature on an old kernel. One of the main motivations of the
>> actions_avail sysctl is to allow userspace to be smart about what the
>> current kernel supports.
> 
> Yeah, agreed. I mean, userspace could also build a little test
> program, toss in RET_LOG, run it and see if it get SIGSYS. But that's
> so much more pain that checking in /proc.
> 
>> I'll be adding logic (requested by Paul) to libseccomp that checks this
>> sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't
>> use libseccomp will have to do something similar.
> 
> Excellent, I had been meaning to ask if you'd chatted with Paul at
> all, since this is an API addition for libseccomp.

We talked through some of it after the initial PR that I submitted to
libseccomp:

  https://github.com/seccomp/libseccomp/pull/64

I'll be updating that as we get closer to a land-able set of kernel patches.

> Speaking of which, can you CC linux-api@ on the next version too?

Yes, good idea!

Tyler



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Tyler Hicks]

[-- Attachment #1.1: Type: text/plain, Size: 3418 bytes --]

On 02/07/2017 06:43 PM, Kees Cook wrote:
> On Tue, Feb 7, 2017 at 4:25 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> On 02/07/2017 06:03 PM, Kees Cook wrote:
>>> On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>>> This patch creates a read-only sysctl containing an ordered list of
>>>> seccomp actions that the kernel supports. The ordering, from left to
>>>> right, is the lowest action value (kill) to the highest action value
>>>> (allow). Currently, a read of the sysctl file would return "kill trap
>>>> errno trace allow". The contents of this sysctl file can be useful for
>>>> userspace code as well as the system administrator.
>>>>
>>>> The path to the sysctl is:
>>>>
>>>>   /proc/sys/kernel/seccomp/actions_avail
>>>>
>>>> libseccomp and other userspace code can easily determine which actions
>>>> the current kernel supports. The set of actions supported by the current
>>>> kernel may be different than the set of action macros found in kernel
>>>> headers that were installed where the userspace code was built.
>>>
>>> This is certainly good: having a discoverable way to detect filter
>>> capabilities. I do wonder if it'd still be easier to just expose the
>>> max_log sysctl as a numeric value, since the SECCOMP_RET_* values are
>>> all part of uapi, so we can't escape their values...
>>
>> I was very torn on whether to use a numeric or string representation
>> here. The reason I decided on string representation is because I think
>> these sysctls are mostly aimed for admins and numeric representations
>> wouldn't be easy to use. I considered added a utility to libseccomp but,
>> since the kernel code to do a string representation was so simple, I
>> went with doing it in the kernel.
> 
> Yeah, I think I like it just because it gives a way to discover the
> UAPI "level"... I will think more about this. For v3, let's keep the
> string stuff.
> 
>> Another possibility is exposing the SECCOMP_RET_*_NAME macros as part of
>> the uapi.
> 
> I like keeping the UAPI minimal. ;)
> 
>>>> +static int __init seccomp_sysctl_init(void)
>>>> +{
>>>> +       struct ctl_table_header *hdr;
>>>> +
>>>> +       hdr = register_sysctl_paths(seccomp_sysctl_path, seccomp_sysctl_table);
>>>> +       kmemleak_not_leak(hdr);
>>>
>>> Will kmemleak complain about this if hdr is saved to a global (or not
>>> saved at all)? Also, something should be reported in the failure
>>> case...
>>
>> I have to admit to blindly following the example set by sysctl_init() in
>> kernel/sysctl.c. I can test what kmemleak will/won't complain about and
>> report back (tomorrow at the earliest).
> 
> Cool, no rush. I'm backlogged on reviews anyway. :)

kmemleak doesn't complain if we save it to a global. That makes sense
because it means that we have a persistent reference to the allocated
memory.

However, kmemleak doesn't complain about this allocation as-is (meaning
that I simply removed the call to kmemleak_not_leak()). From what I can
tell, this is because a reference to the allocated ctl_table_header
struct is saved when __register_sysctl_table() calls init_header(). I
think kmemleak is seeing this reference when doing scans and
(incorrectly) thinking that there's no leak.

I think the safest/cleanest thing to do is leave the call to
kmemleak_not_leak(). Let me know if you disagree.

Tyler


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 1/4] seccomp: Add sysctl to display available actions

[Kees Cook]

On Mon, Feb 13, 2017 at 4:25 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> kmemleak doesn't complain if we save it to a global. That makes sense
> because it means that we have a persistent reference to the allocated
> memory.
>
> However, kmemleak doesn't complain about this allocation as-is (meaning
> that I simply removed the call to kmemleak_not_leak()). From what I can
> tell, this is because a reference to the allocated ctl_table_header
> struct is saved when __register_sysctl_table() calls init_header(). I
> think kmemleak is seeing this reference when doing scans and
> (incorrectly) thinking that there's no leak.
>
> I think the safest/cleanest thing to do is leave the call to
> kmemleak_not_leak(). Let me know if you disagree.

Okay, that's cool. :) Thanks for checking!

-Kees

-- 
Kees Cook
Pixel Security