[PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions

Re: [PATCH 06/22] audit: Abstract...

[PATCH 0/2] Support auditing while...

Tyler Hicks

Monday, 2 January 2017 Mon, 2 Jan '17

10:53 a.m.

This patch set creates the basis for auditing information specific to a given seccomp return action and then starts auditing SECCOMP_RET_ERRNO return actions. The audit messages for SECCOMP_RET_ERRNO return actions include the errno value that will be returned to userspace. Tyler

Show replies by date

Tyler Hicks

Monday, 2 January Mon, 2 Jan

10:53 a.m.

New subject: [PATCH 1/2] seccomp: Allow for auditing functionality specific to return actions

This patch introduces the concept of auditing formats that are specific to the action specified in a filter's return value. Initially, only SECCOMP_RET_KILL has an auditing message that differs from other return actions because it specifies the signal that is to be sent. This patch causes a small functional change in that "sig=0" is not printed when auditing seccomp actions other than SECCOMP_RET_KILL. Signed-off-by: Tyler Hicks <tyhicks(a)canonical.com> --- include/linux/audit.h | 39 +++++++++++++++++++++++++++++++++------ kernel/auditsc.c | 19 +++++++++++++++---- kernel/seccomp.c | 6 +++--- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index f51fca8d..8c588c3 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -85,6 +85,11 @@ struct audit_field { u32 op; }; +struct audit_seccomp_info { + int code; + long signr; +}; + extern int is_audit_feature_set(int which); extern int __init audit_register_class(int class, unsigned *list); @@ -243,7 +248,8 @@ extern void __audit_file(const struct file *); extern void __audit_inode_child(struct inode *parent, const struct dentry *dentry, const unsigned char type); -extern void __audit_seccomp(unsigned long syscall, long signr, int code); +extern void __audit_seccomp(unsigned long syscall, + struct audit_seccomp_info *info); extern void __audit_ptrace(struct task_struct *t); static inline bool audit_dummy_context(void) @@ -313,14 +319,31 @@ static inline void audit_inode_child(struct inode *parent, } void audit_core_dumps(long signr); -static inline void audit_seccomp(unsigned long syscall, long signr, int code) +static inline void audit_seccomp_signal(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())) - __audit_seccomp(syscall, signr, code); + if (signr || unlikely(!audit_dummy_context())) { + struct audit_seccomp_info info = { .code = code, + .signr = signr }; + + __audit_seccomp(syscall, &info); + } +} + +static inline void audit_seccomp_common(unsigned long syscall, int code) +{ + if (!audit_enabled) + return; + + if (code || unlikely(!audit_dummy_context())) { + struct audit_seccomp_info info = { .code = code }; + + __audit_seccomp(syscall, &info); + } } static inline void audit_ptrace(struct task_struct *t) @@ -485,9 +508,13 @@ static inline void audit_inode_child(struct inode *parent, { } static inline void audit_core_dumps(long signr) { } -static inline void __audit_seccomp(unsigned long syscall, long signr, int code) +static inline void __audit_seccomp(unsigned long syscall, + struct audit_seccomp_info *info); +{ } +static inline void audit_seccomp_signal(unsigned long syscall, long signr, + int code) { } -static inline void audit_seccomp(unsigned long syscall, long signr, int code) +static inline void audit_seccomp_common(unsigned long syscall, int code) { } static inline int auditsc_get_stamp(struct audit_context *ctx, struct timespec *t, unsigned int *serial) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index cf1fa43..b3472f2 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -74,6 +74,7 @@ #include <linux/string.h> #include <linux/uaccess.h> #include <uapi/linux/limits.h> +#include <uapi/linux/seccomp.h> #include "audit.h" @@ -2415,7 +2416,7 @@ void audit_core_dumps(long signr) audit_log_end(ab); } -void __audit_seccomp(unsigned long syscall, long signr, int code) +void __audit_seccomp(unsigned long syscall, struct audit_seccomp_info *info) { struct audit_buffer *ab; @@ -2423,9 +2424,19 @@ void __audit_seccomp(unsigned long syscall, long signr, int code) if (unlikely(!ab)) return; audit_log_task(ab); - audit_log_format(ab, " sig=%ld arch=%x syscall=%ld compat=%d ip=0x%lx code=0x%x", - signr, syscall_get_arch(), syscall, - in_compat_syscall(), KSTK_EIP(current), code); + + switch (info->code) { + case SECCOMP_RET_KILL: + audit_log_format(ab, " sig=%ld", info->signr); + break; + default: + break; + } + + audit_log_format(ab, + " arch=%x syscall=%ld compat=%d ip=0x%lx code=0x%x", + syscall_get_arch(), syscall, in_compat_syscall(), + KSTK_EIP(current), info->code); audit_log_end(ab); } diff --git a/kernel/seccomp.c b/kernel/seccomp.c index f7ce79a..54c01b6 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -532,7 +532,7 @@ static void __secure_computing_strict(int this_syscall) #ifdef SECCOMP_DEBUG dump_stack(); #endif - audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL); + audit_seccomp_signal(this_syscall, SIGKILL, SECCOMP_RET_KILL); do_exit(SIGKILL); } @@ -635,14 +635,14 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, case SECCOMP_RET_KILL: default: - audit_seccomp(this_syscall, SIGSYS, action); + audit_seccomp_signal(this_syscall, SIGSYS, action); do_exit(SIGSYS); } unreachable(); skip: - audit_seccomp(this_syscall, 0, action); + audit_seccomp_common(this_syscall, action); return -1; } #else -- 2.7.4

Tyler Hicks

10:53 a.m.

New subject: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

Generate audit records for SECCOMP_RET_ERRNO actions, which were previously not audited. Additionally, include the errno value that will be set in the audit message. Signed-off-by: Tyler Hicks <tyhicks(a)canonical.com> --- include/linux/audit.h | 19 ++++++++++++++++++- kernel/auditsc.c | 3 +++ kernel/seccomp.c | 4 +++- 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index 8c588c3..6815812 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -87,7 +87,10 @@ struct audit_field { struct audit_seccomp_info { int code; - long signr; + union { + int errno; + long signr; + }; }; extern int is_audit_feature_set(int which); @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode *parent, } void audit_core_dumps(long signr); +static inline void audit_seccomp_errno(unsigned long syscall, int errno, + int code) +{ + if (!audit_enabled) + return; + + if (errno || unlikely(!audit_dummy_context())) { + struct audit_seccomp_info info = { .code = code, + .errno = errno }; + + __audit_seccomp(syscall, &info); + } +} + static inline void audit_seccomp_signal(unsigned long syscall, long signr, int code) { diff --git a/kernel/auditsc.c b/kernel/auditsc.c index b3472f2..db5fc9d 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct audit_seccomp_info *info) audit_log_task(ab); switch (info->code) { + case SECCOMP_RET_ERRNO: + audit_log_format(ab, " errno=%d", info->errno); + break; case SECCOMP_RET_KILL: audit_log_format(ab, " sig=%ld", info->signr); break; diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 54c01b6..e99c566 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, /* Set low-order bits as an errno, capped at MAX_ERRNO. */ if (data > MAX_ERRNO) data = MAX_ERRNO; + + audit_seccomp_errno(this_syscall, data, action); syscall_set_return_value(current, task_pt_regs(current), -data, 0); - goto skip; + return -1; case SECCOMP_RET_TRAP: /* Show the handler the original registers. */ -- 2.7.4

Steve Grubb

11:20 a.m.

New subject: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:

...

"exit" is the field name that syscalls use to return errno to user space. I'd rather not see another field created that maps to the same thing. You can check the translation with the auformat utility: http://people.redhat.com/sgrubb/files/auformat.tar.gz $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat "%EXIT\n" Also, I am working to normalize all the records. That mean every event record of the same type has the same fields, in the same order, with the same representation. I would think "exit" could be added to the current record after syscall so that its ordered similarly to a syscall record. -Steve

...

case SECCOMP_RET_KILL: audit_log_format(ab, " sig=%ld", info->signr); break; diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 54c01b6..e99c566 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd, /* Set low-order bits as an errno, capped at MAX_ERRNO. */ if (data > MAX_ERRNO) data = MAX_ERRNO; + + audit_seccomp_errno(this_syscall, data, action); syscall_set_return_value(current, task_pt_regs(current), -data, 0); - goto skip; + return -1; case SECCOMP_RET_TRAP: /* Show the handler the original registers. */

Tyler Hicks

11:42 a.m.

New subject: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On 2017-01-02 12:20:53, Steve Grubb wrote:

...

On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote: > Generate audit records for SECCOMP_RET_ERRNO actions, which were > previously not audited. > > Additionally, include the errno value that will be set in the audit > message. > > Signed-off-by: Tyler Hicks <tyhicks(a)canonical.com> > --- > include/linux/audit.h | 19 ++++++++++++++++++- > kernel/auditsc.c | 3 +++ > kernel/seccomp.c | 4 +++- > 3 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 8c588c3..6815812 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -87,7 +87,10 @@ struct audit_field { > > struct audit_seccomp_info { > int code; > - long signr; > + union { > + int errno; > + long signr; > + }; > }; > > extern int is_audit_feature_set(int which); > @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode > *parent, } > void audit_core_dumps(long signr); > > +static inline void audit_seccomp_errno(unsigned long syscall, int errno, > + int code) > +{ > + if (!audit_enabled) > + return; > + > + if (errno || unlikely(!audit_dummy_context())) { > + struct audit_seccomp_info info = { .code = code, > + .errno = errno }; > + > + __audit_seccomp(syscall, &info); > + } > +} > + > static inline void audit_seccomp_signal(unsigned long syscall, long signr, > int code) > { > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index b3472f2..db5fc9d 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct > audit_seccomp_info *info) audit_log_task(ab); > > switch (info->code) { > + case SECCOMP_RET_ERRNO: > + audit_log_format(ab, " errno=%d", info->errno); > + break; "exit" is the field name that syscalls use to return errno to user space. I'd rather not see another field created that maps to the same thing. You can check the translation with the auformat utility:

Thanks for having a look at the field name I was using. Although I prefer "errno" over "exit" in terms of clarity, I agree that it makes sense to be consistent with the field names across record types. "exit" works for me.

...

http://people.redhat.com/sgrubb/files/auformat.tar.gz $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat "%EXIT\n" Also, I am working to normalize all the records. That mean every event record of the same type has the same fields, in the same order, with the same representation. I would think "exit" could be added to the current record after syscall so that its ordered similarly to a syscall record.

This patch goes against your normalization efforts in more ways than just the placement of the "exit" field. If the action is SECCOMP_RET_KILL, a "sig" field is present but if the action is SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno" field will be present. This happens all within the AUDIT_SECCOMP record type. How would you suggest normalizing AUDIT_SECCOMP records for different seccomp return actions? Tyler

...

-Steve > case SECCOMP_RET_KILL: > audit_log_format(ab, " sig=%ld", info->signr); > break; > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 54c01b6..e99c566 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const > struct seccomp_data *sd, /* Set low-order bits as an errno, capped at > MAX_ERRNO. */ > if (data > MAX_ERRNO) > data = MAX_ERRNO; > + > + audit_seccomp_errno(this_syscall, data, action); > syscall_set_return_value(current, task_pt_regs(current), > -data, 0); > - goto skip; > + return -1; > > case SECCOMP_RET_TRAP: > /* Show the handler the original registers. */

Steve Grubb

12:49 p.m.

New subject: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Monday, January 2, 2017 5:42:47 PM EST Tyler Hicks wrote:

...

On 2017-01-02 12:20:53, Steve Grubb wrote: > On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote: > > Generate audit records for SECCOMP_RET_ERRNO actions, which were > > previously not audited. > > > > Additionally, include the errno value that will be set in the audit > > message. > > > > Signed-off-by: Tyler Hicks <tyhicks(a)canonical.com> > > --- > > > > include/linux/audit.h | 19 ++++++++++++++++++- > > kernel/auditsc.c | 3 +++ > > kernel/seccomp.c | 4 +++- > > 3 files changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 8c588c3..6815812 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -87,7 +87,10 @@ struct audit_field { > > > > struct audit_seccomp_info { > > > > int code; > > > > - long signr; > > + union { > > + int errno; > > + long signr; > > + }; > > > > }; > > > > extern int is_audit_feature_set(int which); > > > > @@ -319,6 +322,20 @@ static inline void audit_inode_child(struct inode > > *parent, } > > > > void audit_core_dumps(long signr); > > > > +static inline void audit_seccomp_errno(unsigned long syscall, int > > errno, > > + int code) > > +{ > > + if (!audit_enabled) > > + return; > > + > > + if (errno || unlikely(!audit_dummy_context())) { > > + struct audit_seccomp_info info = { .code = code, > > + .errno = errno }; > > + > > + __audit_seccomp(syscall, &info); > > + } > > +} > > + > > > > static inline void audit_seccomp_signal(unsigned long syscall, long > > signr, > > > > int code) > > > > { > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index b3472f2..db5fc9d 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2426,6 +2426,9 @@ void __audit_seccomp(unsigned long syscall, struct > > audit_seccomp_info *info) audit_log_task(ab); > > > > switch (info->code) { > > > > + case SECCOMP_RET_ERRNO: > > + audit_log_format(ab, " errno=%d", info->errno); > > + break; > > "exit" is the field name that syscalls use to return errno to user space. > I'd rather not see another field created that maps to the same thing. You > can check > the translation with the auformat utility: Thanks for having a look at the field name I was using. Although I prefer "errno" over "exit" in terms of clarity, I agree that it makes sense to be consistent with the field names across record types. "exit" works for me. > http://people.redhat.com/sgrubb/files/auformat.tar.gz > > $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat > "%EXIT\n" > > Also, I am working to normalize all the records. That mean every event > record of the same type has the same fields, in the same order, with the > same representation. I would think "exit" could be added to the current > record after syscall so that its ordered similarly to a syscall record. This patch goes against your normalization efforts in more ways than just the placement of the "exit" field. If the action is SECCOMP_RET_KILL, a "sig" field is present but if the action is SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno" field will be present. This happens all within the AUDIT_SECCOMP record type. How would you suggest normalizing AUDIT_SECCOMP records for different seccomp return actions?

Typically when the layout has to change, we just give it a new record type. -Steve

...

> > case SECCOMP_RET_KILL: > > audit_log_format(ab, " sig=%ld", info->signr); > > break; > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index 54c01b6..e99c566 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -576,9 +576,11 @@ static int __seccomp_filter(int this_syscall, const > > struct seccomp_data *sd, /* Set low-order bits as an errno, capped at > > MAX_ERRNO. */ > > > > if (data > MAX_ERRNO) > > > > data = MAX_ERRNO; > > > > + > > + audit_seccomp_errno(this_syscall, data, action); > > > > syscall_set_return_value(current, task_pt_regs(current), > > > > -data, 0); > > > > - goto skip; > > + return -1; > > > > case SECCOMP_RET_TRAP: > > /* Show the handler the original registers. */

Paul Moore

4:55 p.m.

New subject: [PATCH 2/2] seccomp: Audit SECCOMP_RET_ERRNO actions with errno values

On Mon, Jan 2, 2017 at 1:49 PM, Steve Grubb <sgrubb(a)redhat.com> wrote:

...

On Monday, January 2, 2017 5:42:47 PM EST Tyler Hicks wrote: > On 2017-01-02 12:20:53, Steve Grubb wrote: > > On Monday, January 2, 2017 4:53:10 PM EST Tyler Hicks wrote:

...

> Thanks for having a look at the field name I was using. Although I > prefer "errno" over "exit" in terms of clarity, I agree that it makes > sense to be consistent with the field names across record types. "exit" > works for me.

FWIW, we have a nice (searchable due to GitHub CSV magic) audit field database at the link below. I will admit that it may be a bit crusty in places, but we are making a new effort to keep it updated, if you notice anything wrong, send email and/or a PR. * https://github.com/linux-audit/audit-documentation/blob/master/specs/fiel...

...

> > http://people.redhat.com/sgrubb/files/auformat.tar.gz > > > > $ ausearch --start today --just-one -m syscall -sv no --raw | ./auformat > > "%EXIT\n" > > > > Also, I am working to normalize all the records. That mean every event > > record of the same type has the same fields, in the same order, with the > > same representation. I would think "exit" could be added to the current > > record after syscall so that its ordered similarly to a syscall record. > > This patch goes against your normalization efforts in more ways than > just the placement of the "exit" field. If the action is > SECCOMP_RET_KILL, a "sig" field is present but if the action is > SECCOMP_RET_ERRNO, the "sig" field will not be present but the "errno" > field will be present. This happens all within the AUDIT_SECCOMP record > type. How would you suggest normalizing AUDIT_SECCOMP records for > different seccomp return actions? Typically when the layout has to change, we just give it a new record type.

I'm going to be very loathe to accept any new record types that *only* reorder fields; if you need to add a new field, simply add it to the end of the record. From my perspective new record types are really only an option if we need to remove a field that is bogus/confusing or some other similar case that is not easily solved. New record types are a last resort. -- paul moore www.paul-moore.com

Paul Moore

4:47 p.m.

On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote:

...

I'm replying to this patchset posting because it his my inbox first, but my comments here apply to both this patchset and the other seccomp/audit patchset you posted. In my experience, we have two or three problems (the count varies depending on perspective) when it comes to seccomp filter reporting: 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. My current thinking - forgive me, this has been kicking around in my head for the better part of six months (longer?) and I haven't attempted to code it up - is to create a sysctl knob for a system wide seccomp logging threshold that would be applied to the high 16-bits of *every* triggered action: if the action was at/below the threshold a record would be emitted, otherwise silence. This should resolve problems #1 and #2, and the code should be relatively straightforward and small. As part of the code above, I expect that all seccomp logging would get routed through a single logging function (sort of like a better implementation of the existing audit_seccomp()) that would check the threshold and trigger the logging if needed. This function could be augmented to check for CONFIG_AUDIT and in the case where audit was not built into the kernel, a simple printk could be used to log the seccomp event; solving problem #3. We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel that is important (I personally waffle on this), but I think that is independent of the ideas above. Thoughts? -- paul moore www.paul-moore.com

Andy Lutomirski

11:56 p.m.

On Mon, Jan 2, 2017 at 2:47 PM, Paul Moore <paul(a)paul-moore.com> wrote:

...

On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: > This patch set creates the basis for auditing information specific to a given > seccomp return action and then starts auditing SECCOMP_RET_ERRNO return > actions. The audit messages for SECCOMP_RET_ERRNO return actions include the > errno value that will be returned to userspace. I'm replying to this patchset posting because it his my inbox first, but my comments here apply to both this patchset and the other seccomp/audit patchset you posted. In my experience, we have two or three problems (the count varies depending on perspective) when it comes to seccomp filter reporting: 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. My current thinking - forgive me, this has been kicking around in my head for the better part of six months (longer?) and I haven't attempted to code it up - is to create a sysctl knob for a system wide seccomp logging threshold that would be applied to the high 16-bits of *every* triggered action: if the action was at/below the threshold a record would be emitted, otherwise silence. This should resolve problems #1 and #2, and the code should be relatively straightforward and small. As part of the code above, I expect that all seccomp logging would get routed through a single logging function (sort of like a better implementation of the existing audit_seccomp()) that would check the threshold and trigger the logging if needed. This function could be augmented to check for CONFIG_AUDIT and in the case where audit was not built into the kernel, a simple printk could be used to log the seccomp event; solving problem #3.

Would this not be doable with a seccomp tracepoint and a BPF filter? --Andy

Paul Moore

Tuesday, 3 January Tue, 3 Jan

1:31 p.m.

On Tue, Jan 3, 2017 at 12:56 AM, Andy Lutomirski <luto(a)amacapital.net> wrote:

...

On Mon, Jan 2, 2017 at 2:47 PM, Paul Moore <paul(a)paul-moore.com> wrote: > On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: >> This patch set creates the basis for auditing information specific to a given >> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return >> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the >> errno value that will be returned to userspace. > > I'm replying to this patchset posting because it his my inbox first, > but my comments here apply to both this patchset and the other > seccomp/audit patchset you posted. > > In my experience, we have two or three problems (the count varies > depending on perspective) when it comes to seccomp filter reporting: > > 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. > > My current thinking - forgive me, this has been kicking around in my > head for the better part of six months (longer?) and I haven't > attempted to code it up - is to create a sysctl knob for a system wide > seccomp logging threshold that would be applied to the high 16-bits of > *every* triggered action: if the action was at/below the threshold a > record would be emitted, otherwise silence. This should resolve > problems #1 and #2, and the code should be relatively straightforward > and small. > > As part of the code above, I expect that all seccomp logging would get > routed through a single logging function (sort of like a better > implementation of the existing audit_seccomp()) that would check the > threshold and trigger the logging if needed. This function could be > augmented to check for CONFIG_AUDIT and in the case where audit was > not built into the kernel, a simple printk could be used to log the > seccomp event; solving problem #3. Would this not be doable with a seccomp tracepoint and a BPF filter?

One of the motivations for the above idea is to make it easier for admins/users to customize the seccomp logging on their own systems, it's not just to make devs lives easier. I feel okay providing guidance to an admin/user the involves setting a sysctl variable, I can't say the same about asking them to write their own BPF ;) -- paul moore www.paul-moore.com

Tyler Hicks

7:31 a.m.

On 01/02/2017 04:47 PM, Paul Moore wrote:

...

Agreed. Those three logging issues are what have been nagging me the most.

...

My current thinking - forgive me, this has been kicking around in my head for the better part of six months (longer?) and I haven't attempted to code it up - is to create a sysctl knob for a system wide seccomp logging threshold that would be applied to the high 16-bits of *every* triggered action: if the action was at/below the threshold a record would be emitted, otherwise silence. This should resolve problems #1 and #2, and the code should be relatively straightforward and small.

I like that idea quite a bit. To be completely honest, for #1, I personally only care about logging SECCOMP_RET_ERRNO actions but this idea solves it in a nice and general way.

...

As part of the code above, I expect that all seccomp logging would get routed through a single logging function (sort of like a better implementation of the existing audit_seccomp()) that would check the threshold and trigger the logging if needed. This function could be augmented to check for CONFIG_AUDIT and in the case where audit was not built into the kernel, a simple printk could be used to log the seccomp event; solving problem #3.

That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we build with CONFIG_AUDIT enabled but don't ship auditd by default so audit_enabled is false. In that default configuration, we still want seccomp audit messages to be printk'ed. I'll need to figure out how to cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is enabled and audit_enabled is false.

...

We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel that is important (I personally waffle on this), but I think that is independent of the ideas above.

I agree that it is independent but SECCOMP_RET_AUDIT would still be important to Ubuntu. Tyler

Paul Moore

1:42 p.m.

On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote:

...

On 01/02/2017 04:47 PM, Paul Moore wrote: > On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: >> This patch set creates the basis for auditing information specific to a given >> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return >> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the >> errno value that will be returned to userspace. > > I'm replying to this patchset posting because it his my inbox first, > but my comments here apply to both this patchset and the other > seccomp/audit patchset you posted. > > In my experience, we have two or three problems (the count varies > depending on perspective) when it comes to seccomp filter reporting: > > 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. Agreed. Those three logging issues are what have been nagging me the most.

/me nods

...

> My current thinking - forgive me, this has been kicking around in my > head for the better part of six months (longer?) and I haven't > attempted to code it up - is to create a sysctl knob for a system wide > seccomp logging threshold that would be applied to the high 16-bits of > *every* triggered action: if the action was at/below the threshold a > record would be emitted, otherwise silence. This should resolve > problems #1 and #2, and the code should be relatively straightforward > and small. I like that idea quite a bit. To be completely honest, for #1, I personally only care about logging SECCOMP_RET_ERRNO actions but this idea solves it in a nice and general way.

Yeah, I'd much rather solve this problem generally; everybody has their favorite action and I'd like to avoid solving the same problem multiple times. Sooo ... you want to take a whack at coding this up? ;)

...

> As part of the code above, I expect that all seccomp logging would get > routed through a single logging function (sort of like a better > implementation of the existing audit_seccomp()) that would check the > threshold and trigger the logging if needed. This function could be > augmented to check for CONFIG_AUDIT and in the case where audit was > not built into the kernel, a simple printk could be used to log the > seccomp event; solving problem #3. That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we build with CONFIG_AUDIT enabled but don't ship auditd by default so audit_enabled is false. In that default configuration, we still want seccomp audit messages to be printk'ed. I'll need to figure out how to cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is enabled and audit_enabled is false.

Heh, so you've got audit built into the kernel but you're not using it; that sounds "fun". Anyway, I think the logging consolidation could still help you, if for no other reason than everything is going through the same function at that point. We could do some other stuff there to handle the case where audit is compiled, but auditd is not running ... we already have some code in place to handle that for other reasons, check kernel/audit.c for more information. I'd still work on the other stuff first and then we can add this in at the end of the patchset.

...

> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel > that is important (I personally waffle on this), but I think that is > independent of the ideas above. I agree that it is independent but SECCOMP_RET_AUDIT would still be important to Ubuntu. Tyler

-- paul moore www.paul-moore.com

Kees Cook

2:44 p.m.

On Tue, Jan 3, 2017 at 11:42 AM, Paul Moore <paul(a)paul-moore.com> wrote:

...

I think this sounds fine too.

...

>> My current thinking - forgive me, this has been kicking around in my >> head for the better part of six months (longer?) and I haven't >> attempted to code it up - is to create a sysctl knob for a system wide >> seccomp logging threshold that would be applied to the high 16-bits of >> *every* triggered action: if the action was at/below the threshold a >> record would be emitted, otherwise silence. This should resolve >> problems #1 and #2, and the code should be relatively straightforward >> and small. > > I like that idea quite a bit. To be completely honest, for #1, I > personally only care about logging SECCOMP_RET_ERRNO actions but this > idea solves it in a nice and general way. Yeah, I'd much rather solve this problem generally; everybody has their favorite action and I'd like to avoid solving the same problem multiple times. Sooo ... you want to take a whack at coding this up? ;) >> As part of the code above, I expect that all seccomp logging would get >> routed through a single logging function (sort of like a better >> implementation of the existing audit_seccomp()) that would check the >> threshold and trigger the logging if needed. This function could be >> augmented to check for CONFIG_AUDIT and in the case where audit was >> not built into the kernel, a simple printk could be used to log the >> seccomp event; solving problem #3. > > That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we > build with CONFIG_AUDIT enabled but don't ship auditd by default so > audit_enabled is false. In that default configuration, we still want > seccomp audit messages to be printk'ed. I'll need to figure out how to > cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is > enabled and audit_enabled is false. Heh, so you've got audit built into the kernel but you're not using it; that sounds "fun". Anyway, I think the logging consolidation could still help you, if for no other reason than everything is going through the same function at that point. We could do some other stuff there to handle the case where audit is compiled, but auditd is not running ... we already have some code in place to handle that for other reasons, check kernel/audit.c for more information. I'd still work on the other stuff first and then we can add this in at the end of the patchset.

Yeah, I think the "should I report it?" threshold sysctl could just check if audit is enabled... I still wonder, though, isn't there a way to use auditctl to get all the seccomp messages you need? -Kees

...

>> We could also add a SECCOMP_RET_AUDIT, or similar, if we still feel >> that is important (I personally waffle on this), but I think that is >> independent of the ideas above. > > I agree that it is independent but SECCOMP_RET_AUDIT would still be > important to Ubuntu. > > Tyler -- paul moore www.paul-moore.com

-- Kees Cook Nexus Security

Steve Grubb

2:53 p.m.

On Tuesday, January 3, 2017 12:44:41 PM EST Kees Cook wrote:

...

>> That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we >> build with CONFIG_AUDIT enabled but don't ship auditd by default so >> audit_enabled is false. In that default configuration, we still want >> seccomp audit messages to be printk'ed. I'll need to figure out how to >> cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is >> enabled and audit_enabled is false. > > Heh, so you've got audit built into the kernel but you're not using > it; that sounds "fun". > > Anyway, I think the logging consolidation could still help you, if for > no other reason than everything is going through the same function at > that point. We could do some other stuff there to handle the case > where audit is compiled, but auditd is not running ... we already have > some code in place to handle that for other reasons, check > kernel/audit.c for more information. I'd still work on the other > stuff first and then we can add this in at the end of the patchset. Yeah, I think the "should I report it?" threshold sysctl could just check if audit is enabled... I still wonder, though, isn't there a way to use auditctl to get all the seccomp messages you need?

If you do "auditctl -e 1" then auditing will be enabled and it will send events to syslog if the audit daemon is not running. -Steve

Paul Moore

2:54 p.m.

On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote:

...

I still wonder, though, isn't there a way to use auditctl to get all the seccomp messages you need?

Not all of the seccomp actions are currently logged, that's one of the problems (and the biggest at the moment). -- paul moore www.paul-moore.com

Kees Cook

3:03 p.m.

On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote:

...

On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: > I still wonder, though, isn't there a way to use auditctl to get all > the seccomp messages you need? Not all of the seccomp actions are currently logged, that's one of the problems (and the biggest at the moment).

Well... sort of. It all gets passed around, but the logic isn't very obvious (or at least I always have to go look it up). include/linux/audit.h: #ifdef CONFIG_AUDITSYSCALL ... 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())) __audit_seccomp(syscall, signr, code); } ... #else /* CONFIG_AUDITSYSCALL */ static inline void audit_seccomp(unsigned long syscall, long signr, int code) { } ... #endif kernel/seccomp.c: switch (action) { case SECCOMP_RET_ERRNO: /* Set low-order bits as an errno, capped at MAX_ERRNO. */ if (data > MAX_ERRNO) data = MAX_ERRNO; syscall_set_return_value(current, task_pt_regs(current), -data, 0); goto skip; ... case SECCOMP_RET_KILL: default: audit_seccomp(this_syscall, SIGSYS, action); do_exit(SIGSYS); } unreachable(); skip: audit_seccomp(this_syscall, 0, action); Current state: - if CONFIG_AUDITSYSCALL=n, then nothing is ever reported. - if audit is disabled, nothing is ever reported. - if a process isn't being specifically audited (!audit_dummy_context()), only signals (RET_KILL) are reported. - when being specifically audited, everything is reported. So, shouldn't it be possible to specifically audit a process and examine the resulting logs for the RET_* level one is interested in ("code=0x%x" in __audit_seccomp())? -Kees -- Kees Cook Nexus Security

Paul Moore

3:13 p.m.

On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote:

...

On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: > On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >> I still wonder, though, isn't there a way to use auditctl to get all >> the seccomp messages you need? > > Not all of the seccomp actions are currently logged, that's one of the > problems (and the biggest at the moment). Well... sort of. It all gets passed around, but the logic isn't very obvious (or at least I always have to go look it up).

Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at least one other action, but I can't remember which off the top of my head)?

...

include/linux/audit.h: #ifdef CONFIG_AUDITSYSCALL ... 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())) __audit_seccomp(syscall, signr, code); } ... #else /* CONFIG_AUDITSYSCALL */ static inline void audit_seccomp(unsigned long syscall, long signr, int code) { } ... #endif kernel/seccomp.c: switch (action) { case SECCOMP_RET_ERRNO: /* Set low-order bits as an errno, capped at MAX_ERRNO. */ if (data > MAX_ERRNO) data = MAX_ERRNO; syscall_set_return_value(current, task_pt_regs(current), -data, 0); goto skip; ... case SECCOMP_RET_KILL: default: audit_seccomp(this_syscall, SIGSYS, action); do_exit(SIGSYS); } unreachable(); skip: audit_seccomp(this_syscall, 0, action); Current state: - if CONFIG_AUDITSYSCALL=n, then nothing is ever reported. - if audit is disabled, nothing is ever reported. - if a process isn't being specifically audited (!audit_dummy_context()), only signals (RET_KILL) are reported. - when being specifically audited, everything is reported. So, shouldn't it be possible to specifically audit a process and examine the resulting logs for the RET_* level one is interested in ("code=0x%x" in __audit_seccomp())? -Kees -- Kees Cook Nexus Security

-- paul moore www.paul-moore.com

Kees Cook

3:21 p.m.

On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote:

...

On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: > On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: >> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >>> I still wonder, though, isn't there a way to use auditctl to get all >>> the seccomp messages you need? >> >> Not all of the seccomp actions are currently logged, that's one of the >> problems (and the biggest at the moment). > > Well... sort of. It all gets passed around, but the logic isn't very > obvious (or at least I always have to go look it up). Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at least one other action, but I can't remember which off the top of my head)?

Sure, but if you're using audit, you don't need RET_ALLOW to be logged because you'll get a full syscall log entry. Logging RET_ALLOW is redundant and provides no new information, it seems to me. -Kees -- Kees Cook Nexus Security

Paul Moore

3:31 p.m.

On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook(a)chromium.org> wrote:

...

On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote: > On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: >> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>> I still wonder, though, isn't there a way to use auditctl to get all >>>> the seccomp messages you need? >>> >>> Not all of the seccomp actions are currently logged, that's one of the >>> problems (and the biggest at the moment). >> >> Well... sort of. It all gets passed around, but the logic isn't very >> obvious (or at least I always have to go look it up). > > Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at > least one other action, but I can't remember which off the top of my > head)? Sure, but if you're using audit, you don't need RET_ALLOW to be logged because you'll get a full syscall log entry. Logging RET_ALLOW is redundant and provides no new information, it seems to me.

I only bring this up as it might be a way to help solve the SECCOMP_RET_AUDIT problem that Tyler mentioned. -- paul moore www.paul-moore.com

Kees Cook

3:44 p.m.

On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul(a)paul-moore.com> wrote:

...

On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook(a)chromium.org> wrote: > On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote: >> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: >>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>>> I still wonder, though, isn't there a way to use auditctl to get all >>>>> the seccomp messages you need? >>>> >>>> Not all of the seccomp actions are currently logged, that's one of the >>>> problems (and the biggest at the moment). >>> >>> Well... sort of. It all gets passed around, but the logic isn't very >>> obvious (or at least I always have to go look it up). >> >> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at >> least one other action, but I can't remember which off the top of my >> head)? > > Sure, but if you're using audit, you don't need RET_ALLOW to be logged > because you'll get a full syscall log entry. Logging RET_ALLOW is > redundant and provides no new information, it seems to me. I only bring this up as it might be a way to help solve the SECCOMP_RET_AUDIT problem that Tyler mentioned.

So, I guess I want to understand why something like this doesn't work, with no changes at all to the kernel: Imaginary "seccomp-audit.c": ... pid = fork(); if (pid) { char cmd[80]; sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid); system(cmd); release... } else { wait for release... execv(argv[1], argv + 1); } ... This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well as all seccomp actions of any kind. (Down side is the need for root to launch auditctl...) Perhaps an improvement to this could be enabling audit when seccomp syscall is seen? I can't tell if auditctl already has something to do this ("start auditing this process and all children when syscall X is performed"). -Kees -- Kees Cook Nexus Security

Tyler Hicks

7:58 p.m.

On 01/04/2017 04:44 AM, Kees Cook wrote:

...

On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul(a)paul-moore.com> wrote: > On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook(a)chromium.org> wrote: >> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>>>> I still wonder, though, isn't there a way to use auditctl to get all >>>>>> the seccomp messages you need? >>>>> >>>>> Not all of the seccomp actions are currently logged, that's one of the >>>>> problems (and the biggest at the moment). >>>> >>>> Well... sort of. It all gets passed around, but the logic isn't very >>>> obvious (or at least I always have to go look it up). >>> >>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at >>> least one other action, but I can't remember which off the top of my >>> head)? >> >> Sure, but if you're using audit, you don't need RET_ALLOW to be logged >> because you'll get a full syscall log entry. Logging RET_ALLOW is >> redundant and provides no new information, it seems to me. > > I only bring this up as it might be a way to help solve the > SECCOMP_RET_AUDIT problem that Tyler mentioned. So, I guess I want to understand why something like this doesn't work, with no changes at all to the kernel: Imaginary "seccomp-audit.c": ... pid = fork(); if (pid) { char cmd[80]; sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid); system(cmd); release... } else { wait for release... execv(argv[1], argv + 1); } ... This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well as all seccomp actions of any kind. (Down side is the need for root to launch auditctl...)

