Re: Limiting SECCOMP audit events

[Tyler Hicks <tyhicks@canonical.com>]

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

On 12/15/2017 08:08 AM, Paul Moore wrote:
> On December 14, 2017 6:06:49 PM Tyler Hicks <tyhicks@canonical.com> wrote:
> 
>> 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);
>> -}
>> -
> 
> Looks good to me but two things:
> 
> * Change the name of __audit_seccomp() to audit_seccomp() since we don't
> have two functions anymore.
> 
> * Are we sure about removing the audit_enabled check? People got pretty
> upset when it wasn't there in the past.

Do you have any references to the complaints so that we can understand
them better? I remember being surprised by commit 96368701 adding the
audit_enabled check (my fault for not watching the list closer) and
having to revert it in Ubuntu with a distro patch.


After sleeping on it for a night, I'm now unsure if the patch I sent in
this thread is what you guys really want. I'll go back to talking in
pseudocode. This is what we have in 4.14:

  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

This is what the patch in this thread does:

--- a/seccomp-log.pseudo
+++ b/seccomp-log.pseudo
@@ -6,7 +6,5 @@
     log
   else if filter-requests-logging && action in actions_logged:
     log
-  else if audit_enabled && process-is-being-audited:
-    log
   else:
     do not log

Instead of that change, now I'm wondering if this is what you really
want:

--- a/seccomp-log.pseudo
+++ b/seccomp-log.pseudo
@@ -6,7 +6,8 @@
     log
   else if filter-requests-logging && action in actions_logged:
     log
-  else if audit_enabled && process-is-being-audited:
+  else if audit_enabled && process-is-being-audited &&
+          action in actions_logged:
     log
   else:
     do not log

After refactoring the 'action in actions_logged' check, it would leave
us with this:

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

Tyler

> 
>>  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);
>>  }
>>
>>  /*
>>
>> --
>> Linux-audit mailing list
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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