Re: Limiting SECCOMP audit events

[Steve Grubb <sgrubb@redhat.com>]

On Thursday, December 14, 2017 6:06:30 PM EST Tyler Hicks 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.

Yes, exactly. I have been experiencing large amounts of SECCOMP events 
starting with qt webkit in kde and thought 4.14 would finally let me tame 
those events. I have opened a couple bz asking developers if they really meant 
to go live with a policy that is experiencing so many denials. But the 
consensus is this is intended. (But I think they also have not actually tried 
to use their audit logs.)


> 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.

And here I thought you were also seeing large numbers of seccomp events and 
were making a way to control what gets logged. In any event, I think we better 
understand each other now. :-)


> 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.

OK. Great. With developers starting to use the trap return value, audit logs 
are getting swamped by benign events. We truly need a knob to eliminate the 
noise from the signal.

> 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.

Sure. If you want to add it, then it should be roughly like this:

               struct tty_struct *tty;
               const struct cred *cred;
               struct audit_buffer *ab;
               char comm[sizeof(current->comm)];

               ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
               if (unlikely(!ab))
                       return;

               cred = current_cred();
               tty = audit_get_tty(current);
               audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
                               task_tgid_nr(current),
                               from_kuid(&init_user_ns, cred->uid),
                               from_kuid(&init_user_ns,
                               audit_get_loginuid(current)),
                               tty ? tty_name(tty) : "(none)",
                               audit_get_sessionid(current));
               audit_put_tty(tty);
               audit_log_task_context(ab);
               audit_log_format(ab, " comm=");
               audit_log_untrustedstring(ab, get_task_comm(comm, current));
               audit_log_d_path_exe(ab, current->mm);
               audit_log_format(ab, "op=seccomp-logging");

<You can log the new value here if you wish - just can't have spaces in the 
value. Numbers or mask is fine.>

               audit_log_format(ab, " res=%u", res);

where res above is a 1 for success and 0 for failure. Failure is likely to be 
due to not having the capability that allows setting the sysctl.

> I hope this helps!

Thanks!

-Steve

> 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);
>  }
> 
>  /*