[PATCH v4 0/4] Improved seccomp logging

[PATCH v4 0/4] Improved seccomp logging

[Tyler Hicks]

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

v1: http://lkml.kernel.org/r/1483375990-14948-1-git-send-email-tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org
v1: http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org

v2: http://lkml.kernel.org/r/1486100262-32391-1-git-send-email-tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org

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

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

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

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

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

Tyler


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

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

-- 
2.7.4

[PATCH v4 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>
---
 Documentation/prctl/seccomp_filter.txt | 16 ++++++++++
 Documentation/sysctl/kernel.txt        |  1 +
 kernel/seccomp.c                       | 55 ++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
index 1e469ef..a5554ff 100644
--- a/Documentation/prctl/seccomp_filter.txt
+++ b/Documentation/prctl/seccomp_filter.txt
@@ -166,7 +166,23 @@ The samples/seccomp/ directory contains both an x86-specific example
 and a more generic example of a higher level macro interface for BPF
 program generation.
 
+Sysctls
+-------
+
+Seccomp's sysctl files can be found in the /proc/sys/kernel/seccomp/
+directory. Here's a description of each file in that directory:
+
+actions_avail:
+	A read-only ordered list of seccomp return values (refer to the
+	SECCOMP_RET_* macros above) in string form. The ordering, from
+	left-to-right, is the least permissive return value to the most
+	permissive return value.
 
+	The list represents the set of seccomp return values supported
+	by the kernel. A userspace program may use this list to
+	determine if the actions found in the seccomp.h, when the
+	program was built, differs from the set of actions actually
+	supported in the current running kernel.
 
 Adding architecture support
 -----------------------
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index a32b4b7..56f9b29 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -74,6 +74,7 @@ show up in /proc/sys/kernel:
 - reboot-cmd                  [ SPARC only ]
 - rtsig-max
 - rtsig-nr
+- seccomp/                    ==> Documentation/prctl/seccomp_filter.txt
 - sem
 - sem_next_id		      [ sysv ipc ]
 - sg-big-buff                 [ generic SCSI device (sg) ]
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f7ce79a..e36dfe9 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,56 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 	return ret;
 }
 #endif
+
+#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"
+
+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);
+	if (!hdr)
+		pr_warn("seccomp: sysctl registration failed\n");
+	else
+		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 v4 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/log_max_action

The actions_avail sysctl can be read to discover the valid action names
that can be written to the log_max_action 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 log_max_action. This exception
preserves the existing auditing behavior of tasks with an allocated
audit context.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 Documentation/prctl/seccomp_filter.txt |  21 ++++++
 include/linux/audit.h                  |   6 +-
 kernel/seccomp.c                       | 123 ++++++++++++++++++++++++++++++++-
 3 files changed, 142 insertions(+), 8 deletions(-)

diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/prctl/seccomp_filter.txt
index a5554ff..487cb0c 100644
--- a/Documentation/prctl/seccomp_filter.txt
+++ b/Documentation/prctl/seccomp_filter.txt
@@ -184,6 +184,27 @@ actions_avail:
 	program was built, differs from the set of actions actually
 	supported in the current running kernel.
 
+log_max_action:
+	A read-write file containing the most permissive seccomp return
+	value that should be logged. The list of valid strings that can
+	be written to this file can be found in the actions_avail sysctl.
+
+	The actions_avail sysctl can also serve as a way to discover the
+	ordering of seccomp return values so that you can easily
+	understand which return values will be logged. For example,
+	assume that the actions_avail file contains
+	"kill trap errno trace allow" and "errno" is written to the
+	log_max_action file. The SECCOMP_RET_KILL, SECCOMP_RET_TRAP, and
+	SECCOMP_RET_ERRNO actions will be logged.
+
+	It is important to note that the value of log_max_action does
+	not prevent certain actions from being logged when the audit
+	subsystem is configured to audit a task. If the action is more
+	permissive than what's written to log_max_action, the final
+	decision on whether to audit the action for that task is
+	ultimately left up to the audit subsystem to decide for all
+	seccomp return values other than SECCOMP_RET_ALLOW.
+
 Adding architecture support
 -----------------------
 
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 e36dfe9..270a227 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason)
 }
 #endif	/* CONFIG_SECCOMP_FILTER */
 