Hey Kees - Thanks for the suggestion! Here are some of the reasons that it doesn't quite work: 1) We don't install/run auditd by default and would continue to prefer not to in some situations where resources are tight. 2) We block a relatively small number of syscalls as compared to what are allowed so auditing all syscalls is a really heavyweight solution. That could be fixed with a better -S argument, though. 3) We sometimes only block certain arguments for a given syscall and auditing all instances of the syscall could still be a heavyweight solution. 4) If the application spawns children processes, that rule doesn't audit their syscalls. That can be fixed with ppid=%d but then grandchildren pids are a problem. 5) Cleanup of the audit rule for an old pid, before the pid is reused, could be difficult. Tyler

...

Perhaps an improvement to this could be enabling audit when seccomp syscall is seen? I can't tell if auditctl already has something to do this ("start auditing this process and all children when syscall X is performed"). -Kees

Richard Guy Briggs

10:43 p.m.

On 2017-01-04 08:58, Tyler Hicks wrote:

...

On 01/04/2017 04:44 AM, Kees Cook wrote: > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul(a)paul-moore.com> wrote: >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook(a)chromium.org> wrote: >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all >>>>>>> the seccomp messages you need? >>>>>> >>>>>> Not all of the seccomp actions are currently logged, that's one of the >>>>>> problems (and the biggest at the moment). >>>>> >>>>> Well... sort of. It all gets passed around, but the logic isn't very >>>>> obvious (or at least I always have to go look it up). >>>> >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at >>>> least one other action, but I can't remember which off the top of my >>>> head)? >>> >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged >>> because you'll get a full syscall log entry. Logging RET_ALLOW is >>> redundant and provides no new information, it seems to me. >> >> I only bring this up as it might be a way to help solve the >> SECCOMP_RET_AUDIT problem that Tyler mentioned. > > So, I guess I want to understand why something like this doesn't work, > with no changes at all to the kernel: > > Imaginary "seccomp-audit.c": > > ... > pid = fork(); > if (pid) { > char cmd[80]; > > sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid); > system(cmd); > release... > } else { > wait for release... > execv(argv[1], argv + 1); > } > ... > > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well > as all seccomp actions of any kind. (Down side is the need for root to > launch auditctl...) Hey Kees - Thanks for the suggestion! Here are some of the reasons that it doesn't quite work: 1) We don't install/run auditd by default and would continue to prefer not to in some situations where resources are tight. 2) We block a relatively small number of syscalls as compared to what are allowed so auditing all syscalls is a really heavyweight solution. That could be fixed with a better -S argument, though. 3) We sometimes only block certain arguments for a given syscall and auditing all instances of the syscall could still be a heavyweight solution. 4) If the application spawns children processes, that rule doesn't audit their syscalls. That can be fixed with ppid=%d but then grandchildren pids are a problem.

