From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tyler Hicks Subject: Re: Limiting SECCOMP audit events Date: Fri, 15 Dec 2017 15:47:14 +0000 Message-ID: <20171215154713.GA16170@sec> References: <58203247.sCqcla2mis@x2> <36cd827f-201c-8f76-2883-ecd930cbb1f4@canonical.com> <3499769.OM7YpPIT3e@x2> <20171214230629.GA451@sec> <1605a80e588.280e.85c95baa4474aabc7814e68940a78392@paul-moore.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8718256544430125752==" Return-path: In-Reply-To: <1605a80e588.280e.85c95baa4474aabc7814e68940a78392@paul-moore.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore , Steve Grubb Cc: Linux Audit List-Id: linux-audit@redhat.com --===============8718256544430125752== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="rwEMma7ioTxnRzrJ" Content-Disposition: inline --rwEMma7ioTxnRzrJ Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 12/15/2017 08:08 AM, Paul Moore wrote: > On December 14, 2017 6:06:49 PM Tyler Hicks wrote: >=20 >> 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 =3D kill_process kill_thread errno >>> >>>> >>> >>>> I agree with Kees here. IMO, you only want "kill_process kill_thread" >>> >>>> which is the default. >>> >>> =A0 >>> >>> 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 =3D=3D RET_ALLOW: >> do not log >> else if action =3D=3D RET_KILL && RET_KILL in actions_logged: >> log >> else if action =3D=3D 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); >> -} >> - >=20 > Looks good to me but two things: >=20 > * Change the name of __audit_seccomp() to audit_seccomp() since we don't > have two functions anymore. >=20 > * 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 =3D=3D RET_ALLOW: do not log else if action =3D=3D RET_KILL && RET_KILL in actions_logged: log else if action =3D=3D 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 =3D=3D RET_ALLOW: do not log else if action not in actions_logged: do not log else if action =3D=3D RET_KILL: log else if action =3D=3D RET_LOG: log else if filter-requests-logging: log else if audit_enabled && process-is-being-audited: log else: do not log Tyler >=20 >> 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, i= nt 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 syscal= l, >> 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 >=20 >=20 --rwEMma7ioTxnRzrJ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJaM+6BAAoJENaSAD2qAscKWm8QAMHHnbcuFMV9D8dZejpjwrmT E54ANOR//JuNmmv/QjHMeasH1+CLOnI611WcxLDl3F5QqUr3IjYUuY0nmNWR/miT Qao4znteRuuX6xQF085EWCScI2aCco1BxrW6DW2t477/5OQADJkyZ5EmWDuvUeBL QJ3raiMf04qbgTX+hghE6xjPJc7/3ZrDJjRhMHBul5YobauoLcgwKTyZWdMyJKW1 UK+IvDlPBe7UvPTSh7KaLPsiV5FB1qtQh4fdCs0UPCdf0BRxfKsUQuY7r8f4swbF klIvfHHRZsdjA/YEp5ZYg+Lju+ddajyRhJuPZXE2YKCnzAhQO4Fxvybol1xxU6hk MC0MW3UrFp3+hj+Wvcc3/PfuvZPVd/QMsRCc4kBdAYV6VA5hJ3IyzUHx1HyPLUat rd7atZldIhEZGT0o5f/i9/uxyI12ZCDHCaiUhzApZitCJwJv5KVHJXwlyyWR81Ke v/EVy6Bo0V/pjY/OgDFKdpCLoNd4CPebyvAjzcbr27o+NJeOYQZFhUJXpVxKcB6L cHmWz46sQ+rWaSWeoztagf20UvtSUhUZE+Ae0FgtMBsEKBlxK/xXm2rAVBU6sTI6 2ZML/Fs2j/XIGhXLastG6BdbxhKIgTTDVyMOn4JlRwyGIpcMk5sExrJh0ofWzinN 0AvS8jagjPW00eMDA8UD =KwxS -----END PGP SIGNATURE----- --rwEMma7ioTxnRzrJ-- --===============8718256544430125752== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============8718256544430125752==--