+static u32 seccomp_log_max_action = SECCOMP_RET_KILL;
+
+static inline 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_log_max_action)
+		return __audit_seccomp(syscall, signr, action);
+
+	/* Let the audit subsystem decide if the action should be audited based
+	 * on whether the current task itself is being audited.
+	 */
+	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 +550,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 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		return 0;
 
 	case SECCOMP_RET_ALLOW:
+		/* Open-coded seccomp_log(), optimized for the RET_ALLOW hot
+		 * path.
+		 *
+		 * We only want to log RET_ALLOW actions when the admin has
+		 * configured them to be logged via the log_max_action sysctl.
+		 * Therefore, call __audit_seccomp() directly so that RET_ALLOW
+		 * actions are not audited simply because the task is being
+		 * audited.
+		 */
+		if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW))
+			__audit_seccomp(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
@@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 #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_log_max_action_handler(struct ctl_table *ro_table, int write,
+					  void __user *buffer, size_t *lenp,
+					  loff_t *ppos)
+{
+	char name[SECCOMP_RET_MAX_NAME_LEN + 1] = { };
+	struct ctl_table table;
+	int ret;
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!write) {
+		const char *namep;
+
+		if (!seccomp_name_from_action(&namep, seccomp_log_max_action))
+			return -EINVAL;
+
+		strncpy(name, namep, sizeof(name) - 1);
+	}
+
+	table = *ro_table;
+	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_log_max_action = action;
+	}
+
+	return 0;
+}
+
 static struct ctl_path seccomp_sysctl_path[] = {
 	{ .procname = "kernel", },
 	{ .procname = "seccomp", },
@@ -937,6 +1049,11 @@ static struct ctl_table seccomp_sysctl_table[] = {
 		.mode		= 0444,
 		.proc_handler	= proc_dostring,
 	},
+	{
+		.procname	= "log_max_action",
+		.mode		= 0644,
+		.proc_handler	= seccomp_log_max_action_handler,
+	},
 	{ }
 };
 
-- 
2.7.4

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

[Kees Cook]

On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index e36dfe9..270a227 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason)
>  }
>  #endif /* CONFIG_SECCOMP_FILTER */
>
> +static u32 seccomp_log_max_action = SECCOMP_RET_KILL;
> +
> +static inline 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_log_max_action)
> +               return __audit_seccomp(syscall, signr, action);
> +
> +       /* Let the audit subsystem decide if the action should be audited based
> +        * on whether the current task itself is being audited.
> +        */

Nitpick on comment style, please use:

/*
 * line 1
 * line 2...
 */

> +       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 +550,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 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>                 return 0;
>
>         case SECCOMP_RET_ALLOW:
> +               /* Open-coded seccomp_log(), optimized for the RET_ALLOW hot
> +                * path.
> +                *
> +                * We only want to log RET_ALLOW actions when the admin has
> +                * configured them to be logged via the log_max_action sysctl.
> +                * Therefore, call __audit_seccomp() directly so that RET_ALLOW
> +                * actions are not audited simply because the task is being
> +                * audited.
> +                */
> +               if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW))
> +                       __audit_seccomp(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
> @@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>  #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[] = {

As long as I'm nit-picking, this can be const too. :)

-Kees

-- 
Kees Cook
Pixel Security

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

[Tyler Hicks]

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

On 02/15/2017 07:10 PM, Kees Cook wrote:
> On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index e36dfe9..270a227 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason)
>>  }
>>  #endif /* CONFIG_SECCOMP_FILTER */
>>
>> +static u32 seccomp_log_max_action = SECCOMP_RET_KILL;
>> +
>> +static inline 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_log_max_action)
>> +               return __audit_seccomp(syscall, signr, action);
>> +
>> +       /* Let the audit subsystem decide if the action should be audited based
>> +        * on whether the current task itself is being audited.
>> +        */
> 
> Nitpick on comment style, please use:
> 
> /*
>  * line 1
>  * line 2...
>  */