This patch that wasn't accepted upstream might be useful: https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html

...

5) Cleanup of the audit rule for an old pid, before the pid is reused, could be difficult. Tyler > Perhaps an improvement to this could be enabling audit when seccomp > syscall is seen? I can't tell if auditctl already has something to do > this ("start auditing this process and all children when syscall X is > performed"). > > -Kees

- RGB -- Richard Guy Briggs <rgb(a)redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635

Kees Cook

Wednesday, 4 January Wed, 4 Jan

12:31 a.m.

On Tue, Jan 3, 2017 at 8:43 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:

...

On 2017-01-04 08:58, Tyler Hicks wrote: > On 01/04/2017 04:44 AM, Kees Cook wrote: > > On Tue, Jan 3, 2017 at 1:31 PM, Paul Moore <paul(a)paul-moore.com> wrote: > >> On Tue, Jan 3, 2017 at 4:21 PM, Kees Cook <keescook(a)chromium.org> wrote: > >>> On Tue, Jan 3, 2017 at 1:13 PM, Paul Moore <paul(a)paul-moore.com> wrote: > >>>> On Tue, Jan 3, 2017 at 4:03 PM, Kees Cook <keescook(a)chromium.org> wrote: > >>>>> On Tue, Jan 3, 2017 at 12:54 PM, Paul Moore <paul(a)paul-moore.com> wrote: > >>>>>> On Tue, Jan 3, 2017 at 3:44 PM, Kees Cook <keescook(a)chromium.org> wrote: > >>>>>>> I still wonder, though, isn't there a way to use auditctl to get all > >>>>>>> the seccomp messages you need? > >>>>>> > >>>>>> Not all of the seccomp actions are currently logged, that's one of the > >>>>>> problems (and the biggest at the moment). > >>>>> > >>>>> Well... sort of. It all gets passed around, but the logic isn't very > >>>>> obvious (or at least I always have to go look it up). > >>>> > >>>> Last time I checked SECCOMP_RET_ALLOW wasn't logged (as well as at > >>>> least one other action, but I can't remember which off the top of my > >>>> head)? > >>> > >>> Sure, but if you're using audit, you don't need RET_ALLOW to be logged > >>> because you'll get a full syscall log entry. Logging RET_ALLOW is > >>> redundant and provides no new information, it seems to me. > >> > >> I only bring this up as it might be a way to help solve the > >> SECCOMP_RET_AUDIT problem that Tyler mentioned. > > > > So, I guess I want to understand why something like this doesn't work, > > with no changes at all to the kernel: > > > > Imaginary "seccomp-audit.c": > > > > ... > > pid = fork(); > > if (pid) { > > char cmd[80]; > > > > sprintf(cmd, "auditctl -a always,exit -S all -F pid=%d", pid); > > system(cmd); > > release... > > } else { > > wait for release... > > execv(argv[1], argv + 1); > > } > > ... > > > > This should dump all syscalls (both RET_ALLOW and RET_ERRNO), as well > > as all seccomp actions of any kind. (Down side is the need for root to > > launch auditctl...) > > Hey Kees - Thanks for the suggestion! > > Here are some of the reasons that it doesn't quite work: > > 1) We don't install/run auditd by default and would continue to prefer > not to in some situations where resources are tight.

Strictly speaking, auditd isn't needed for auditctl, IIUC.

...

> 2) We block a relatively small number of syscalls as compared to what > are allowed so auditing all syscalls is a really heavyweight solution. > That could be fixed with a better -S argument, though.

Yeah, it seems like there needs to be some kind of improvement there on the audit side (I was thinking a better -F). The all-or-nothing approach is way too big a hammer.

...

> 3) We sometimes only block certain arguments for a given syscall and > auditing all instances of the syscall could still be a heavyweight solution. > > 4) If the application spawns children processes, that rule doesn't audit > their syscalls. That can be fixed with ppid=%d but then grandchildren > pids are a problem. This patch that wasn't accepted upstream might be useful: https://www.redhat.com/archives/linux-audit/2015-August/msg00067.html https://www.redhat.com/archives/linux-audit/2015-August/msg00068.html

I'd like this regardless. It's really difficult to audit trees of processes before they launch. :)

...

> 5) Cleanup of the audit rule for an old pid, before the pid is reused, > could be difficult. > > Tyler > > > Perhaps an improvement to this could be enabling audit when seccomp > > syscall is seen? I can't tell if auditctl already has something to do > > this ("start auditing this process and all children when syscall X is > > performed"). > > > > -Kees - RGB -- Richard Guy Briggs <rgb(a)redhat.com> Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635

