Re: [PATCH] audit: always report seccomp violations

[Kees Cook]

On Sun, Mar 25, 2012 at 11:47 AM, Casey Schaufler
<casey@xxxxxxxxxxxxxxxx> wrote:
> On 3/23/2012 4:32 PM, Kees Cook wrote:
>> When a program violates its own seccomp rules, that is a pretty dire
>> situation, and the audit message should always be reported (not just
>> when there is already a rule active for the process).
>
> Hmm. If the program is never going to violate its own
> seccomp rules it seems sort of silly to have them in the
> first place, doesn't it? Oh, I know that the expectation
> of seccomp is that the application would only try something
> you've disallowed if it gets compromised. Problem is that

Well, either compromised or doing something new (e.g. a library in the
code has changed).

> Modern Programmers tend to rely very heavily on the opaque
> behavior of APIs that they don't understand nor particularly
> care if they understand. When assumptions are made about the
> behavior of the API code, and the API code changes, as
> occurs with amazing frequency on today's mobile devices,
> there are going to be surprises. I would wager that the
> modern frequency of API changes will result in this behavior
> being very unpopular.

You seem to be advocating for my patch -- instead of the program
"silently" getting killed, now there will be notification. A seccomp
failure is extremely uncommon; much less common that core dumps. This
is why it should always be reported -- it is uncommon and important to
notice.

>> This change makes the audit_seccomp() logic similar to audit_core_dumps()
>> (it does not require an active context). Since core dumps are more
>> common, they sit behind an "audit_enabled" test. Audit reports of seccomp
>> failures should always be visible, and fall back to printk when auditd
>> is not running.
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>>  include/linux/audit.h |    8 +-------
>>  kernel/auditsc.c      |   11 +++++++++--
>>  2 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index ed3ef19..596077f 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -463,7 +463,7 @@ extern void audit_putname(const char *name);
>>  extern void __audit_inode(const char *name, const struct dentry *dentry);
>>  extern void __audit_inode_child(const struct dentry *dentry,
>>                               const struct inode *parent);
>> -extern void __audit_seccomp(unsigned long syscall);
>> +extern void audit_seccomp(unsigned long syscall);
>>  extern void __audit_ptrace(struct task_struct *t);
>>
>>  static inline int audit_dummy_context(void)
>> @@ -508,12 +508,6 @@ static inline void audit_inode_child(const struct dentry *dentry,
>>  }
>>  void audit_core_dumps(long signr);
>>
>> -static inline void audit_seccomp(unsigned long syscall)
>> -{
>> -     if (unlikely(!audit_dummy_context()))
>> -             __audit_seccomp(syscall);
>> -}
>> -
>>  static inline void audit_ptrace(struct task_struct *t)
>>  {
>>       if (unlikely(!audit_dummy_context()))
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index af1de0f..a5caecd 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2693,7 +2693,7 @@ static void audit_log_abend(struct audit_buffer *ab, char *reason, long signr)
>>   * @signr: signal value
>>   *
>>   * If a process ends with a core dump, something fishy is going on and we
>> - * should record the event for investigation.
>> + * should record the event for investigation, if auditing is enabled.
>>   */
>>  void audit_core_dumps(long signr)
>>  {
>> @@ -2710,7 +2710,14 @@ void audit_core_dumps(long signr)
>>       audit_log_end(ab);
>>  }
>>
>> -void __audit_seccomp(unsigned long syscall)
>> +/**
>> + * audit_seccomp - record information about processes that violate seccomp
>> + * @syscall: syscall number that triggered the seccomp violation
>> + *
>> + * If a process violates its own seccomp rules, something has gone very
>> + * wrong, and this event should always be reported for investigation.
>> + */
>> +void audit_seccomp(unsigned long syscall)
>>  {
>>       struct audit_buffer *ab;
>>
>



-- 
Kees Cook
ChromeOS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/