On 02/07/2017 06:24 PM, Kees Cook wrote: > On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks <tyhi...@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 <tyhi...@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 >
signature.asc
Description: OpenPGP digital signature
-- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit