Re: Limiting SECCOMP audit events

[Tyler Hicks <tyhicks@canonical.com>]

On 12/14/2017 09:19 AM, Steve Grubb wrote:
> On Thursday, December 14, 2017 10:04:48 AM EST Tyler Hicks wrote:
> 
>> On 12/13/2017 05:58 PM, Steve Grubb wrote:
> 
>> > Over the last month, the amount of seccomp events in audit logs is
> 
>> > sky-rocketing. I have over a million events in the last 2 days. Most of
> 
>> > this is generated by firefox and qt webkit.
> 
>> >
> 
>> > I am wondering if the audit package should ship a file for
> 
>> >
> 
>> > /usr/lib/sysctl.d/60-auditd.conf
> 
>> >
> 
>> > wherein it has
> 
>> >
> 
>> > kernel.seccomp.actions_logged = kill_process kill_thread errno
> 
>>
> 
>> I agree with Kees here. IMO, you only want "kill_process kill_thread"
> 
>> which is the default.
> 
>  
> 
> The default appears to be all of the types of events without setting
> kernel.seccomp.actions_logged.

Ah, right. I didn't correctly remember the final implementation details.
The default sysctl setting is to allow all actions except for RET_ALLOW
to be logged.

I think the easiest description of the logic is in the commit message of
59f5cf44a38284eb9e76270c786fb6cc62ef8ac4:

    if action == RET_ALLOW:
      do not log
    else if action == RET_KILL && RET_KILL in actions_logged:
      log
    else if action == RET_LOG && RET_LOG in actions_logged:
      log
    else if filter-requests-logging && action in actions_logged:
      log
    else if audit_enabled && process-is-being-audited:
      log
    else:
      do not log

I think I originally misunderstood your first email in this thread. I
thought you were saying that you were experiencing more seccomp audit
events in 4.14 versus 4.13 and that you felt a regression had been
introduced. After rereading, I think you're asking why you're getting
seccomp RET_TRAP actions logged even though "trap" isn't in the
actions_logged sysctl. 

The reason is because I didn't get clear direction from the audit
folks about to do when audit is enabled and the process is being audited
and, therefore, I didn't feel comfortable rocking the boat. In that
situation, the decision to log is the same as it was in earlier kernels.
Specifically, you're hitting the last "else if" conditional in the
pseudocode above.

If you're happy with having the actions_logged sysctl control whether or
not to log seccomp actions taken for processes that are being audited,
then I think the following (untested) patch should do exactly what you
want. I imagine that you'd also want seccomp to emit audit events
whenever the value of the actions_logged sysctl is changed, which should
be pretty easy to do.

I hope this helps!

Tyler

diff --git a/include/linux/audit.h b/include/linux/audit.h
index af410d9..095b5dd 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -304,12 +304,6 @@ 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)
-{
-	if (audit_enabled && unlikely(!audit_dummy_context()))
-		__audit_seccomp(syscall, signr, code);
-}
-
 static inline void audit_ptrace(struct task_struct *t)
 {
 	if (unlikely(!audit_dummy_context()))
@@ -502,8 +496,6 @@ 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, long signr, int code)
-{ }
 static inline int auditsc_get_stamp(struct audit_context *ctx,
 			      struct timespec64 *t, unsigned int *serial)
 {
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 5f0dfb2ab..914a707 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -590,12 +590,6 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
 	 */
 	if (log)
 		return __audit_seccomp(syscall, signr, action);
-
-	/*
-	 * Let the audit subsystem decide if the action should be audited based
-	 * on whether the current task itself is being audited.
-	 */
-	return audit_seccomp(syscall, signr, action);
 }
 
 /*