11:37 p.m.
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@canoni...
http://lkml.kernel.org/r/1483377999-15019-2-git-send-email-tyhicks@canoni...
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
11:37 p.m.
New subject: [PATCH v2 1/4] seccomp: Add sysctl to display available actions
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(a)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
6:03 p.m.
New subject: [PATCH v2 1/4] seccomp: Add sysctl to display available actions
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
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...
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...
-Kees
--
Kees Cook
Pixel Security
6:25 p.m.
New subject: [PATCH v2 1/4] seccomp: Add sysctl to display available actions
On 02/07/2017 06:03 PM, Kees Cook wrote:
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.
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
6:43 p.m.
New subject: [PATCH v2 1/4] seccomp: Add sysctl to display available actions
On Tue, Feb 7, 2017 at 4:25 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
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. ;)
Cool, no rush. I'm backlogged on reviews anyway. :)
-Kees
--
Kees Cook
Pixel Security
6:25 p.m.
New subject: [PATCH v2 1/4] seccomp: Add sysctl to display available actions
On 02/07/2017 06:43 PM, Kees Cook 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.
Tyler
11:37 p.m.
New subject: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged
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(a)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
6:24 p.m.
New subject: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
/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. :)
Please mark this inline...
Based on my thoughts below, this test can actually be removed (making
the audit_seccomp() call unconditional), since callers will always be
!= RET_ALLOW.
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;
This line should be in patch 1.
This feels fragile... though I don't have a good suggestion yet. :P
{ } preferred over { 0 }
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.
(Though I still wonder if a numeric would be easier...)
-Kees
--
Kees Cook
Pixel Security
5:56 p.m.
New subject: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged
On 02/07/2017 06:24 PM, Kees Cook wrote:
Kees and I discussed this a bit over IRC today. We settled on
log_max_action for v3 of the patch set.
Will do.
Agreed.
That makes sense.
I agree and I also don't have a good solution. I didn't like having to
hard code it.
No problem.
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
6:05 p.m.
New subject: [PATCH v2 2/4] seccomp: Add sysctl to configure actions that should be logged
On Fri, Feb 10, 2017 at 3:56 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
And, heh, reading it again now, my example should be ==
SECCOMP_RET_ALLOW (which is 0, yes, but eek raw number, bad me).
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.
Sounds great, thanks!
-Kees
--
Kees Cook
Pixel Security
11:37 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
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(a)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
6:33 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com> 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?
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.
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.
-Kees
--
Kees Cook
Pixel Security
3:49 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
On Tue, Feb 7, 2017 at 4:33 PM, Kees Cook <keescook(a)chromium.org> wrote:
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/9...
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
--
Kees Cook
Pixel Security
6:01 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
On 02/07/2017 06:33 PM, Kees Cook wrote:
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.
That makes sense. I'll do that in v3.
Sure! It actually has to be two different cases now that we're doing the
hot path approach for SECCOMP_RET_ALLOW.
Tyler
6:08 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
On Fri, Feb 10, 2017 at 4:01 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
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.
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
6:15 p.m.
New subject: [PATCH v2 3/4] seccomp: Create an action to log before allowing
On 02/10/2017 06:08 PM, Kees Cook wrote:
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
11:37 p.m.
New subject: [PATCH v2 4/4] seccomp: Add tests for SECCOMP_RET_LOG
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(a)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
6:39 p.m.
New subject: [PATCH v2 4/4] seccomp: Add tests for SECCOMP_RET_LOG
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
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
5:58 p.m.
On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhicks(a)canonical.com> wrote:
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
3131
days inactive
3142
days old
linux-audit@lists.linux-audit.osci.io
19 comments
2 participants
participants (2)
-
Kees Cook -
Tyler Hicks