-Kees -- Kees Cook Nexus Security

Tyler Hicks

Tuesday, 3 January Tue, 3 Jan

8:04 p.m.

On 01/04/2017 02:42 AM, Paul Moore wrote:

...

On Tue, Jan 3, 2017 at 8:31 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: > On 01/02/2017 04:47 PM, Paul Moore wrote: >> On Mon, Jan 2, 2017 at 11:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: >>> This patch set creates the basis for auditing information specific to a given >>> seccomp return action and then starts auditing SECCOMP_RET_ERRNO return >>> actions. The audit messages for SECCOMP_RET_ERRNO return actions include the >>> errno value that will be returned to userspace. >> >> I'm replying to this patchset posting because it his my inbox first, >> but my comments here apply to both this patchset and the other >> seccomp/audit patchset you posted. >> >> In my experience, we have two or three problems (the count varies >> depending on perspective) when it comes to seccomp filter reporting: >> >> 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. > > Agreed. Those three logging issues are what have been nagging me the most. /me nods >> My current thinking - forgive me, this has been kicking around in my >> head for the better part of six months (longer?) and I haven't >> attempted to code it up - is to create a sysctl knob for a system wide >> seccomp logging threshold that would be applied to the high 16-bits of >> *every* triggered action: if the action was at/below the threshold a >> record would be emitted, otherwise silence. This should resolve >> problems #1 and #2, and the code should be relatively straightforward >> and small. > > I like that idea quite a bit. To be completely honest, for #1, I > personally only care about logging SECCOMP_RET_ERRNO actions but this > idea solves it in a nice and general way. Yeah, I'd much rather solve this problem generally; everybody has their favorite action and I'd like to avoid solving the same problem multiple times. Sooo ... you want to take a whack at coding this up? ;)

