Re: [PATCH] audit: always report seccomp violations

[Casey Schaufler]

On 3/26/2012 8:56 AM, Kees Cook wrote:
> 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.

Silence is golden. The situation that I am concerned with is one where
a library changes and a program preforms an action that results in a
violation. The application runtime environment notices the applications
demise and restarts it, resulting in a repeat of the violation.

In a classic computer environment you would want the log filled with
notifications so that the user could do something about it. On a
phone, settop box, TV set or seatback entertainment system logging is
evil. No one who has any business seeing a log message has any desire
to see one. It does not matter how important the log message might be.

It's getting harder and harder to have rational error handling at the
OS level as application environments move to higher levels and greater
abstraction. Because seccomp is an OS interface level facility there
are going to be many cases where it fails to align with the intent of
its highly abstracted users. When it does, the programmers are not
going to look at the OS level logs, they are going to look at the API
definitions and such.

In the end I am opposed to any logging that can't be turned off. There
is enough difference in environments and expectations that you can't
say that something should always be reported. I am not saying that I
approve of this situation, but it is clear that most modern application
developers want to hear as little from the OS as possible. Even in
cases where they should be paying attention.

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

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