No problem.

> 
>> +       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 +550,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 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>                 return 0;
>>
>>         case SECCOMP_RET_ALLOW:
>> +               /* Open-coded seccomp_log(), optimized for the RET_ALLOW hot
>> +                * path.
>> +                *
>> +                * We only want to log RET_ALLOW actions when the admin has
>> +                * configured them to be logged via the log_max_action sysctl.
>> +                * Therefore, call __audit_seccomp() directly so that RET_ALLOW
>> +                * actions are not audited simply because the task is being
>> +                * audited.
>> +                */
>> +               if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW))
>> +                       __audit_seccomp(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
>> @@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>  #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[] = {
> 
> As long as I'm nit-picking, this can be const too. :)

I'll have to cast to a non-const pointer when assigning ctl_table.data
but I think that's fine in this case.

Tyler

> 
> -Kees
> 



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

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

[Tyler Hicks]

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

On 02/16/2017 12:40 PM, Tyler Hicks wrote:
> On 02/15/2017 07:10 PM, Kees Cook wrote:
>> On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>>> index e36dfe9..270a227 100644
>>> --- a/kernel/seccomp.c
>>> +++ b/kernel/seccomp.c
>>> @@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason)
>>>  }
>>>  #endif /* CONFIG_SECCOMP_FILTER */
>>>
>>> +static u32 seccomp_log_max_action = SECCOMP_RET_KILL;
>>> +
>>> +static inline 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_log_max_action)
>>> +               return __audit_seccomp(syscall, signr, action);
>>> +
>>> +       /* Let the audit subsystem decide if the action should be audited based
>>> +        * on whether the current task itself is being audited.
>>> +        */
>>
>> Nitpick on comment style, please use:
>>
>> /*
>>  * line 1
>>  * line 2...
>>  */
> 
> No problem.
> 
>>
>>> +       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 +550,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 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>>                 return 0;
>>>
>>>         case SECCOMP_RET_ALLOW:
>>> +               /* Open-coded seccomp_log(), optimized for the RET_ALLOW hot
>>> +                * path.
>>> +                *
>>> +                * We only want to log RET_ALLOW actions when the admin has
>>> +                * configured them to be logged via the log_max_action sysctl.
>>> +                * Therefore, call __audit_seccomp() directly so that RET_ALLOW
>>> +                * actions are not audited simply because the task is being
>>> +                * audited.
>>> +                */
>>> +               if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW))
>>> +                       __audit_seccomp(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
>>> @@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>>>  #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[] = {
>>
>> As long as I'm nit-picking, this can be const too. :)
> 
> I'll have to cast to a non-const pointer when assigning ctl_table.data
> but I think that's fine in this case.

I was confused about which array you were talking about. I thought you
were talking about seccomp_actions_avail[].

No casts are needed when making seccomp_action_names[] const.

Tyler


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

[PATCH v4 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                       | 7 +++++++
 3 files changed, 14 insertions(+)

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

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

[Kees Cook]

On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> Extend the kernel selftests for seccomp to test the newly added
> SECCOMP_RET_LOG action. The added tests follow the example of existing
> tests.
>
> Unfortunately, the tests are not capable of inspecting the audit log to
> verify that the syscall was logged.

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

-Kees

-- 
Kees Cook
Pixel Security

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

[Tyler Hicks]

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

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

Sure thing!

Tyler

> 
> -Kees
> 



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

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

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

[Kees Cook]

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

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

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

-Kees

-- 
Kees Cook
Pixel Security