Yes, I can do that.

...

>> As part of the code above, I expect that all seccomp logging would get >> routed through a single logging function (sort of like a better >> implementation of the existing audit_seccomp()) that would check the >> threshold and trigger the logging if needed. This function could be >> augmented to check for CONFIG_AUDIT and in the case where audit was >> not built into the kernel, a simple printk could be used to log the >> seccomp event; solving problem #3. > > That doesn't fully solve #3 for me. In Ubuntu (and I think Debian), we > build with CONFIG_AUDIT enabled but don't ship auditd by default so > audit_enabled is false. In that default configuration, we still want > seccomp audit messages to be printk'ed. I'll need to figure out how to > cleanly allow opting into seccomp audit messages when CONFIG_AUDIT is > enabled and audit_enabled is false. Heh, so you've got audit built into the kernel but you're not using it; that sounds "fun".

Users that need full auditing functionality can simply install auditd but most users don't require it. Ubuntu has done it this way for many years and the lack of seccomp auditing when auditd isn't running has been the only problem that I can remember.

...

Anyway, I think the logging consolidation could still help you, if for no other reason than everything is going through the same function at that point. We could do some other stuff there to handle the case where audit is compiled, but auditd is not running ... we already have some code in place to handle that for other reasons, check kernel/audit.c for more information. I'd still work on the other stuff first and then we can add this in at the end of the patchset.

