* [PATCH] audit: always report seccomp violations @ 2012-03-23 23:32 Kees Cook 2012-03-25 18:47 ` Casey Schaufler 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2012-03-23 23:32 UTC (permalink / raw) To: linux-kernel; +Cc: Eric Paris, Al Viro 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). 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@chromium.org> --- 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; -- 1.7.0.4 -- Kees Cook Chrome OS Security ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: always report seccomp violations 2012-03-23 23:32 [PATCH] audit: always report seccomp violations Kees Cook @ 2012-03-25 18:47 ` Casey Schaufler 2012-03-26 15:56 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Casey Schaufler @ 2012-03-25 18:47 UTC (permalink / raw) To: Kees Cook; +Cc: linux-kernel, Eric Paris, Al Viro, Casey Schaufler 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 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. > > 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@chromium.org> > --- > 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: always report seccomp violations 2012-03-25 18:47 ` Casey Schaufler @ 2012-03-26 15:56 ` Kees Cook 2012-03-26 16:59 ` Casey Schaufler 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2012-03-26 15:56 UTC (permalink / raw) To: Casey Schaufler; +Cc: linux-kernel, Eric Paris, Al Viro On Sun, Mar 25, 2012 at 11:47 AM, Casey Schaufler <casey@schaufler-ca.com> 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@chromium.org> >> --- >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: always report seccomp violations 2012-03-26 15:56 ` Kees Cook @ 2012-03-26 16:59 ` Casey Schaufler 2012-03-26 17:02 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Casey Schaufler @ 2012-03-26 16:59 UTC (permalink / raw) To: Kees Cook; +Cc: linux-kernel, Eric Paris, Al Viro, Casey Schaufler On 3/26/2012 8:56 AM, Kees Cook wrote: > On Sun, Mar 25, 2012 at 11:47 AM, Casey Schaufler > <casey@schaufler-ca.com> 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@chromium.org> >>> --- >>> 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; >>> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: always report seccomp violations 2012-03-26 16:59 ` Casey Schaufler @ 2012-03-26 17:02 ` Kees Cook 2012-03-29 18:27 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2012-03-26 17:02 UTC (permalink / raw) To: Casey Schaufler; +Cc: linux-kernel, Eric Paris, Al Viro On Mon, Mar 26, 2012 at 9:59 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/26/2012 8:56 AM, Kees Cook wrote: >> On Sun, Mar 25, 2012 at 11:47 AM, Casey Schaufler >> <casey@schaufler-ca.com> 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. Handling logging output is up to the system owner. Already all segfaults are reported to dmesg. Seccomp violation is a much less common case, so I think the benefit outweighs the potential for "clutter". If a system owner wants to ignore klog, they can go ahead and do so. :) -Kees -- Kees Cook ChromeOS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: always report seccomp violations 2012-03-26 17:02 ` Kees Cook @ 2012-03-29 18:27 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2012-03-29 18:27 UTC (permalink / raw) To: Eric Paris; +Cc: linux-kernel, Al Viro, Casey Schaufler On Mon, Mar 26, 2012 at 10:02 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Mar 26, 2012 at 9:59 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 3/26/2012 8:56 AM, Kees Cook wrote: >>> On Sun, Mar 25, 2012 at 11:47 AM, Casey Schaufler >>> <casey@schaufler-ca.com> 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. > > Handling logging output is up to the system owner. Already all > segfaults are reported to dmesg. Seccomp violation is a much less > common case, so I think the benefit outweighs the potential for > "clutter". If a system owner wants to ignore klog, they can go ahead > and do so. :) Eric, do you have thoughts on this? I'd really like to see seccomp failures regardless of audit daemon state. -Kees -- Kees Cook ChromeOS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-29 18:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-23 23:32 [PATCH] audit: always report seccomp violations Kees Cook 2012-03-25 18:47 ` Casey Schaufler 2012-03-26 15:56 ` Kees Cook 2012-03-26 16:59 ` Casey Schaufler 2012-03-26 17:02 ` Kees Cook 2012-03-29 18:27 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.