/me nods. It is easy to continue to distro patch out the recently added check for audit_enabled until a better solution for all distros can be identified/upstreamed. Thanks again! Tyler

...

Andy Lutomirski

Monday, 2 January Mon, 2 Jan

11:57 p.m.

On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote:

...

Not that I'm opposed to the idea, but what's the intended purpose?

Tyler Hicks

Tuesday, 3 January Tue, 3 Jan

7:53 a.m.

On 01/02/2017 11:57 PM, Andy Lutomirski wrote:

...

On Mon, Jan 2, 2017 at 8:53 AM, Tyler Hicks <tyhicks(a)canonical.com> wrote: > This patch set creates the basis for auditing information specific to a given > seccomp return action and then starts auditing SECCOMP_RET_ERRNO return > actions. The audit messages for SECCOMP_RET_ERRNO return actions include the > errno value that will be returned to userspace. > Not that I'm opposed to the idea, but what's the intended purpose?

Ubuntu has a security sandbox, which includes seccomp as a part of the confinement strategy, that we're using to confine untrusted third-party applications. Today, we're using SECCOMP_RET_KILL as the default action when the applications make a call to a syscall that is not allowed by the sandbox. It is great from a security perspective but not so great from the perspective of the application developer as their application (or in some cases, an interpretor) may work fine without the illegal syscall but it doesn't get the chance to because it is killed. In the near future, we want to switch over to using SECCOMP_RET_ERRNO (the errno is still TBD) as the default action to improve the application developer experience. The largest remaining blocker is that there are no audit messages when a SECCOMP_RET_ERRNO action is taken. Therefore, we can't suggest (to the application developer or to the user) which sandbox knobs need to be turned to better suite their application, we can't let the application developer know that a syscall they're using is illegal outside of them having to debug an odd errno value, and we can't let the user know of a potentially subverted process that's under confinement of the sandbox. All of that could be addressed if SECCOMP_RET_ERRNO actions generated audit messages. I hope that helps to understand the use case. Tyler

2908

days inactive

2910

days old

linux-audit@lists.linux-audit.osci.io

Manage subscription

25 comments

7 participants

tags (0)

participants (7)

Andy Lutomirski
Andy Lutomirski
Kees Cook
Paul Moore
Richard Guy Briggs
Steve Grubb
Tyler Hicks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

[PATCH 0/2] Begin auditing SECCOMP_RET_ERRNO return actions