Re: [PATCH] audit: create explicit AUDIT_SECCOMP event type
On Monday, November 26, 2012 09:45:56 AM Kees Cook wrote:
> On Mon, Nov 26, 2012 at 6:14 AM, Steve Grubb wrote:
> > On Monday, November 19, 2012 01:56:53 PM Kees Cook wrote:
> >> The seccomp path was using AUDIT_ANOM_ABEND from when seccomp mode 1
> >> could only kill a process. While we still want to make sure an audit
> >> record is forced on a kill, this should use a separate record type since
> >> seccomp mode 2 introduces other behaviors. In the case of "handled"
> >> behaviors (process wasn't killed), only emit a record if the process is
> >> under inspection.
> >
> > Under the old record type, we know that the process is being terminated.
> > Therefore the meaning of the action is known as its implicit in the record
> > type. With this new type, we need to record what behavior is being
> > enforced on the process. I don't see where that is being recorded.
>
> The action is encoded in the "code=". If one is doing seccomp
> auditing, this code will be meaningful already.
>
> > Could we add that?
>
> I'd rather not expand the code into the separate meanings if we don't
> have to. It's part of the BPF already, so it's useful to leave it
> as-is, IMO.
Support for this has been added in the user space utilities. This event type
switch actually fixes a problem where the seccomp use of AUDIT_ANOM_ABEND makes
it malformed because it has different fields. This could be pushed into stable
(after testing) in my opinion since it corrects a problem.
ack: Steve Grubb
> >> Cc: Julien Tinnes
> >> Cc: Will Drewry
> >> Signed-off-by: Kees Cook
> >> ---
> >>
> >> include/linux/audit.h |3 ++-
> >> include/uapi/linux/audit.h |1 +
> >> kernel/auditsc.c | 14 +++---
> >> 3 files changed, 14 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/audit.h b/include/linux/audit.h
> >> index bce729a..9d5104d 100644
> >> --- a/include/linux/audit.h
> >> +++ b/include/linux/audit.h
> >> @@ -157,7 +157,8 @@ void audit_core_dumps(long signr);
> >>
> >> static inline void audit_seccomp(unsigned long syscall, long signr, int
> >>
> >> code) {
> >> - if (unlikely(!audit_dummy_context()))
> >> + /* Force a record to be reported if a signal was delivered. */
> >> + if (signr || unlikely(!audit_dummy_context()))
> >>
> >> __audit_seccomp(syscall, signr, code);
> >>
> >> }
> >>
> >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> >> index 76352ac..09a2d94 100644
> >> --- a/include/uapi/linux/audit.h
> >> +++ b/include/uapi/linux/audit.h
> >> @@ -106,6 +106,7 @@
> >>
> >> #define AUDIT_MMAP 1323/* Record showing descriptor and
> >> flags in>
> > mmap */
> >
> >> #define AUDIT_NETFILTER_PKT 1324/* Packets traversing netfilter
> >> chains
> >
> > */
> >
> >> #define AUDIT_NETFILTER_CFG 1325/* Netfilter chain modifications */
> >>
> >> +#define AUDIT_SECCOMP1326/* Secure Computing event
> >> */
> >>
> >> #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> >> #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 2f186ed..157e989 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -2735,7 +2735,7 @@ void __audit_mmap_fd(int fd, int flags)
> >>
> >> context->type = AUDIT_MMAP;
> >>
> >> }
> >>
> >> -static void audit_log_abend(struct audit_buffer *ab, char *reason, long
> >> signr) +static void audit_log_task(struct audit_buffer *ab)
> >>
> >> {
> >>
> >> kuid_t auid, uid;
> >> kgid_t gid;
> >>
> >> @@ -2753,6 +2753,11 @@ static void audit_log_abend(struct audit_buffer
> >> *ab,
> >> char *reason, long signr) audit_log_task_context(ab);
> >>
> >> audit_log_format(ab, " pid=%d comm=", current->pid);
> >> audit_log_untrustedstring(ab, current->comm);
> >>
> >> +}
> >> +
> >> +static void audit_log_abend(struct audit_buffer *ab, char *reason, long
> >> signr) +{
> >> + audit_log_task(ab);
> >>
> >>
Re: [PATCH 7/8] audit: clean up AUDIT_GET/SET local variables and future-proof API
On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
> Re-named confusing local variable names (status_set and status_get didn't
> agree with their command type name) and reduced their scope.
>
> Future-proof API changes by not depending on the exact size of the
> audit_status struct.
I wished things like this were coordinated before being written. We had some
discussion of this back in July under a topic, "audit: implement generic
feature setting and retrieving". Maybe that API can be fixed so its not just
set/unset but can take a number as well.
Also, because there is no way to query the kernel to see what kind of things
it supports, we've typically defined a new message type and put into it exactly
what we need. In other words, if you want something expandable, the define a
new message type like AUDIT_GET_EXT and AUDIT_SET_EXT and build it to be
expandable.
Then in a future version of auditctl it will try to use the new command and
fall back to the old one if it gets EINVAL. Then some years later the old GET
and SET can be deprecated. But the audit code base has to support a wide
variety of kernels and suddenly making on resizable might break old code on
new kernel. A new message type is a safer migration path.
-Steve
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 51 +++
> 1 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index acfa7a9..3d17670 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -635,7 +635,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct
> nlmsghdr *nlh) {
> u32 seq;
> void*data;
> - struct audit_status *status_get, status_set;
> int err;
> struct audit_buffer *ab;
> u16 msg_type = nlh->nlmsg_type;
> @@ -661,47 +660,51 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) data = nlmsg_data(nlh);
>
> switch (msg_type) {
> - case AUDIT_GET:
> - status_set.enabled = audit_enabled;
> - status_set.failure = audit_failure;
> - status_set.pid = audit_pid;
> - status_set.rate_limit= audit_rate_limit;
> - status_set.backlog_limit = audit_backlog_limit;
> - status_set.lost = atomic_read(&audit_lost);
> - status_set.backlog = skb_queue_len(&audit_skb_queue);
> + case AUDIT_GET: {
> + struct audit_status s;
> + s.enabled= audit_enabled;
> + s.failure= audit_failure;
> + s.pid= audit_pid;
> + s.rate_limit = audit_rate_limit;
> + s.backlog_limit = audit_backlog_limit;
> + s.lost = atomic_read(&audit_lost);
> + s.backlog= skb_queue_len(&audit_skb_queue);
> audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
> - &status_set, sizeof(status_set));
> + &s, sizeof(s));
> break;
> - case AUDIT_SET:
> - if (nlh->nlmsg_len < sizeof(struct audit_status))
> - return -EINVAL;
> - status_get = (struct audit_status *)data;
> - if (status_get->mask & AUDIT_STATUS_ENABLED) {
> - err = audit_set_enabled(status_get->enabled);
> + }
> + case AUDIT_SET: {
> + struct audit_status s;
> + memset(&s, 0, sizeof(s));
> + /* guard against past and future API changes */
> + memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
> + if (s.mask & AUDIT_STATUS_ENABLED) {
> + err = audit_set_enabled(s.enabled);
> if (err < 0)
> return err;
> }
> - if (status_get->mask & AUDIT_STATUS_FAILURE) {
> - err = audit_set_failure(status_get->failure);
> + if (s.mask & AUDIT_STATUS_FAILURE) {
> + err = audit_set_failure(s.failure);
> if (err < 0)
> return err;
> }
> - if (status_get->mask & AUDIT_STATUS_PID) {
> - int new_pid = status_get->pid;
> + if (s.mask & AUDIT_STATUS_PID) {
> + int new_pid = s.pid;
>
> if (audit_enabled != AUDIT_OFF)
> audit_log_config_change("audit_pid", new_pid,
> audit_pid,
1);
> audit_pid = new_pid;
> audit_nlk_portid = NETLINK_CB(skb).portid;
> }
> - if (status_get->mask & AUDIT_STATUS_RATE_LIMIT) {
> - err = audit_set_rate_limit(status_get->rate_l
Re: audit looks unmaintained? [was: Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid]
On Tuesday, September 10, 2013 07:20:33 PM Oleg Nesterov wrote:
> On 09/08, Oleg Nesterov wrote:
> > First of all, I do not pretend I understand this code. This was mostly
> > the question, and in fact I mostly asked about audit_bprm() in 0/1.
> >
> > However,
> >
> > On 08/30, Steve Grubb wrote:
> > > On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote:
> > > > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote:
> > > > > Btw. audit looks unmaintained... if you are going to take care of
> > > > > this code, perhaps you can look at
> > > > >
> > > > > http://marc.info/?l=linux-kernel&m=137589907108485
> > > > > http://marc.info/?l=linux-kernel&m=137590271809664
> > >
> > > You don't want to clear the TIF audit flag when context == NULL. What
> > > that will do is make a bunch of inauditable processes. There are times
> > > when audit is disabled and then re-enabled later. If the flag gets
> > > cleared, then a task's syscall will never enter the auditing framework
> > > from kernel/entry_64.S.
> > >
> > > That flag is 0 when auditing has never ever been enabled. If auditing is
> > > enabled, it should always be a 1 unless the task filter has determined
> > > that
> > > this process should not be audited ever. In practice, this is almost
> > > never
> > > used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is
> > > why we have the boot argument. Not setting audit=1 on the boot
> > > arguments means that any process running before the audit daemon
> > > enables auditing can never ever be audited because the only place its
> > > set is when processes are cloned.>
> > Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally?
> >
> > And I do not understand "when context == NULL" above. Say,
> > audit_syscall_entry() does nothing if !audit_context, and nobody except
> > copy_process() does audit_alloc(). So why do we need to trigger the
> > audit's paths if it is NULL?>
> > > Hope this clears up the use. NAK to the patch, it'll break auditing.
> >
> > Not really, but thanks for your reply anyway.
>
> So, Steve, do you still think that patch was wrong? Attached below
> just in case.
I think this looks OK. If the task filter NACK's auditing the process, then
clearing the flag is probably correct. I have design notes from back around the
2.6.7 kernel saying this was the intention.
ACK.
-Steve
> [PATCH 1/1] audit_alloc: clear TIF_SYSCALL_AUDIT if !audit_context
>
> If audit_filter_task() nacks the new thread it makes sense
> to clear TIF_SYSCALL_AUDIT which can be copied from parent
> by dup_task_struct().
>
> A wrong TIF_SYSCALL_AUDIT is not really bad, but it triggers
> the "slow" audit paths in entry.S.
>
> Signed-off-by: Oleg Nesterov
> ---
> kernel/auditsc.c |4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9845cb3..95293ab 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -943,8 +943,10 @@ int audit_alloc(struct task_struct *tsk)
> return 0; /* Return if not auditing. */
>
> state = audit_filter_task(tsk, &key);
> - if (state == AUDIT_DISABLED)
> + if (state == AUDIT_DISABLED) {
> + clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> return 0;
> + }
>
> if (!(context = audit_alloc_context(state))) {
> kfree(key);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: audit looks unmaintained? [was: Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid]
On Sunday, September 08, 2013 05:54:35 PM Oleg Nesterov wrote: > Sorry for delay, vacation. > > First of all, I do not pretend I understand this code. This was mostly > the question, and in fact I mostly asked about audit_bprm() in 0/1. > > However, > > On 08/30, Steve Grubb wrote: > > On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote: > > > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote: > > > > Btw. audit looks unmaintained... if you are going to take care of > > > > this code, perhaps you can look at > > > > > > > > http://marc.info/?l=linux-kernel&m=137589907108485 > > > > http://marc.info/?l=linux-kernel&m=137590271809664 > > > > You don't want to clear the TIF audit flag when context == NULL. What that > > will do is make a bunch of inauditable processes. There are times when > > audit is disabled and then re-enabled later. If the flag gets cleared, > > then a task's syscall will never enter the auditing framework from > > kernel/entry_64.S. > > > > That flag is 0 when auditing has never ever been enabled. If auditing is > > enabled, it should always be a 1 unless the task filter has determined > > that > > this process should not be audited ever. In practice, this is almost never > > used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why > > we have the boot argument. Not setting audit=1 on the boot arguments > > means that any process running before the audit daemon enables auditing > > can never ever be audited because the only place its set is when > > processes are cloned. > > Then why audit_alloc() doesn't set TIF_SYSCALL_AUDIT unconditionally? The code I'm looking at does right at the end of the function. > And I do not understand "when context == NULL" above. Say, > audit_syscall_entry() does nothing if !audit_context, and nobody except > copy_process() does audit_alloc(). So why do we need to trigger the audit's > paths if it is NULL? Because if you enter the audit framework, that means auditing has been turned on at some point in the past, and could be turned back on at some point in the future. If auditing has never been enabled, then you never enter the audit framework at all. If the context is NULL, then audit is not currently enabled. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: audit looks unmaintained? [was: Re: [PATCH 11/12] pid: rewrite task helper functions avoiding task->pid and task->tgid]
On Friday, August 30, 2013 03:06:46 PM Richard Guy Briggs wrote: > On Tue, Aug 27, 2013 at 07:11:34PM +0200, Oleg Nesterov wrote: > > Btw. audit looks unmaintained... if you are going to take care of > > this code, perhaps you can look at > > > > http://marc.info/?l=linux-kernel&m=137589907108485 > > http://marc.info/?l=linux-kernel&m=137590271809664 You don't want to clear the TIF audit flag when context == NULL. What that will do is make a bunch of inauditable processes. There are times when audit is disabled and then re-enabled later. If the flag gets cleared, then a task's syscall will never enter the auditing framework from kernel/entry_64.S. That flag is 0 when auditing has never ever been enabled. If auditing is enabled, it should always be a 1 unless the task filter has determined that this process should not be audited ever. In practice, this is almost never used. But ensuring the TIF_SYSCALL_AUDIT set to 1 on all processes is why we have the boot argument. Not setting audit=1 on the boot arguments means that any process running before the audit daemon enables auditing can never ever be audited because the only place its set is when processes are cloned. Hope this clears up the use. NAK to the patch, it'll break auditing. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data
On Tuesday, April 09, 2013 02:39:32 AM Eric W. Biederman wrote:
> Andrew Morton writes:
> > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs
wrote:
> >> audit rule additions containing "-F auid!=4294967295" were failing with
> >> EINVAL.
> >>
> >> UID_INVALID (and GID_INVALID) is actually a valid uid (gid) for setting
> >> and
> >> testing against audit rules. Remove the check for invalid uid and gid
> >> when
> >> parsing rules and data for logging.
>
> In general testing against invalid uid appears completely bogus, and
> should always return true. As it is and essentially always has been
> incorrect to explicitly set any kernel uid to that value.
This is the unset value that daemons would have. When a real person logs in,
pam_loginuid writes the loginuid that was authenticated to. So, any time the
value is -1, we are dealing with a daemon or system process. When it comes to
auditing, people usually make an exception so that daemon and normal system
activity is not recorded. So, you would make a rule something like
-a always,exit -S open -F exit=-EPERM -F auid>=500 -F auid!=-1
> The only case where this appears to make the least little bit of sense
> is if the goal of the test is to test to see if an audit logloginuid
> has been set at all. In which case depending on a test against
> 4294967295 is bogus because you are depending on an intimate internal
> kernel implementation detail.
Its been this way and documented since at least 9 years ago. The audit system
has been broken for all intents and purposes since the 3.7 kernel was
released.
Thanks,
-Steve
> Certainly removing the gid_valid tests is completely gratitious in this
> case.
>
> >> Revert part of ca57ec0f00c3f139c41bf6b0a5b9bcc95bbb2ad7 (2012-09-11) to
> >> fix
> >> this.
> >
> > Eric, can you please take a look?
> >
> >> Signed-off-by: Richard Guy Briggs
> >> ---
> >>
> >> kernel/auditfilter.c | 12
> >> 1 files changed, 0 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index f9fc54b..457ee39 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -360,10 +360,7 @@ static struct audit_entry
> >> *audit_rule_to_entry(struct audit_rule *rule)>>
> >>/* bit ops not implemented for uid comparisons */
> >>if (f->op == Audit_bitmask || f->op == Audit_bittest)
> >>
> >>goto exit_free;
> >>
> >> -
> >>
> >>f->uid = make_kuid(current_user_ns(), f->val);
> >>
> >> - if (!uid_valid(f->uid))
> >> - goto exit_free;
> >
> > It concerns me that map_id_down() can return -1 on error and that this
> > change causes the kernel to no longer notice that error?
>
> Me too. Where we only communicate with audit in the initial user
> namespace right now it isn't absolutely broken but it certainly isn't a
> habit I want to get into.
>
> How about something like my untested patch below that add an explicit
> operation to test if loginuid has been set?
>
> Eric
>
> From: "Eric W. Biederman"
> Date: Tue, 9 Apr 2013 02:22:10 -0700
> Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
>
> audit rule additions containing "-F auid!=4294967295" were failing
> with EINVAL.
>
> Apparently some userland audit rule sets want to know if loginuid uid
> has been set and are using a test for auid != 4294967295 to determine
> that.
>
> In practice that is a horrible way to ask if a value has been set,
> because it relies on subtle implementation details and will break
> every time the uid implementation in the kernel changes.
>
> So add a clean way to test if the audit loginuid has been set, and
> silently convert the old idiom to the cleaner and more comprehensible
> new idiom.
>
> Reported-By: Richard Guy Briggs wrote:
> Signed-off-by: "Eric W. Biederman"
> ---
> include/linux/audit.h |5 +
> include/uapi/linux/audit.h |1 +
> kernel/auditfilter.c | 29 +
> kernel/auditsc.c |5 -
> 4 files changed, 39 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a9fefe2..8a1ddde 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct *t)
> #define audit_signals 0
> #endif /* CONFIG_AUDITSYSCALL */
>
> +static inline bool audit_loginuid_set(struct task_struct *tsk)
> +{
> + return uid_valid(audit_get_loginuid(tsk));
> +}
> +
> #ifdef CONFIG_AUDIT
> /* These are defined in audit.c */
> /* Public API */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9f096f1..9554a19 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -246,6 +246,7 @@
> #define AUDIT_OBJ_TYPE 21
> #define AUDIT_OBJ_LE
Re: [PATCH] [BZ905179] audit: omit check for uid and gid validity in audit rules and data
On Tuesday, April 16, 2013 03:38:23 PM Richard Guy Briggs wrote:
> On Tue, Apr 09, 2013 at 02:39:32AM -0700, Eric W. Biederman wrote:
> > Andrew Morton writes:
> > > On Wed, 20 Mar 2013 15:18:17 -0400 Richard Guy Briggs
wrote:
> > >> audit rule additions containing "-F auid!=4294967295" were failing with
> > >> EINVAL.>
> > The only case where this appears to make the least little bit of sense
> > is if the goal of the test is to test to see if an audit logloginuid
> > has been set at all. In which case depending on a test against
> > 4294967295 is bogus because you are depending on an intimate internal
> > kernel implementation detail.
> >
> > How about something like my untested patch below that add an explicit
> > operation to test if loginuid has been set?
>
> Sorry for the delay in testing this, I had another urgent bug and a
> belligerent test box...
>
> I like this approach better than mine now that I understand it. I've
> tested the patch below without any changes. It works as expected with
> my previous test case. I don't know if a Signed-off-by: is appropriate
> for me in this case, but I'll throw in a:
>
> Tested-by: Richard Guy Briggs
>
> and recommend a:
>
> Reported-By: Steve Grubb
If this is the approved patch, can it be put in stable? The audit system
hasn't worked as intended since January.
Thanks,
-Steve
> > From: "Eric W. Biederman"
> > Date: Tue, 9 Apr 2013 02:22:10 -0700
> > Subject: [PATCH] audit: Make testing for a valid loginuid explicit.
> >
> > audit rule additions containing "-F auid!=4294967295" were failing
> > with EINVAL.
> >
> > Apparently some userland audit rule sets want to know if loginuid uid
> > has been set and are using a test for auid != 4294967295 to determine
> > that.
> >
> > In practice that is a horrible way to ask if a value has been set,
> > because it relies on subtle implementation details and will break
> > every time the uid implementation in the kernel changes.
> >
> > So add a clean way to test if the audit loginuid has been set, and
> > silently convert the old idiom to the cleaner and more comprehensible
> > new idiom.
> >
> > Reported-By: Richard Guy Briggs wrote:
> > Signed-off-by: "Eric W. Biederman"
> > ---
> >
> > include/linux/audit.h |5 +
> > include/uapi/linux/audit.h |1 +
> > kernel/auditfilter.c | 29 +
> > kernel/auditsc.c |5 -
> > 4 files changed, 39 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index a9fefe2..8a1ddde 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -390,6 +390,11 @@ static inline void audit_ptrace(struct task_struct
> > *t)
> >
> > #define audit_signals 0
> > #endif /* CONFIG_AUDITSYSCALL */
> >
> > +static inline bool audit_loginuid_set(struct task_struct *tsk)
> > +{
> > + return uid_valid(audit_get_loginuid(tsk));
> > +}
> > +
> >
> > #ifdef CONFIG_AUDIT
> > /* These are defined in audit.c */
> >
> > /* Public API */
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 9f096f1..9554a19 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -246,6 +246,7 @@
> >
> > #define AUDIT_OBJ_TYPE 21
> > #define AUDIT_OBJ_LEV_LOW 22
> > #define AUDIT_OBJ_LEV_HIGH 23
> >
> > +#define AUDIT_LOGINUID_SET 24
> >
> > /* These are ONLY useful when checking
> >
> > * at syscall exit time (AUDIT_AT_EXIT). */
> >
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 540f986..6381d17 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -349,6 +349,12 @@ static struct audit_entry *audit_rule_to_entry(struct
> > audit_rule *rule)>
> > if (f->op == Audit_bad)
> >
> > goto exit_free;
> >
> > + /* Support legacy tests for a valid loginuid */
> > + if ((f->type == AUDIT_LOGINUID) && (f->val == 4294967295)) {
> > + f->type = AUDIT_LOGINUID_SET;
> > + f->val = 0;
> > + }
> > +
> >
Re: [PATCH] audit: create explicit AUDIT_SECCOMP event type
On Monday, November 19, 2012 01:56:53 PM Kees Cook wrote:
> The seccomp path was using AUDIT_ANOM_ABEND from when seccomp mode 1
> could only kill a process. While we still want to make sure an audit
> record is forced on a kill, this should use a separate record type since
> seccomp mode 2 introduces other behaviors. In the case of "handled"
> behaviors (process wasn't killed), only emit a record if the process is
> under inspection.
Under the old record type, we know that the process is being terminated.
Therefore the meaning of the action is known as its implicit in the record
type. With this new type, we need to record what behavior is being enforced on
the process. I don't see where that is being recorded.
Could we add that?
Thanks,
-Steve
> Cc: Julien Tinnes
> Cc: Will Drewry
> Signed-off-by: Kees Cook
> ---
> include/linux/audit.h |3 ++-
> include/uapi/linux/audit.h |1 +
> kernel/auditsc.c | 14 +++---
> 3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index bce729a..9d5104d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -157,7 +157,8 @@ void audit_core_dumps(long signr);
>
> static inline void audit_seccomp(unsigned long syscall, long signr, int
> code) {
> - if (unlikely(!audit_dummy_context()))
> + /* Force a record to be reported if a signal was delivered. */
> + if (signr || unlikely(!audit_dummy_context()))
> __audit_seccomp(syscall, signr, code);
> }
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 76352ac..09a2d94 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -106,6 +106,7 @@
> #define AUDIT_MMAP 1323/* Record showing descriptor and flags
> in
mmap */
> #define AUDIT_NETFILTER_PKT 1324/* Packets traversing netfilter chains
*/
> #define AUDIT_NETFILTER_CFG 1325/* Netfilter chain modifications */
> +#define AUDIT_SECCOMP1326/* Secure Computing event */
>
> #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2f186ed..157e989 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2735,7 +2735,7 @@ void __audit_mmap_fd(int fd, int flags)
> context->type = AUDIT_MMAP;
> }
>
> -static void audit_log_abend(struct audit_buffer *ab, char *reason, long
> signr) +static void audit_log_task(struct audit_buffer *ab)
> {
> kuid_t auid, uid;
> kgid_t gid;
> @@ -2753,6 +2753,11 @@ static void audit_log_abend(struct audit_buffer *ab,
> char *reason, long signr) audit_log_task_context(ab);
> audit_log_format(ab, " pid=%d comm=", current->pid);
> audit_log_untrustedstring(ab, current->comm);
> +}
> +
> +static void audit_log_abend(struct audit_buffer *ab, char *reason, long
> signr) +{
> + audit_log_task(ab);
> audit_log_format(ab, " reason=");
> audit_log_string(ab, reason);
> audit_log_format(ab, " sig=%ld", signr);
> @@ -2783,8 +2788,11 @@ void __audit_seccomp(unsigned long syscall, long
> signr, int code) {
> struct audit_buffer *ab;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> - audit_log_abend(ab, "seccomp", signr);
> + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> + if (unlikely(!ab))
> + return;
> + audit_log_task(ab);
> + audit_log_format(ab, " sig=%ld", signr);
> audit_log_format(ab, " syscall=%ld", syscall);
> audit_log_format(ab, " compat=%d", is_compat_task());
> audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: asm/unistd.h
It would seem to me that after hearing how the macros are used in practice, wouldn't turning them into inline functions be an improvement? This is something gcc supports, it accomplishes the same thing, and has the added advantage of type checking. http://gcc.gnu.org/onlinedocs/gcc-2.95.3/gcc_4.html#SEC92 Or perhaps type checking macro arguments would be another fertile area for the Stanford Checker... Cheers, Steve Grubb - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] minor do_syslog cleanup
Hello,
This patch removes extra setting of the error value in the do_syslog
function. The patch is against 2.2.16, but printk.c seems to have changed
little so it probably applies against other kernels.
See Ya,
Steve Grubb
--- printk.orig Thu Nov 30 07:58:58 2000
+++ printk.cThu Nov 30 08:55:07 2000
@@ -123,19 +123,18 @@
unsigned long i, j, limit, count;
int do_clear = 0;
char c;
- int error = -EPERM;
+ int error = 0;
- error = 0;
switch (type) {
case 0: /* Close log */
break;
case 1: /* Open log */
break;
case 2: /* Read from log */
- error = -EINVAL;
- if (!buf || len < 0)
+ if (!buf || len < 0) {
+ error = -EINVAL;
goto out;
- error = 0;
+ }
if (!len)
goto out;
error = verify_area(VERIFY_WRITE,buf,len);
@@ -163,10 +162,10 @@
do_clear = 1;
/* FALL THRU */
case 3: /* Read last kernel messages */
- error = -EINVAL;
- if (!buf || len < 0)
+ if (!buf || len < 0) {
+ error = -EINVAL;
goto out;
- error = 0;
+ }
if (!len)
goto out;
error = verify_area(VERIFY_WRITE,buf,len);
@@ -224,15 +223,15 @@
spin_unlock_irq(&console_lock);
break;
case 8:
- error = -EINVAL;
- if (len < 1 || len > 8)
+ if (len < 1 || len > 8) {
+ error = -EINVAL;
goto out;
+ }
if (len < minimum_console_loglevel)
len = minimum_console_loglevel;
spin_lock_irq(&console_lock);
console_loglevel = len;
spin_unlock_irq(&console_lock);
- error = 0;
break;
default:
error = -EINVAL;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
[Patch] performance enhancement for simple_strtoul
Hello,
The following patch is a faster implementation of the simple_strtoul
function. This function differs from the original in that it reduces the
multiplies to shifts and logical operations wherever possible. My testing
shows that it adds around 100 bytes, but is about 6% faster on a K6-2. (It
is 40% faster that glibc's strtoul, but that's a different story.) My guess
is that the performance gain will be higher on platforms with slower
multiplication instructions. I have tested it for numerical accuracy so I
think this is safe to apply. If anyone is interested, I can also supply a
test application that demonstrates the performance gain. This patch was
generated against 2.2.16, but should apply to 2.2.19 cleanly. In
2.4.0-test9, simple_strtoul starts on line 19 rather than 17, hopefully
that's not a problem.
Cheers,
Steve Grubb
-
--- lib/vsprintf.orig Fri Dec 1 08:58:02 2000
+++ lib/vsprintf.c Wed Dec 20 08:42:26 2000
@@ -14,10 +14,13 @@
#include
#include
+/*
+* This function converts base 8, 10, or 16 only - Steve Grubb
+*/
unsigned long simple_strtoul(const char *cp,char **endp,unsigned int base)
{
- unsigned long result = 0,value;
-
+ unsigned char c;
+ unsigned long result = 0;
if (!base) {
base = 10;
if (*cp == '0') {
@@ -29,11 +32,36 @@
}
}
}
- while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
- ? toupper(*cp) : *cp)-'A'+10) < base) {
- result = result*base + value;
- cp++;
- }
+ c = *cp;
+switch (base) {
+case 10:
+while (isdigit(c)) {
+result = (result*10) + (c & 0x0f);
+c = *(++cp);
+}
+break;
+case 16:
+while (isxdigit(c)) {
+result = result<<4;
+if (c&0x40)
+ result += (c & 0x07) + 9;
+else
+result += c & 0x0f;
+c = *(++cp);
+}
+break;
+case 8:
+while (isdigit(c)) {
+if ((c&0x37) == c)
+result = (result<<3) + (c & 0x07);
+else
+break;
+c = *(++cp);
+}
+break;
+default: /* Is anything else used by the kernel? */
+break;
+}
if (endp)
*endp = (char *)cp;
return result;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
[Test Case] performance enhancement for simple_strtoul
Hello,
Here's the test case for the suggested simple_strtoul function. I just
finished testing on a P3 where it seems to show a 16-20% speed improvement
over the current algorithm.
compile it as:
gcc /usr/src/linux/lib/ctype.c strtoul_test.c -o strtoul_test
You can change the numeric base value with this define to 8, 10, or 16 to
see the speed change for each numeric representation:
#define BASE 10
Have fun,
Steve Grubb
--strtoul_test.c--
#include
#include
#include
#include
#include
#include
struct timeval last_stopwatch_time;
void stopwatch()
{
struct timeval now;
int delta;
gettimeofday(&now, 0);
delta = (now.tv_sec - last_stopwatch_time.tv_sec) * 100 +
(now.tv_usec - last_stopwatch_time.tv_usec);
printf ("Stopwatch: elapsed time %d.%03d seconds\n\n",
delta / 100, (delta / 1000) % 1000);
last_stopwatch_time = now;
}
unsigned long old_simple_strtoul(const char *cp,char **endp,unsigned int
base)
{
unsigned long result = 0,value;
if (!base) {
base = 10;
if (*cp == '0') {
base = 8;
cp++;
if ((*cp == 'x') && isxdigit(cp[1])) {
cp++;
base = 16;
}
}
}
while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
? toupper(*cp) : *cp)-'A'+10) < base) {
result = result*base + value;
cp++;
}
if (endp)
*endp = (char *)cp;
return result;
}
unsigned long new_simple_strtoul2(const char *cp,char **endp,unsigned int
base)
{
unsigned char c;
unsigned long result = 0;
if (!base) {
base = 10;
if (*cp == '0') {
base = 8;
cp++;
if ((*cp == 'x') && isxdigit(cp[1])) {
cp++;
base = 16;
}
}
}
c = *cp;
switch (base) {
case 10:
while (isdigit(c)) {
result = (result*10) + (c & 0x0f);
c = *(++cp);
}
break;
case 16:
while (isxdigit(c)) {
result = (result<<4);
if (c&0x40)
result += (c & 0x07) + 9;
else
result += (c & 0x0f);
c = *(++cp);
}
break;
case 8:
while (isdigit(c)) {
if ((c&0x37) == c)
result = (result<<3) + (c & 0x07);
else
break;
c = *(++cp);
}
}
if (endp)
*endp = (char *)cp;
return result;
}
#define NUMBER_TO_TEST 32768
#define BASE 10
char f[3][3] = { "%d", "%X", "%o"};
char str[NUMBER_TO_TEST][32];
unsigned long r[NUMBER_TO_TEST];
int main()
{
int rn, i, j, iterations = 1000;
time_t tm;
time(&tm);
srand((unsigned) tm);
rn = rand();
// do setup here
for (i=0; ihttp://www.tux.org/lkml/
Re: [Patch] performance enhancement for simple_strtoul
Hello, I thought about that. This would be my recommendation for glibc where the general public may be doing scientific applications. But this is the kernel and there are people that would reject my patch purely on the basis that it adds precious bytes to the kernel. But since the kernel is "controllable" & printf() and its variants only support 8, 10, & 16, perhaps a better solution might be to trap the odd case and write something for it if its that important, or simply don't allow it. The base guessing part at the beginning of the function only supports base 8, 10, & 16. Therefore, the only way to require another base is to specify it in the function call (param - unsigned int base). A quick scan of the current linux source shows no one using something odd. So... If the maintainers of vsprintf.c want support for all number bases, that's fine with me. Just say the word & I'll gen up another patch...but it will be more bytes. Cheers, Steve Grubb - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch] performance enhancement for simple_strtoul
Hello,
I continued experimenting with the Test Case and found a further speed
improvement & I am re-submiting the patch. It is the same as the first one
with the two local variables changed to register storage types.
On a K6-2, I now see:
Base 10 - 28% speedup
Base 16 - 24% speedup
Base 8 - 30% speedup
On a P3 system, I now see:
Base 10 - 25% speedup
Base 16 - 17% speedup
Base 8 - 20% speedup
It seems gcc creates much better code with the variables set to register
types. Please apply the following patch. It should apply to any recent 2.2.x
without problems. In 2.4 the function starts 2 lines later.
Cheers,
Steve Grubb
--
--- lib/vsprintf.orig Fri Dec 1 08:58:02 2000
+++ lib/vsprintf.c Wed Dec 20 13:14:13 2000
@@ -14,10 +14,13 @@
#include
#include
+/*
+* This function converts base 8, 10, or 16 only - Steve Grubb
+*/
unsigned long simple_strtoul(const char *cp,char **endp,unsigned int base)
{
- unsigned long result = 0,value;
-
+ register unsigned char c;
+ register unsigned long result = 0;
if (!base) {
base = 10;
if (*cp == '0') {
@@ -29,11 +32,36 @@
}
}
}
- while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
- ? toupper(*cp) : *cp)-'A'+10) < base) {
- result = result*base + value;
- cp++;
- }
+ c = *cp;
+switch (base) {
+case 10:
+while (isdigit(c)) {
+result = (result*10) + (c & 0x0f);
+c = *(++cp);
+}
+break;
+case 16:
+while (isxdigit(c)) {
+result = result<<4;
+if (c&0x40)
+ result += (c & 0x07) + 9;
+else
+result += c & 0x0f;
+c = *(++cp);
+}
+break;
+case 8:
+while (isdigit(c)) {
+if ((c&0x37) == c)
+result = (result<<3) + (c & 0x07);
+else
+break;
+c = *(++cp);
+}
+break;
+default: /* Is anything else used by the kernel? */
+break;
+}
if (endp)
*endp = (char *)cp;
return result;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: file system auditing based on location and name
On Wednesday 06 July 2005 19:50, Greg KH wrote: > As inotify works off of open file descriptors, yes, this is true. But, > again, if you think this is really important, then why not just work > with inotify to provide that kind of support to it? http://marc.theaimsgroup.com/?l=linux-kernel&m=110265021327578&w=2 I think Tim was told not to dig into inotify. A lot of effort has been put into testing the code Tim has presented with review from several kernel developers (listed in the cc). They too should step up and give their opinion on this. I want to believe questions were asked about this last December when we were starting into this effort. I think the conclusion from the inotify people was for us to proceed and then when we know what we really want, we can refactor should anything be in common. > I suggest you work together with the inotify developers to hash out your > differences, as it sounds like you are duplicating a lot of the same > functionality. Maybe yes and no. Now that the fs audit code is out, I think we can spot commonality. The only common piece that I can think of is just the hook. The whole rest of it is different. I hope the inotify people comment on this to see if there is indeed something that should be refactored. > Do you have any documetation or example userspace code that shows how to > use this auditfs interface you have created? people.redhat.com/sgrubb/audit The audit package is currently distributed in Fedora Core 4. The code to use Tim's fs audit code is in the user space app, but is waiting for the kernel pieces. There is a man page for auditctl that shows all the options. (fs specific options are -wWpk ) To watch /etc/shadow, you would issue: auditctl -w /etc/shadow -p wa this will generate events for any update to the file including changes to ownership or permissions. We are interested in attribute changes as well. If you wanted to watch a file in a chroot directory, you could do this: auditctl -w /var/chroot/etc/shadow -p wa -k /var/chroot The audit events would indicate the path from the perspective of the app generating the events, but since we added the /var/chroot key, we can see that it really came from the chroot dir. Hope this helps... -Steve Grubb - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: file system auditing based on location and name
On Thursday 07 July 2005 14:15, Greg KH wrote: > I fail to see any refactoring here, why not make your patch rely on > theirs? At the time this code was developed, inotify was not in the kernel. We would be patching against another patch that's not in the kernel. > > The whole rest of it is different. I hope the inotify people comment > > on this to see if there is indeed something that should be refactored. > > I realize your userspace access is different, yet I do not believe yet > that it should be this way. The problems that we are faced with are dictated by CAPP and other security profiles. The audit user space access has been in the kernel for over a year, so that's not really a good thing to go changing. > No documentation on the auditfs interface :( That's what Tim's email was providing. Its a new component. > > The audit package is currently distributed in Fedora Core 4. The code to > > use Tim's fs audit code is in the user space app, but is waiting for the > > kernel pieces. > > So the userspace package in FC4 will not use auditfs? Right. You get a few warnings due to missing functionality. If the kernel were patched with Tim's code, it all works as expected. We have worked out the user space access and that shouldn't be changing. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: file system auditing based on location and name
On Thursday 07 July 2005 15:04, Greg KH wrote: > You are adding auditfs, a new userspace access, right? Not sure what you mean. This is using the same netlink interface that all the rest of the audit system is using for command and control. Nothing has changed here. What is different is the message I send into the kernel. The audit system dispatches it into Tim's code which in turn sets up the watch. The notification that the event has occurred uses the same interface that all other audit events use. Its just a different message type. It is filtered by some attributes and if it is still of interest, the event is placed in the queue so it is serialized with all other events. > His email provided no documentation that I could see. Am I just missing > something? The auditfs code is programmed by filling out the watch_transport structure and sending a AUDIT_WATCH_INS message type. The perms, pathlen, & keylen are all that's filled out. The path & key are stored back to back in the payload section. To delete, you do the same thing and send AUDIT_WATCH_REM message. Yes, this should be added to the documentation. > > > So the userspace package in FC4 will not use auditfs? > > > > Right. You get a few warnings due to missing functionality. If the kernel > > were patched with Tim's code, it all works as expected. We have worked > > out the user space access and that shouldn't be changing. > > Then what use is auditfs for if you don't need it? But we do. You cannot audit files who's inode may change using the current syscall techniques. I put the code into FC4 hoping that one day we will have a kernel that supports it. The current audit system code lets you audit by syscall, which means you can audit open or write, but then you get it for all processes that open or write. Or you could limit it with a pid, but that's impossible since you would have to predict the future pid when putting the audit rule in. Tim's code lets you say I want change notification to this file only. The notification follows the audit format with all relavant pieces of information gathered at the time of the event and serialized with all other events. > Am I correct in thinking that you all need to split this patch into two > pieces, the new inode stuff, and auditfs, as neither one has anything to > do with the other? It could be split to make it easier to read, but one's useless without the other. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Syscall auditing - move "name=" field to the end
On Thursday 17 March 2005 12:57, Chris Wright wrote: > Steve, are you working on processing log data, do you have a preference? Yes, I am working on a utility to process the data. I have 4 comments: 1) Fields that magically appear and dissappear are problematic for fast parsing. 2) There should be a way to control what fields the kernel emits. The dissappearing fields are what I take to be a stab at message compression. By having a mask driven approach and always emitting those fields, we can parse faster and have compression. 3) Fields that potentially have a space, tab, or carriage return in them need escaping or quoting if they are sent in human readable format. 4) There should be a mode/format status variable so that in the future we can tell the kernel to switch to another (binary) format. This way human readable records can go to syslog and special apps like the audit daemon can switch to another format (binary data ?) which might be more efficient. I haven't spent anytime looking at what makes sense for a binary format, nor do we have time for that right now. But I'd like to look at that in the future. -Steve Grubb - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] SELinux AVC audit log ipaddr field support (for task_struct->curr_ip)
On Thursday 10 March 2005 10:16, Stephen Smalley wrote: > (e.g. the exe= information currently generated by avc_audit could be done > by audit_log_exit instead). I already have a patch that does that. It tells you what program is being run instead of /bin/bash. I'll send it in a day or two for everyone to look over. -Steve Grubb - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Friday 26 October 2007 04:42:28 pm Tony Jones wrote: > Thread flag TIF_SYSCALL_AUDIT is not cleared for new children when audit > context creation has been disabled (auditctl -e0). This can cause new > children forked from a parent created when audit was enabled to not take > the fastest syscall path thru entry.S This came up almost 2 years ago: https://www.redhat.com/archives/linux-audit/2005-September/msg00048.html The problem is that removing that flag makes the children unauditable in the future. The only place that flag gets set is during fork. Unless I'm missing something, to make all children auditable again would mean stopping all processes and or'ing that flag into all thread info areas. I do not want to propose that patch to LKML. :) -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 01:20:58 pm Tony Jones wrote: > > The problem is that removing that flag makes the children unauditable in > > the future. The only place that flag gets set is during fork. > > I don't see this. If the child does not have the TIF_SYSCALL_AUDIT flag, it never goes into audit_syscall_entry. It becomes unauditable. > The case that would be undesirable would be for a task to have an audit > context but to not have the thread flag enabled. That would just be a small allocation of memory that will be returned when the process exits. From an auditing PoV, something that is undesirable is the inability to audit a process that you want to audit. > > Unless I'm missing something, to make all children auditable again would > > mean stopping all processes and or'ing that flag into all thread info > > areas. > > I think you are. Or maybe the code was different two years ago so that the > above made sense. > > In the above scenario, audit is disabled, a new child is forked, we bail > early so there is no audit context (and now there is no flag in the thread > area). Currently there is no way this task is ever going to be audited as > there is no audit context. So when audit is re-enabled, how do you make that task auditable? > If this task forks a new child, at this point the value of audit enabled > will determine if there should be a context allocated and it will allocate > the TIF flag also. In the new child, but not the parent. > I don't see your stopping all processes scenario. That is so you can walk the process table and "or" the bit in unconditionally. All processes need to be auditable or you've got a security hole. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Monday 29 October 2007 07:15:30 pm Tony Jones wrote: > On Mon, Oct 29, 2007 at 06:04:31PM -0400, Steve Grubb wrote: > > So when audit is re-enabled, how do you make that task auditable? > > No idea. How do you do it currently? HINT: current->audit_context == NULL > for these tasks. If !audit_enabled, then audit_alloc() is not going to > allocate an audit_context for the task. We are looking into this - at one time it did. Someone should follow up with a path correcting this soon. But I doubt the audit system will work correctly if the flag gets removed as there is no good way to add it again later. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: clear thread flag for new children
On Thursday 01 November 2007 01:23:24 pm Tony Jones wrote:
> > We are looking into this - at one time it did. Someone should follow up
> > with a path correcting this soon. But I doubt the audit system will work
> > correctly if the flag gets removed as there is no good way to add it
> > again later.
>
> So on the syscall path you're now going to something like (pseudocode):
> if (unlikely(current->audit_context)) {audit_syscall_entry()}
> else if (audit_enabled) {audit_alloc(); audit_syscall_entry()}
Something like that. The TIF flag is to pick off processes that we are
interested in auditing, for performance reasons if audit is disabled the
context is not created. But if auditing is re-enabled, processes can
be "repaired" so that the are auditable again by allocating the context on
the fly the first time we see a process. Without the flag, they never get
steered into the audit system where this can happen.
> I agree, if this is what you want to do, then clearing the thread flag
> would be a bad idea.
If any other performance improvements jumps out at you, please bring them up.
> I'd assumed the current behaviour was by design as allocating contexts at
> syscall time doesn't seem a great idea but if you need the functionality,
> you need the functionality.
Yep
-Steve
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [2.6 patch] kernel/audit.c: change the exports to EXPORT_SYMBOL_GPL
On Sunday 29 July 2007 11:02:33 Adrian Bunk wrote: > They are still completely unused, but hopefully some of the theoretical > code that might use it will appear in the kernel in the near future... > > Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]> > Acked-by: Steve Grubb <[EMAIL PROTECTED]> I am reluctant to say that I ack this patch for a couple reasons: 1) We are talking about a basic logging facility that should be open like printk() is. 2) There are no user space GPL restrictions to use the audit netlink API, so why restrict who can send audit events via the in-kernel interfaces? It just doesn't make sense to have 2 different licenses for in-kernel vs user space audit event recording. Its the same subsystem differing only by where the event originated. 3) The API has been unrestricted for years. I don't think its a good idea to take a basic logging API away from people that have programmed to it. 4) In the absence of the in-kernel audit logging api, people will either create parallel infrastructure or resort to using printk. It will be difficult for end users to correlate security events from 2 different logs. I would support there being a mechanism for anyone who wants to reduce the number of exported symbols for their own kernels - I believe that is the basic problem here. But I think there are enough reasons to continue keeping this API open and unrestricted for anyone that wants it that way. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] audit support for SH
On Wednesday 07 November 2007 12:04:46 am Yuichi Nakamura wrote: > I found syscall audit does not work on SH(SuperH). > I made patch to support syscall audit for SH. I think this is close, but it looks like you missed the syscall classification piece. You can find an example here: arch/x86_64/kernel/audit.c Its used for determining which syscalls we are interested in for watches. Also, IBM and HP both have released audit test suites. You should run the CAPP tests at a minimum to see if you have hooked everything that is expected. If you have SE Linux enabled for that platform, you may want to try the LSPP tests but you would need have the MLS policy installed. IBM's announcement is here: https://www.redhat.com/archives/redhat-lspp/2007-August/msg2.html and HP's here: https://www.redhat.com/archives/linux-audit/2007-August/msg00030.html And...user space would need an update for the syscall table and arches so that you can run the tests. Please send that patch to linux-audit mail list. Thanks, -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Audit: Add TTY input auditing
On Thursday 07 June 2007 04:13:42 Jan Engelhardt wrote: > >Add TTY input auditing, used to audit system administrator's actions. > > _What_ exactly does it audit? In theory, it should audit the actions performed by the sysadmin. This patch doesn't cover actions done via X windows interface. > And why does it only audit sysadmin actions? This is what's called out for in various security standards such as NISPOM, DCID 6/3, and PCI. Its all about non-repudiation. > Is this supposed to be a keylogger? Sort of. Its supposed to allow the Security Officer the ability to check on what an admin was doing if they were found to be attempting access to information outside their duties. Or if something is found to be misconfigured, they can backtrack and see how it became that way if that was important. Some people try to be compliant with these security standards with user space tools like rootsh, but that is too easy to detect and defeat. And then it does not put its data into the audit system where its correlated with other system events. >What about tty output? That is not required. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Audit: Add TTY input auditing
On Thursday 07 June 2007 11:42, Casey Schaufler wrote: > > tools like rootsh, but that is too easy to detect and defeat. And then it > > does not put its data into the audit system where its correlated with > > other system events. > > The evaluation teams that I have worked with (OrangeBook and CC) > as well as their technical advisory groups have always been clear > that keyboard logging is not an appropriate mechanism for security > audit logging. There is insufficient correlation between what is > typed and security relevent actions for keyboard (or mouse event) > logging to meet the audit requirements. Ok, this is a sample set of requirements we are trying to meet: Implement automated audit trails for all system components to reconstruct the following events: All actions taken by any individual with root or administrative privileges If we do not get commands typed at a prompt, we have to audit by execve. Auditing execve for uid = 0 produces millions of events since that includes daemons. If we get clever and restrict auditing to events for root uid and auid >= 500, we still wind up with millions of events because of shell scripts. People in control of some of these security targets said to me that auditing by execve cannot be the solution, because the audit trail becomes too big, unwieldy, full of irrelavent events, and not easily analysed. So, that approach does not work for people either. Casey, so what approach would you take to meet the requirement? > You have to log what happened. Which can be done by auditing for execution of specific apps or watching access to certain files. So, I don't see tty auditing as something that replaces other auditing, it allows us to form a better picture of what happened to the system. > Logging what was requested is insufficient and logging what was > typed, which may or may not have resulted in an actual request is > not helpful to meeting security audit requirements. I would disagree. Its helpful to complete the picture of what's happening on the system. -Steve - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC(v2): Audit Kernel Container IDs
On Monday, October 16, 2017 8:33:40 PM EDT Richard Guy Briggs wrote: > On 2017-10-12 16:33, Casey Schaufler wrote: > > On 10/12/2017 7:14 AM, Richard Guy Briggs wrote: > > > Containers are a userspace concept. The kernel knows nothing of them. > > > > > > The Linux audit system needs a way to be able to track the container > > > provenance of events and actions. Audit needs the kernel's help to do > > > this. > > > > > > Since the concept of a container is entirely a userspace concept, a > > > registration from the userspace container orchestration system initiates > > > this. This will define a point in time and a set of resources > > > associated with a particular container with an audit container ID. > > > > > > The registration is a pseudo filesystem (proc, since PID tree already > > > exists) write of a u8[16] UUID representing the container ID to a file > > > representing a process that will become the first process in a new > > > container. This write might place restrictions on mount namespaces > > > required to define a container, or at least careful checking of > > > namespaces in the kernel to verify permissions of the orchestrator so it > > > can't change its own container ID. A bind mount of nsfs may be > > > necessary in the container orchestrator's mntNS. > > > Note: Use a 128-bit scalar rather than a string to make compares faster > > > and simpler. > > > > > > Require a new CAP_CONTAINER_ADMIN to be able to carry out the > > > registration. > > > > Hang on. If containers are a user space concept, how can > > you want CAP_CONTAINER_ANYTHING? If there's not such thing as > > a container, how can you be asking for a capability to manage > > them? > > There is such a thing, but the kernel doesn't know about it yet. This > same situation exists for loginuid and sessionid which are userspace > concepts that the kernel tracks for the convenience of userspace. As > for its name, I'm not particularly picky, so if you don't like > CAP_CONTAINER_* then I'm fine with CAP_AUDIT_CONTAINERID. It really > needs to be distinct from CAP_AUDIT_WRITE and CAP_AUDIT_CONTROL since we > don't want to give the ability to set a containerID to any process that > is able to do audit logging (such as vsftpd) and similarly we don't want > to give the orchestrator the ability to control the setup of the audit > daemon. A long time ago, we were debating what should guard against rouge processes from setting the loginuid. Casey argued that the ability to set the loginuid means they have the ability to control the audit trail. That means that it should be guarded by CAP_AUDIT_CONTROL. I think the same logic applies today. The ability to arbitrarily set a container ID means the process has the ability to indirectly control the audit trail. -Steve
Re: RFC(v2): Audit Kernel Container IDs
On Tuesday, October 17, 2017 12:43:18 PM EDT Casey Schaufler wrote: > > The idea is that processes spawned into a container would be labelled > > by the container orchestration system. It's unclear what should happen > > to processes using nsenter after the fact, but policy for that should > > be up to the orchestration system. > > I'm fine with that. The user space policy can be anything y'all like. I think there should be a login event. > > The label will be used as a tag for audit information. > > Deep breath ... > > Which *is* a kernel security policy mechanism. Since the "label" > is part of the audit information that the kernel is guaranteeing > changing it would be covered by CAP_AUDIT_CONTROL. If the kernel > does not use the "label" for any other purpose this is the only > capability that makes sense for it. I agree. The ability to set the container label grants the ability to evade rules or modify audit rules. CAP_AUDIT_CONTROL makes sense to me. > > I think you were missing label inheritance above. > > > > The security implications are that anything that can change the label > > could also hide itself and its doings from the audit system and thus > > would be used as a means to evade detection. Yes. We have the same problem with loginuid. There are restrictions on who can change it once set. And then we made an immutable flag so that people that want a hard guarantee can get that. -Steve
Re: RFC(v2): Audit Kernel Container IDs
On Tuesday, October 17, 2017 1:57:43 PM EDT James Bottomley wrote: > > > > The idea is that processes spawned into a container would be > > > > labelled by the container orchestration system. It's unclear > > > > what should happen to processes using nsenter after the fact, but > > > > policy for that should be up to the orchestration system. > > > > > > I'm fine with that. The user space policy can be anything y'all > > > like. > > > > I think there should be a login event. > > I thought you wanted this for containers? Container creation doesn't > have login events. In an unprivileged orchestration system it may be > hard to synthetically manufacture them. I realize this. This work is very similar to problems we've solved 12 years ago. We'll figure out what the right name is for it down the road. But the concept is the same. If something enters a container, we need to know about it. It needs to get tagged and be associated with the container. The way this was solved for the loginuid problem was to add a session identifier so that new logins of the same loginuid can coexist and we can trace actions back to a specific login. I'd think we can apply lessons learned from a while back to make container identification act similarly. -Steve
Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote: > > >> > It might be simplest to just apply a corrective patch over top of > > >> > this one so that you don't have to muck about with git branches and > > >> > commit messages. > > >> > > >> A quick note on the "corrective patch": given we are just days away > > >> from the merge window opening, it is *way* to late for something like > > >> that, at this point the only options are to leave it as-is or > > >> yank/revert and make another pass during the next development phase. > > > > > > Then yank it. I think that is overreacting but given the options you > > > presented its the only one that avoids changing a critical field > > > format. > > > > It's not overreacting Steve, there is simply no way we can test and > > adequately soak new changes in the few days we have left. Its just moving the output of the information a few lines down further in the code. 10 minutes of work, tops. > > Event yanks/reverts carry a risk at this stage, but I consider that the > > less risky option for these patches. Neither is a great option, and that > > is why I'm rather annoyed. > > I don't really see that this is my choice to include it or not. This is > the upstream maintainer's decision. > > I can't say I'd be thrilled to have my name on something that stuffs up > the system though. It still isn't clear to me why an incomplete path > from some seemingly random place in the filesystem tree is preferable to > something that gives it an anchor point, at least to human interpreters. The path should stay. Just the file system type needs decoupling and moving. > Adding an fstype to the record is an interesting idea, but then creates > a void for all the rest of the properly formed records that don't need > it and will need more work to find it, wasting bandwidth with > "fstype=?". I would let it optionally swing in and out at the end of the record. This should never show up on a normal system because the rules will suppress generating this information by default. So, it really won't be all that visible. > How are the analysis tools stymied by a text prefix to a path that it can't > find anyways? Because they do not actually resolve anything in the file system. They take the event as ground truth and use that. Also, the tools expect name=value. This has been the way since the beginning. We do not lump multiple independent values together. And then what if the path has a special character in it? The whole value then has to be encoded. And I don't think the patch is using untrusted string like it should. > Since we have a chance to fix it before it goes upstream, I think it > should either be yanked and respun, or add a corrective patch and submit > them together. > > > >> As for the objection itself: ungh. There is really no good reason why > > >> you couldn't have seen this in the *several* *months* prior to this; > > >> Richard wrote a nice patch description which *included* sample audit > > >> events, and you were involved in discussions regarding this patchset. > > >> To say I'm disappointed would be an understatement. > > > > > > I am also disappointed to find that we are modifying a searchable field > > > that has been defined since 2005. The "name" field is very important. > > > It's used in quite a few reports, its used in the text format, it's > > > searchable, and we have a dictionary that defines exactly what it is. > > > Fields that are searchable and used in common reports cannot be changed > > > without a whole lot of coordination. I'm also disappointed to have to > > > point out that new information should go in its own field. I thought > > > this was common knowledge. In any event, it was caught and problems can > > > be avoided. > > So why does this make it unsearchable? I didn't say it makes in unsearchable, but now that you mention it...it does in one case. Searchable fields are more important. They typically are the object or subject or some kind of special attribute that is commonly searched on to group events. Searchable fields can either be partial or full word match. By combining information in the same field, it will change this behavior. The path name is the object of the event. By combining information that is not the object with it, everyone will have to change and update their software to handle this change in behavior. > I still don't understand any explanations that have been made so far Try ausearch --text on those events, or aureport --file. -Steve
Re: RFC(v2): Audit Kernel Container IDs
On Thursday, October 19, 2017 7:11:33 PM EDT Aleksa Sarai wrote: > >>> The registration is a pseudo filesystem (proc, since PID tree already > >>> exists) write of a u8[16] UUID representing the container ID to a file > >>> representing a process that will become the first process in a new > >>> container. This write might place restrictions on mount namespaces > >>> required to define a container, or at least careful checking of > >>> namespaces in the kernel to verify permissions of the orchestrator so it > >>> can't change its own container ID. A bind mount of nsfs may be > >>> necessary in the container orchestrator's mntNS. > >>> Note: Use a 128-bit scalar rather than a string to make compares faster > >>> and simpler. > >>> > >>> Require a new CAP_CONTAINER_ADMIN to be able to carry out the > >>> registration. > >> > >> Wouldn't CAP_AUDIT_WRITE be sufficient? After all, this is for auditing. > > > > No, because then any process with that capability (vsftpd) could change > > its own container ID. This is discussed more in other parts of the > > thread... For the record, I changed my mind. CAP_AUDIT_CONTROL is the correct capability. > Not if we make the container ID append-only (to support nesting), or > write-once (the other idea thrown around). Well...I like to use lessons learned if they can be applied. In the normal world without containers we have uid, auid, and session_id. uid is who you are now, auid is how you got into the system, session_id distinguishes individual auids. We have a default auid of -1 for system objects and a real number for people. I think there should be the equivalent of auid and session_id but tailored for containers. Loginuid == container id. It can be set, overridden, or appended to (we'll figure this out later) in very limited circumstances. Container_session == session which is tamper-proof. This way things can enter a container with the same ID but under a different session. And everything else gets to inherit the original ID. This way we can trace actions to something that entered the container rather than normal system activity in the container. What a security officer wants to know is what did people do inside the system / container. The system objects we typically don't care about. Sure they might get hacked and then work on behalf of someone, but they would almost always pop a shell so that they can have freedom. That should set off an AVC or create other activity that gets picked up. -Steve > In that case, you can't move "out" from a particular container ID, you can > only go "deeper". These semantics don't make sense for generic containers, > but since the point of this facility is *specifically* for audit I imagine > that not being able to move a process from a sub-container's ID is a > benefit.
[PATCH 1/1] fanotify: pre-approve listener's OPEN_PERM access requests
Hello,
If a daemon using FANOTIFY needs to open a file on a watched filesystem and
its wanting OPEN_PERM events, we get deadlock. (This could happen because
of a library the daemon is using suddenly decides it needs to look in a new
file.) Even though the man page says that the daemon should approve its own
access decision, it really can't. If its in the middle of working on a
request and that in turn generates another request, the second request is
going to sit in the queue inside the kernel and not be read because the
daemon is waiting on a library call that will never finish. We also have no
idea how many requests are stacked up before we get to it. So, it really
can't approve its own access requests.
The solution is to assume that the daemon is going to approve its own file
access requests. So, any requested access that matches the pid of the program
receiving fanotify events should be pre-approved in the kernel and not sent
to user space for approval. This should prevent deadlock.
Signed-off-by: sgrubb
---
fs/notify/fanotify/fanotify.c | 9 +
fs/notify/fanotify/fanotify_user.c | 3 +++
include/linux/fsnotify_backend.h | 2 ++
3 files changed, 14 insertions(+)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d2f97ec..469ce6d 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -105,6 +105,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark
*inode_mark,
{
__u32 marks_mask, marks_ignored_mask;
struct path *path = data;
+ struct pid *cur_pid;
pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
" data_type=%d\n", __func__, inode_mark, vfsmnt_mark,
@@ -139,6 +140,14 @@ static bool fanotify_should_send_event(struct
fsnotify_mark *inode_mark,
BUG();
}
+ /* Assume the listening process will approve its own requests */
+ cur_pid = get_pid(task_tgid(current));
+ if (pid_nr(vfsmnt_mark->group->fanotify_data.pid) == pid_nr(cur_pid)) {
+ put_pid(cur_pid);
+ return false;
+ }
+ put_pid(cur_pid);
+
if (d_is_dir(path->dentry) &&
!(marks_mask & FS_ISDIR & ~marks_ignored_mask))
return false;
diff --git a/fs/notify/fanotify/fanotify_user.c
b/fs/notify/fanotify/fanotify_user.c
index 8e8e6bc..510e3bc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -387,6 +387,8 @@ static int fanotify_release(struct inode *ignored, struct
file *file)
*/
wake_up(&group->fanotify_data.access_waitq);
#endif
+ /* Get rid of reference held since fanotify_init */
+ put_pid(group->fanotify_data.pid);
/* matches the fanotify_init->fsnotify_alloc_group */
fsnotify_destroy_group(group);
@@ -740,6 +742,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags,
unsigned int, event_f_flags)
group->fanotify_data.user = user;
atomic_inc(&user->fanotify_listeners);
+ group->fanotify_data.pid = get_pid(task_tgid(current));
oevent = fanotify_alloc_event(NULL, FS_Q_OVERFLOW, NULL);
if (unlikely(!oevent)) {
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 533c440..48938ad 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -16,6 +16,7 @@
#include
#include
#include
+#include
/*
* IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
@@ -184,6 +185,7 @@ struct fsnotify_group {
int f_flags;
unsigned int max_marks;
struct user_struct *user;
+ struct pid *pid;
} fanotify_data;
#endif /* CONFIG_FANOTIFY */
};
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Should audit_seccomp check audit_enabled?
On Friday, October 23, 2015 03:38:05 PM Paul Moore wrote: > On Fri, Oct 23, 2015 at 1:01 PM, Kees Cook wrote: > > On Fri, Oct 23, 2015 at 9:19 AM, Andy Lutomirski wrote: > >> I would argue that, if auditing is off, audit_seccomp shouldn't do > >> anything. After all, unlike e.g. selinux, seccomp is not a systemwide > >> policy, and seccomp signals might be ordinary behavior that's internal > >> to the seccomp-using application. IOW, for people with audit compiled > >> in and subscribed by journald but switched off, I think that the > >> records shouldn't be emitted. > >> > >> If you agree, I can send the two-line patch. > > > > I think signr==0 states (which I would identify as "intended > > behavior") don't need to be reported under any situation, but audit > > folks wanted to keep it around. > > Wearing my libseccomp hat, I would like some logging when the seccomp > filter triggers a result other than allow. I don't care if this is > via audit or printk(), I just want some notification. If we go the > printk route and people really don't want to see anything in their > logs, I suppose we could always add a sysctl knob to turn off the > message completely (we would still need to do whatever audit records > are required, see below). > > Wearing my audit hat, I want to make sure we tick off all the right > boxes for the various certifications that people care about. Steve > Grubb has commented on what he needs in the past, although I'm not > sure it was on-list, so I'll ask him to repeat it here. I went back and reviewed my notes since this came up in the current Common Criteria evaluation. What we decided to do is treat syscall failures which failed due to seccomp the same as syscall failures caused by dropping capabilities. Both are opt-in DAC policies. That means we don't care. Do whatever you like. :-) -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated
On Wed, 7 Mar 2018 18:43:42 -0500 Paul Moore wrote: > ... and I just realized that linux-audit isn't on the To/CC line, > adding them now. > > Link to the patch is below. > > * https://marc.info/?t=15204188763&r=1&w=2 Yes...I wished I was in on the beginning of this discussion. Here's the problem. We need all tasks auditable unless specifically dismissed as uninteresting. This would be a task,never rule. The way we look at it, is if it boots with audit=1, then we know auditd is expected to run at some point. So, we need all tasks to stay auditable. If they weren't and auditd enabled auditing, then we'd need to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every process in the system. It was decided that this is too ugly. So, we need them all to be auditable if there is any intent to audit. It doesn't matter if there are rules loaded or not. All processes have to stay within reach. What might be acceptable is to add one more state to audit boot variable to indicate that auditing is never expected. We currently have: disabled - which means we'll decide later, enabled, and immutable (no changes allowed). Then have calls to audit_enable or loading rules fail on that flag state so that user space can log that there is a conflict (boot vs daemon) that has to be resolved. As long as we can fail in a discoverable way, I think it would be OK to do something like this. Also, I don't think we want that to be the default state at the moment because the current default is keep all processes auditable. -Steve
Re: [PATCH ghak21 V2 0/4] audit: address ANOM_LINK excess records
On Mon, 12 Mar 2018 02:31:16 -0400
Richard Guy Briggs wrote:
> Audit link denied events were being unexpectedly produced in a
> disjoint way when audit was disabled, and when they were expected,
> there were duplicate PATH records. This patchset addresses both
> issues for symlinks and hardlinks.
>
> This was introduced with
> commit b24a30a7305418ff138ff51776fc555ec57c011a
> ("audit: fix event coverage of AUDIT_ANOM_LINK")
> commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
> ("fs: add link restriction audit reporting")
>
> Here are the resulting events:
>
> symlink:
> type=PROCTITLE msg=audit(03/12/2018 02:21:49.578:310) :
> proctitle=ls ./my-passwd type=PATH msg=audit(03/12/2018
> 02:21:49.578:310) : item=1 name=/tmp/ inode=13529 dev=00:27
> mode=dir,sticky,777 ouid=root ogid=root rdev=00:00
> obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none
> cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(03/12/2018
> 02:21:49.578:310) : item=0 name=./my-passwd inode=17090 dev=00:27
> mode=link,777 ouid=rgb ogid=rgb rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none
> cap_fi=none cap_fe=0 cap_fver=0 type=CWD msg=audit(03/12/2018
> 02:21:49.578:310) : cwd=/tmp type=SYSCALL msg=audit(03/12/2018
> 02:21:49.578:310) : arch=x86_64 syscall=stat success=no
> exit=EACCES(Permission denied) a0=0x7ffd79950dda a1=0x563f658a03c8
> a2=0x563f658a03c8 a3=0x79950d00 items=2 ppid=552 pid=629 auid=root
> uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root
> fsgid=root tty=ttyS0 ses=1 comm=ls exe=/usr/bin/ls
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> type=ANOM_LINK msg=audit(03/12/2018 02:21:49.578:310) :
> op=follow_link ppid=552 pid=629 auid=root uid=root gid=root euid=root
> suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1
> comm=ls exe=/usr/bin/ls
So, if we now only emit the ANOM_LINK event when audit is enabled, we
should get rid of all the duplicate information in that record. The
SYSCALL record has all that information.
-Steve
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> hardlink: type=PROCTITLE msg=audit(03/12/2018
> 02:24:39.813:314) : proctitle=ln test test-ln type=PATH
> msg=audit(03/12/2018 02:24:39.813:314) : item=1 name=/tmp inode=13529
> dev=00:27 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00
> obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none
> cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(03/12/2018
> 02:24:39.813:314) : item=0 name=test inode=18112 dev=00:27
> mode=file,700 ouid=root ogid=root rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none
> cap_fi=none cap_fe=0 cap_fver=0 type=CWD msg=audit(03/12/2018
> 02:24:39.813:314) : cwd=/tmp type=SYSCALL msg=audit(03/12/2018
> 02:24:39.813:314) : arch=x86_64 syscall=linkat success=no
> exit=EPERM(Operation not permitted) a0=0xff9c a1=0x7ffccba77629
> a2=0xff9c a3=0x7ffccba7762e items=2 ppid=605 pid=638 auid=rgb
> uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb sgid=rgb
> fsgid=rgb tty=pts0 ses=4 comm=ln exe=/usr/bin/ln
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> type=ANOM_LINK msg=audit(03/12/2018 02:24:39.813:314) : op=linkat
> ppid=605 pid=638 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb
> egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=4 comm=ln exe=/usr/bin/ln
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> See also: https://github.com/linux-audit/audit-kernel/issues/51
>
> Richard Guy Briggs (4):
> audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
> audit: link denied should not directly generate PATH record
> audit: add refused symlink to audit_names
> audit: add parent of refused symlink to audit_names
>
> fs/namei.c| 5 +++--
> include/linux/audit.h | 9 +
> kernel/audit.c| 43
> --- 3 files changed, 40
> insertions(+), 17 deletions(-)
>
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Mon, 12 Mar 2018 11:52:56 -0400
Richard Guy Briggs wrote:
> On 2018-03-12 11:53, Paul Moore wrote:
> > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > wrote:
> > > On 2018-03-12 11:12, Paul Moore wrote:
> > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > >> wrote:
> > >> > Audit link denied events for symlinks had duplicate PATH
> > >> > records rather than just updating the existing PATH record.
> > >> > Update the symlink's PATH record with the current dentry and
> > >> > inode information.
> > >> >
> > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > >> > Signed-off-by: Richard Guy Briggs
> > >> > ---
> > >> > fs/namei.c | 1 +
> > >> > 1 file changed, 1 insertion(+)
> > >>
> > >> Why didn't you include this in patch 4/4 like I asked during the
> > >> previous review?
> > >
> > > Please see the last comment of:
> > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> >
> > Yes, I just saw that ... I hadn't seen your replies on the v1
> > patches until I had finished reviewing v2. I just replied to that
> > mail in the v1 thread, but basically you need to figure out what is
> > necessary here and let us know. If I have to figure it out it
> > likely isn't going to get done with enough soak time prior to the
> > upcoming merge window.
>
> Steve? I was hoping you could chime in here.
If the CWD record will always be the same as the PARENT record, then we
do not need the parent record. Duplicate information is bad. Like all
the duplicate SYSCALL information.
-Steve
> I'd just include it for completeness unless Steve thinks it will stand
> on its own and doesn't want the overhead.
>
> > >> > diff --git a/fs/namei.c b/fs/namei.c
> > >> > index 50d2533..00f5041 100644
> > >> > --- a/fs/namei.c
> > >> > +++ b/fs/namei.c
> > >> > @@ -945,6 +945,7 @@ static inline int may_follow_link(struct
> > >> > nameidata *nd) if (nd->flags & LOOKUP_RCU)
> > >> > return -ECHILD;
> > >> >
> > >> > + audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> > >> > audit_log_link_denied("follow_link",
> > >> > &nd->stack[0].link); return -EACCES;
> > >> > }
> > >>
> > >> paul moore
> > >
> > > - RGB
> >
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Tue, 13 Mar 2018 06:11:08 -0400
Richard Guy Briggs wrote:
> On 2018-03-13 09:35, Steve Grubb wrote:
> > On Mon, 12 Mar 2018 11:52:56 -0400
> > Richard Guy Briggs wrote:
> >
> > > On 2018-03-12 11:53, Paul Moore wrote:
> > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > > wrote:
> > > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > > >> wrote:
> > > > >> > Audit link denied events for symlinks had duplicate PATH
> > > > >> > records rather than just updating the existing PATH record.
> > > > >> > Update the symlink's PATH record with the current dentry
> > > > >> > and inode information.
> > > > >> >
> > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > > > >> > Signed-off-by: Richard Guy Briggs
> > > > >> > ---
> > > > >> > fs/namei.c | 1 +
> > > > >> > 1 file changed, 1 insertion(+)
> > > > >>
> > > > >> Why didn't you include this in patch 4/4 like I asked during
> > > > >> the previous review?
> > > > >
> > > > > Please see the last comment of:
> > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > > > >
> > > >
> > > > Yes, I just saw that ... I hadn't seen your replies on the v1
> > > > patches until I had finished reviewing v2. I just replied to
> > > > that mail in the v1 thread, but basically you need to figure
> > > > out what is necessary here and let us know. If I have to
> > > > figure it out it likely isn't going to get done with enough
> > > > soak time prior to the upcoming merge window.
> > >
> > > Steve? I was hoping you could chime in here.
> >
> > If the CWD record will always be the same as the PARENT record,
> > then we do not need the parent record. Duplicate information is
> > bad. Like all the duplicate SYSCALL information.
>
> The CWD record could be different from the PARENT record, since I
> could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test.
> Does the parent record even matter since it might not be a directory
> operation like creat, unlink or rename?
There's 2 issues. One is creating the path if what we have is relative.
In this situation CWD should be enough. But if the question is whether
the PARENT directory should be included...what if the PARENT
permissions do not allow the successful name resolution? In that case
we might only get a PARENT record no? In that case we would need it.
-Steve
> > > I'd just include it for completeness unless Steve thinks it will
> > > stand on its own and doesn't want the overhead.
> > >
> > > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > > >> > index 50d2533..00f5041 100644
> > > > >> > --- a/fs/namei.c
> > > > >> > +++ b/fs/namei.c
> > > > >> > @@ -945,6 +945,7 @@ static inline int
> > > > >> > may_follow_link(struct nameidata *nd) if (nd->flags &
> > > > >> > LOOKUP_RCU) return -ECHILD;
> > > > >> >
> > > > >> > + audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> > > > >> > audit_log_link_denied("follow_link",
> > > > >> > &nd->stack[0].link); return -EACCES;
> > > > >> > }
> > > > >>
> > > > >> paul moore
> > > > >
> > > > > - RGB
> > > >
> > > > paul moore
> > >
> > > - RGB
>
> - RGB
>
> --
> Richard Guy Briggs
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Tue, 13 Mar 2018 06:52:51 -0400
Richard Guy Briggs wrote:
> On 2018-03-13 11:38, Steve Grubb wrote:
> > On Tue, 13 Mar 2018 06:11:08 -0400
> > Richard Guy Briggs wrote:
> >
> > > On 2018-03-13 09:35, Steve Grubb wrote:
> > > > On Mon, 12 Mar 2018 11:52:56 -0400
> > > > Richard Guy Briggs wrote:
> > > >
> > > > > On 2018-03-12 11:53, Paul Moore wrote:
> > > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > > > > wrote:
> > > > > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > > > > >> wrote:
> > > > > > >> > Audit link denied events for symlinks had duplicate
> > > > > > >> > PATH records rather than just updating the existing
> > > > > > >> > PATH record. Update the symlink's PATH record with the
> > > > > > >> > current dentry and inode information.
> > > > > > >> >
> > > > > > >> > See:
> > > > > > >> > https://github.com/linux-audit/audit-kernel/issues/21
> > > > > > >> > Signed-off-by: Richard Guy Briggs ---
> > > > > > >> > fs/namei.c | 1 +
> > > > > > >> > 1 file changed, 1 insertion(+)
> > > > > > >>
> > > > > > >> Why didn't you include this in patch 4/4 like I asked
> > > > > > >> during the previous review?
> > > > > > >
> > > > > > > Please see the last comment of:
> > > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > > > > > >
> > > > > >
> > > > > > Yes, I just saw that ... I hadn't seen your replies on the
> > > > > > v1 patches until I had finished reviewing v2. I just
> > > > > > replied to that mail in the v1 thread, but basically you
> > > > > > need to figure out what is necessary here and let us know.
> > > > > > If I have to figure it out it likely isn't going to get
> > > > > > done with enough soak time prior to the upcoming merge
> > > > > > window.
> > > > >
> > > > > Steve? I was hoping you could chime in here.
> > > >
> > > > If the CWD record will always be the same as the PARENT record,
> > > > then we do not need the parent record. Duplicate information is
> > > > bad. Like all the duplicate SYSCALL information.
> > >
> > > The CWD record could be different from the PARENT record, since I
> > > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test.
> > > Does the parent record even matter since it might not be a
> > > directory operation like creat, unlink or rename?
> >
> > There's 2 issues. One is creating the path if what we have is
> > relative. In this situation CWD should be enough. But if the
> > question is whether the PARENT directory should be included...what
> > if the PARENT permissions do not allow the successful name
> > resolution? In that case we might only get a PARENT record no? In
> > that case we would need it.
>
> I think in the case of symlink creation, normal file create code path
> would be in effect, and would properly log parent and symlink source
> file paths (if a rule to log it was in effect) which is not something
> that would trigger a symlink link denied error. Symlink link denied
> happens only when trying to actually follow the link before
> resolving the target path of a read/write/exec of the symlink target.
>
> If the parent permissions of the link's target don't allow successful
> name resolution then the symlink link denied condition isn't met, but
> rather any other rule that applies to the target path.
Then I guess the PARENT record is not needed.
-Steve
> > > > > I'd just include it for completeness unless Steve thinks it
> > > > > will stand on its own and doesn't want the overhead.
> > > > >
> > > > > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > >> > index 50d2533..00f5041 100644
> > > > > > >> > --- a/fs/namei.c
> > > > > > >> > +++ b/fs/namei.c
> > > > > > >> > @@ -945,6 +945,7 @@ static inline int
> > > > > > >> > may_follow_link(struct nameidata *nd) if (nd->flags &
> > > > > > >> > LOOKUP_RCU) return -ECHILD;
> > > > > > >> >
> > > > > > >> > + audit_inode(nd->name,
> > > > > > >> > nd->stack[0].link.dentry, 0);
> > > > > > >> > audit_log_link_denied("follow_link",
> > > > > > >> > &nd->stack[0].link); return -EACCES; }
> > > > > > >>
> > > > > > >> paul moore
> > > > > > >
> > > > > > > - RGB
> > > > > >
> > > > > > paul moore
> > > > >
> > > > > - RGB
> > >
> > > - RGB
> > >
> > > --
> > > Richard Guy Briggs
> > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > Remote, Ottawa, Red Hat Canada
> > > IRC: rgb, SunRaycer
> > > Voice: +1.647.777.2635, Internal: (81) 32635
> >
>
> - RGB
>
> --
> Richard Guy Briggs
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs wrote:
> Implement the proc fs write to set the audit container ID of a
> process, emitting an AUDIT_CONTAINER record to document the event.
>
> This is a write from the container orchestrator task to a proc entry
> of the form /proc/PID/containerid where PID is the process ID of the
> newly created task that is to become the first task in a container,
> or an additional task added to a container.
>
> The write expects up to a u64 value (unset: 18446744073709551615).
>
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> res=0
The was one thing I was wondering about. Currently when we set the
loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
set the container id, the event should be AUDIT_CONTAINERID or
AUDIT_CONTAINER_ID.
During syscall events, the path info is returned in a a record simply
called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
calling the record that gets attached to everything
AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> The "op" field indicates an initial set. The "pid" to "ses" fields
> are the orchestrator while the "opid" field is the object's PID, the
> process being "contained". Old and new container ID values are given
> in the "contid" fields, while res indicates its success.
>
> It is not permitted to self-set, unset or re-set the container ID. A
> child inherits its parent's container ID, but then can be set only
> once after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
>
> Signed-off-by: Richard Guy Briggs
> ---
> fs/proc/base.c | 37
> include/linux/audit.h | 16 +
> include/linux/init_task.h | 4 ++-
> include/linux/sched.h | 1 +
> include/uapi/linux/audit.h | 2 ++
> kernel/auditsc.c | 84
> ++ 6 files changed, 143
> insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> * file, char __user * buf, .read = proc_sessionid_read,
> .llseek = generic_file_llseek,
> };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char
> __user *buf,
> +size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + u64 containerid;
> + int rv;
> + struct task_struct *task = get_proc_task(inode);
> +
> + if (!task)
> + return -ESRCH;
> + if (*ppos != 0) {
> + /* No partial writes. */
> + put_task_struct(task);
> + return -EINVAL;
> + }
> +
> + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> + if (rv < 0) {
> + put_task_struct(task);
> + return rv;
> + }
> +
> + rv = audit_set_containerid(task, containerid);
> + put_task_struct(task);
> + if (rv < 0)
> + return rv;
> + return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> + .write = proc_containerid_write,
> + .llseek = generic_file_llseek,
> +};
> +
> #endif
>
> #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> CONFIG_AUDITSYSCALL REG("loginuid", S_IWUSR|S_IRUGO,
> proc_loginuid_operations), REG("sessionid", S_IRUGO,
> proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), diff --git a/include/linux/audit.h
> b/include/linux/audit.h index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET
>
> struct audit_sig_info {
> uid_t uid;
> @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct
> task_struct *t) extern int auditsc_get_stamp(struct audit_context
> *ctx, struct timespec64 *t, unsigned int *serial);
> extern int audit_set_loginuid(k
Re: [RFC PATCH ghak32 V2 03/13] audit: log container info of syscalls
On Fri, 16 Mar 2018 05:00:30 -0400
Richard Guy Briggs wrote:
> Create a new audit record AUDIT_CONTAINER_INFO to document the
> container ID of a process if it is present.
As mentioned in a previous email, I think AUDIT_CONTAINER is more
suitable for the container record. One more comment below...
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c03e syscall=257
> success=yes exit=3 a0=ff9c a1=56374e1cef30 a2=241 a3=1b6 items=2
> ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
> sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key="tmpcontainerid" type=CWD msg=audit(1519924845.499:257):
> cwd="/root" type=PATH msg=audit(1519924845.499:257): item=0
> name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0
> rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT
> cap_fp= cap_fi= cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1
> name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0
> ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0
> nametype=CREATE cap_fp= cap_fi=
> cap_fe=0 cap_fver=0 type=PROCTITLE msg=audit(1519924845.499:257):
> proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER_INFO msg=audit(1519924845.499:257): op=task
> contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 5 +
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 20
> kernel/auditsc.c | 2 ++
> 4 files changed, 28 insertions(+)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index fe4ba3f..3acbe9d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -154,6 +154,8 @@ extern void
> audit_log_link_denied(const char *operation, extern int
> audit_log_task_context(struct audit_buffer *ab); extern void
> audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk);
> +extern int audit_log_container_info(struct task_struct *tsk,
> + struct audit_context *context);
>
> extern int audit_update_lsm_rules(void);
>
> @@ -205,6 +207,9 @@ static inline int audit_log_task_context(struct
> audit_buffer *ab) static inline void audit_log_task_info(struct
> audit_buffer *ab, struct task_struct *tsk)
> { }
> +static inline int audit_log_container_info(struct task_struct *tsk,
> + struct audit_context
> *context); +{ }
> #define audit_enabled 0
> #endif /* CONFIG_AUDIT */
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 921a71f..e83ccbd 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
> #define AUDIT_REPLACE1329/* Replace auditd
> if this packet unanswerd */ #define AUDIT_KERN_MODULE
> 1330 /* Kernel Module events */ #define
> AUDIT_FANOTIFY1331/* Fanotify access decision
> */ +#define AUDIT_CONTAINER_INFO 1332/* Container ID
> information */ #define AUDIT_AVC 1400/* SE
> Linux avc denial or grant */ #define AUDIT_SELINUX_ERR
> 1401 /* Internal SE Linux Errors */ diff --git
> a/kernel/audit.c b/kernel/audit.c index 3f2f143..a12f21f 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2049,6 +2049,26 @@ void audit_log_session_info(struct
> audit_buffer *ab) audit_log_format(ab, " auid=%u ses=%u", auid,
> sessionid); }
>
> +/*
> + * audit_log_container_info - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + */
> +int audit_log_container_info(struct task_struct *tsk, struct
> audit_context *context) +{
> + struct audit_buffer *ab;
> +
> + if (!audit_containerid_set(tsk))
> + return 0;
> + /* Generate AUDIT_CONTAINER_INFO with container ID */
> + ab = audit_log_start(context, GFP_KERNEL,
> AUDIT_CONTAINER_INFO);
> + if (!ab)
> + return -ENOMEM;
> + audit_log_format(ab, "contid=%llu",
> audit_get_containerid(tsk));
> + audit_log_end(ab);
> + return 0;
> +}
> +
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> audit_log_format(ab, " key=");
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a6b0a52..65be110 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1453,6 +1453,8 @@ static void audit_log_exit(struct audit_context
> *context, struct task_struct *ts
> audit_log_proctitle(tsk, context);
>
> + audit_log_container_info(tsk, context);
Would there be any problem moving audit_log_container_info before
audit_log_proctitle? There are some assumptions that proctitle is the
last r
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Thu, 17 May 2018 17:56:00 -0400
Richard Guy Briggs wrote:
> > During syscall events, the path info is returned in a a record
> > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > rather than calling the record that gets attached to everything
> > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
>
> Considering the container initiation record is different than the
> record to document the container involved in an otherwise normal
> syscall, we need two names. I don't have a strong opinion what they
> are.
>
> I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> are different enough to be visually distinct while leaving
> AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> containerid filtering")
How about AUDIT_CONTAINER for the auxiliary record? The one that starts
the container, I don't have a strong opinion on. Could be
AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
for filtering could be AUDIT_CID or AUDIT_CONTID if that helps decide
what the initial event might be. Normally, it should match the field
being filtered.
Best Regards,
-Steve
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Fri, 18 May 2018 11:21:06 -0400
Richard Guy Briggs wrote:
> On 2018-05-18 09:56, Steve Grubb wrote:
> > On Thu, 17 May 2018 17:56:00 -0400
> > Richard Guy Briggs wrote:
> >
> > > > During syscall events, the path info is returned in a a record
> > > > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > > > rather than calling the record that gets attached to everything
> > > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > >
> > > Considering the container initiation record is different than the
> > > record to document the container involved in an otherwise normal
> > > syscall, we need two names. I don't have a strong opinion what
> > > they are.
> > >
> > > I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> > > are different enough to be visually distinct while leaving
> > > AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> > > containerid filtering")
>
> (Sorry, I had intended AUDIT_CONTAINER for the first in that paragraph
> above.)
>
> > How about AUDIT_CONTAINER for the auxiliary record? The one that
> > starts the container, I don't have a strong opinion on. Could be
> > AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
> > AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
> > for filtering could be AUDIT_CID or AUDIT_CONTID if that helps
> > decide what the initial event might be. Normally, it should match
> > the field being filtered.
>
> Ok, I had shortened the record field name to "contid=" to be unique
> enough while not using too much netlink bandwidth. I could have used
> "cid=" but that could be unobvious or ambiguous. I didn't want to use
> the full "containerid=" due to that. I suppose I could change the
> field name macro to AUDIT_CONTID.
>
> For the one that starts the container, I'd prefer to leave the name a
> bit more general than "_INIT", "_START", so maybe I'll swap them
> around and use AUDIT_CONTAINER_INFO for the startup record, and use
> AUDIT_CONTAINER for the syscall auxiliary record.
>
> Does that work?
I'll go along with that. Thanks. But making that swap frees up
AUDIT_CONTAINER_ID which could be the first event. But
AUDIT_CONTAINER_INFO is also fine with me.
Best Regards,
-Steve
Re: [PATCH] TaskTracker : Simplified thread information tracker.
On Monday, January 12, 2015 03:13:12 PM Tetsuo Handa wrote:
> Thank you for comments.
>
> Richard Guy Briggs wrote:
> > Steve already mentioned any user-influenced fields need to be escaped,
> > so I'd recommend audit_log_untrustedstring() as being much simpler from
> > your perspective and much better tested and understood from audit
> > maintainer's perspective. At least use the existing 'o' printf format
> > specifier instead of inventing your own. I do acknowledge that the
> > resulting output from your function is easier to read in its raw format
> > passed from the kernel, however, it makes your code harder to maintain.
>
> I'm not sure whether I should use audit_log_untrustedstring().
That is the accepted encoding. We do not want 2 different kinds of encoding
functions.
> This record contains multiple user-influenced comm names. If I use
> audit_log_untrustedstring(), I would need to split this record into
> multiple records like history[0]='...' history[1]='...' history[2]='...'
> in order to avoid matching delimiters (i.e. ';', '=' and '>') used in
> this record.
That sounds like a good change to me. Audit records are always name=value with
a space between fields. We need this to always stay like this because the
tooling expects that format. There is nowhere in the audit logs we use =>.
> This would also change from "char *" in "struct task_struct"
> to array of struct { "comm name", "pid", "stamp" } in "struct task_struct".
> I don't know whether such change makes easier to maintain than now.
You can still use char *, just use history[x]= to append with. We faced the
same issue regarding argv[] logging. You might look at the execve record
generation to get some ideas how that was handled.
> > As for the date-stamping bits, they seem to be the majority of the code
> > in audit_update_history(). I'd just emit a number and punt that to
> > userspace for decoding. Alternatively, I'd use an existing service in
> > the kernel to do that date formatting, or at least call a new function
> > to format that date string should a suitable one not already exist, so
> > you can remove that complexity from audit_update_history().
>
> Since I don't know existing functions for date formatting,
All time in the audit system is "%lu.%03lu", t.tv_sec, t.tv_nsec/100. User
space tooling expects this.
-Steve
> I split it as a new function. If it is acceptable, I'd like to make that
> function public and replace tomoyo_convert_time() in security/tomoyo/util.c
> with that function.
>
> Regards.
>
>
> >From 50d59b5640a7501b8d5f843fb57283fcb62b1118 Mon Sep 17 00:00:00 2001
>
> From: Tetsuo Handa
> Date: Mon, 12 Jan 2015 14:45:23 +0900
> Subject: [PATCH] audit: Emit history of thread's comm name.
>
> When an unexpected system event (e.g. reboot) occurs, the administrator may
> want to identify which application triggered the event. System call auditing
> could be used for recording such event. However, the audit log may not be
> able to provide sufficient information for identifying the application
> because the audit log does not reflect how the program was executed.
>
> This patch adds ability to trace how the program was executed and emit it
> as an auxiliary record in the form of comm name, pid and time stamp pairs
> as of execve().
>
> type=UNKNOWN[1329] msg=audit(1421039813.810:3693): history='
> name=swapper\0570;pid=1;start=20150112140720=>name=init;pid=1;
> start=20150112140721=>name=systemd;pid=1;start=20150112140722=>
> name=sshd;pid=2473;start=20150112050733=>name=sshd;pid=9838;
> start=20150112051105=>name=bash;pid=9840;start=20150112051108=>
> name=tail;pid=9876;start=20150112051653'
>
> Since converting all bytes using audit_log_untrustedstring() makes this
> record unparsable because this record includes multiple user-influenced
> comm names which may match delimiters used in this record (i.e. ';', '='
> and '>'), only bytes which are not alphabets, numbers, '.', '_' nor '-' are
> escaped.
>
> Signed-off-by: Tetsuo Handa
> ---
> fs/exec.c | 1 +
> include/linux/audit.h | 4 ++
> include/linux/init_task.h | 5 ++
> include/linux/sched.h | 3 +
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 1 +
> kernel/auditsc.c | 155
> + 7 files changed, 170
> insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index ad8798e..5e92651 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1200,6 +1200,7 @@ void install_exec_creds(struct linux_binprm *bprm)
> commit_creds(bprm->cred);
> bprm->cred = NULL;
>
> + audit_update_history();
> /*
>* Disable monitoring for regular users
>* when executing setuid binaries. Must
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af84234..74310a7 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -221,6 +221,8
Re: [PATCH] audit: add containerid support for IMA-audit
On Monday, May 21, 2018 5:57:29 PM EDT Stefan Berger wrote: > Should some of the fields from INTEGRITY_PCR also appear in > INTEGRITY_RULE? If so, which ones? > >>> > >>> pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > >>> required to be searchable > >>> > We could probably refactor the current integrity_audit_message() and > have ima_parse_rule() call into it to get those fields as well. I > suppose adding new fields to it wouldn't be considered breaking user > space? > >>> > >>> The audit user space utilities pretty much expects those fields in that > >>> order for any IMA originating events. You can add things like op or > >>> cause before > >> > >> We will call into audit_log_task, which will put the parameters into > >> correct order: > >> > >> auid uid gid ses subj pid comm exe > > > > I'm telling you what the correct order is. :-) A long time ago, the IMA > > Thanks. Was getting confused. > > > system had audit events with the order I'm mentioning. For example, > > here's one from a log I collected back in 2013: > > > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 > > auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure" > > cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs > > ino=5413 res=0 > > > > it was missing "tty" and "exe", but the order is as I mentioned. The > > expectation is that INTEGRITY events maintain this established order > > across all events. > > I am *appending* exe= and tty= now: > > type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > op="invalid_pcr" cause="open_writers" comm="ssh" > name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 > exe="/usr/bin/ssh" tty=tty2 It is safe to put them where they belong. The tools currently skip over tty and exe if they would be present. So, nothing would get messed up. Its very simple to add an "if" statement to check for these new fields and collect them for searching. Also, "res" is traditionally the last field in any event. Thanks, -Steve
Re: [PATCH] audit: add containerid support for IMA-audit
On Tuesday, May 22, 2018 9:43:46 AM EDT Richard Guy Briggs wrote: > On 2018-05-21 17:57, Stefan Berger wrote: > > On 05/21/2018 02:30 PM, Steve Grubb wrote: > > > Hello Stefan, > > > > > > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote: > > > > On 05/21/2018 12:58 PM, Steve Grubb wrote: > > > > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote: > > > > > > > audit_log_container_info() then releasing the local context. > > > > > > > This > > > > > > > version of the record has additional concerns covered here: > > > > > > > https://github.com/linux-audit/audit-kernel/issues/52 > > > > > > > > > > > > Following the discussion there and the concern with breaking user > > > > > > space, > > > > > > how can we split up the AUDIT_INTEGRITY_RULE that is used in > > > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking > > > > > > user > > > > > > space'? > > > > > > > > > > > > A message produced by ima_parse_rule() looks like this here: > > > > > > > > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): > > > > > > action="dont_measure" > > > > > > fsmagic="0x9fa0" res=1 > > > > > > > > > > Why is action and fsmagic being logged as untrusted strings? > > > > > Untrusted > > > > > strings are used when an unprivileged user can affect the contents > > > > > of the > > > > > field such as creating a file with space or special characters in > > > > > the > > > > > name. > > > > > > > > > > Also, subject and object information is missing. Who loaded this > > > > > rule? > > > > > > > > > > > in contrast to that an INTEGRITY_PCR record type: > > > > > > > > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 > > > > > > auid=0 > > > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > > > > op="invalid_pcr" cause="open_writers" comm="scp" > > > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1 > > > > > > > > > > Why is op & cause being logged as an untrusted string? This also > > > > > has > > > > > incomplete subject information. > > > > > > > > It's calling audit_log_string() in both cases: > > > > > > > > https://elixir.bootlin.com/linux/latest/source/security/integrity/int > > > > egrity _audit.c#L48 > > > > > > I see. Looking things over, I see that it seems like it should do the > > > right thing. But the original purpose for this function is here: > > > > > > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944 > > > > > > This is where it is logging an untrusted string and has to decide to > > > encode it or just place it in quotes. If it has quotes, that means > > > it's an untrusted string but has no control characters in it. I think > > > you want to use audit_log_format() for any string that is trustworthy. > > > > Replacing all occurrences (in IMA) of audit_log_string() with > > audit_log_format(). > > > > > As an aside, I wonder why audit_log_string() is in the API when it is a > > > helper to audit_log_untrustedstring() ? Without understanding the > > > rules of untrusted strings, it can't be used correctly without > > > re-inventing audit_log_untrustedstring() by hand. > > > > > > > > > Should some of the fields from INTEGRITY_PCR also appear in > > > > > > INTEGRITY_RULE? If so, which ones? > > > > > > > > > > pid, uid, auid, tty, session, subj, comm, exe, res. <- these are > > > > > required to be searchable > > > > > > > > > > > We could probably refactor the current integrity_audit_message() > > > > > > and > > > > > > have ima_parse_rule() call into it to get those fields as well. I > > > > > > suppose adding new fields to it wouldn't be considered breaking > > > > > > user > > > > > >
Re: [PATCH 2/3] seccomp: Audit attempts to modify the actions_logged sysctl
On Tuesday, May 1, 2018 11:18:55 AM EDT Paul Moore wrote: > On Fri, Apr 27, 2018 at 3:16 PM, Tyler Hicks wrote: > > The decision to log a seccomp action will always be subject to the > > value of the kernel.seccomp.actions_logged sysctl, even for processes > > that are being inspected via the audit subsystem, in an upcoming patch. > > Therefore, we need to emit an audit record on attempts at writing to the > > actions_logged sysctl when auditing is enabled. > > > > This patch updates the write handler for the actions_logged sysctl to > > emit an audit record on attempts to write to the sysctl. Successful > > writes to the sysctl will result in a record that includes a normalized > > list of logged actions in the "actions" field and a "res" field equal to > > 0. Unsuccessful writes to the sysctl will result in a record that > > doesn't include the "actions" field and has a "res" field equal to 1. > > > > Not all unsuccessful writes to the sysctl are audited. For example, an > > audit record will not be emitted if an unprivileged process attempts to > > open the sysctl file for reading since that access control check is not > > part of the sysctl's write handler. > > > > Below are some example audit records when writing various strings to the > > actions_logged sysctl. > > > > Writing "not-a-real-action" emits: > > type=CONFIG_CHANGE msg=audit(1524600971.363:119): pid=1651 uid=0 > > auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee" > > op=seccomp-logging res=1 > > > > Writing "kill_process kill_thread errno trace log" emits: > > type=CONFIG_CHANGE msg=audit(1524601023.982:131): pid=1658 uid=0 > > auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee" > > op=seccomp-logging actions="kill_process kill_thread errno trace log" > > res=0 > > I've got some additional comments regarding the fields in the code > below, but it would be good to hear Steve comment on the "actions" > field since his userspace tools are extremely picky about what they > will accept. Its not that the audit user space applications are picky, its that we have a coding standard that everyone needs to abide by so that any parser coded to the specification works. In short, we should not have spaces inside the "" because that can trick a naive parser. What we typically do in a situation like this is add a comma as a separator. But having "" means that the value is untrusted and subject to escaping. I don't think that is the case here. Output is not controlled by the user. Its a list of well known names. > It looks like you are treating the actions as an untrusted string, which is > good, so I suspect you are okay, but still The function below that logs names is calling audit_log_format which does not handle untrusted strings. I would suggest not treating it as an untrusted string, but as a string with no spaces in it. actions=kill_process,kill_thread,errno,trace,log > > Writing the string "log log errno trace kill_process kill_thread", which > > is unordered and contains the log action twice, results in the same > > > > value as the previous example for the actions field: > > type=CONFIG_CHANGE msg=audit(1524601204.365:152): pid=1704 uid=0 > > auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee" > > op=seccomp-logging actions="kill_process kill_thread errno trace log" > > res=0 > > > > No audit records are generated when reading the actions_logged sysctl. > > > > Suggested-by: Steve Grubb > > Signed-off-by: Tyler Hicks > > --- > > > > include/linux/audit.h | 3 +++ > > kernel/auditsc.c | 37 + > > kernel/seccomp.c | 43 ++- > > 3 files changed, 74 insertions(+), 9 deletions(-) > > ... > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 75d5b03..b311d7d 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -233,6 +233,7 @@ extern void __audit_inode_child(struct inode *parent, > > > > const struct dentry *dentry, > > const unsigned char type); > > > > extern void __audit_seccomp(unsigned long syscall, long signr, int > > code); > > > > +extern void audit_seccomp_actions_logged(const char *names, int res); > > > > extern void __audit_ptrace(struct task_struct *t); > > >
Re: [PATCH v2 3/4] seccomp: Audit attempts to modify the actions_logged sysctl
On Wednesday, May 2, 2018 11:53:19 AM EDT Tyler Hicks wrote:
> The decision to log a seccomp action will always be subject to the
> value of the kernel.seccomp.actions_logged sysctl, even for processes
> that are being inspected via the audit subsystem, in an upcoming patch.
> Therefore, we need to emit an audit record on attempts at writing to the
> actions_logged sysctl when auditing is enabled.
>
> This patch updates the write handler for the actions_logged sysctl to
> emit an audit record on attempts to write to the sysctl. Successful
> writes to the sysctl will result in a record that includes a normalized
> list of logged actions in the "actions" field and a "res" field equal to
> 0. Unsuccessful writes to the sysctl will result in a record that
> doesn't include the "actions" field and has a "res" field equal to 1.
>
> Not all unsuccessful writes to the sysctl are audited. For example, an
> audit record will not be emitted if an unprivileged process attempts to
> open the sysctl file for reading since that access control check is not
> part of the sysctl's write handler.
>
> Below are some example audit records when writing various strings to the
> actions_logged sysctl.
>
> Writing "not-a-real-action", when the kernel.seccomp.actions_logged
> sysctl previously was "kill_process kill_thread trap errno trace log",
> emits this audit record:
>
> type=CONFIG_CHANGE msg=audit(1525275273.537:130): op=seccomp-logging
> old-actions=kill_process,kill_thread,trap,errno,trace,log res=0
>
> If you then write "kill_process kill_thread errno trace log", this audit
> record is emitted:
>
> type=CONFIG_CHANGE msg=audit(1525275310.208:136): op=seccomp-logging
> actions=kill_process,kill_thread,errno,trace,log
> old-actions=kill_process,kill_thread,trap,errno,trace,log res=1
>
> If you then write the string "log log errno trace kill_process
> kill_thread", which is unordered and contains the log action twice,
> it results in the same actions value as the previous record:
>
> type=CONFIG_CHANGE msg=audit(1525275325.613:142): op=seccomp-logging
> actions=kill_process,kill_thread,errno,trace,log
> old-actions=kill_process,kill_thread,errno,trace,log res=1
>
> No audit records are generated when reading the actions_logged sysctl.
ACK for the format of the records.
-Steve
> Suggested-by: Steve Grubb
> Signed-off-by: Tyler Hicks
> ---
> include/linux/audit.h | 5 +
> kernel/auditsc.c | 25 +
> kernel/seccomp.c | 51
> ++- 3 files changed, 72
> insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 75d5b03..d4e35e7 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -233,6 +233,8 @@ extern void __audit_inode_child(struct inode *parent,
> const struct dentry *dentry,
> const unsigned char type);
> extern void __audit_seccomp(unsigned long syscall, long signr, int code);
> +extern void audit_seccomp_actions_logged(const char *names,
> + const char *old_names, int res);
> extern void __audit_ptrace(struct task_struct *t);
>
> static inline bool audit_dummy_context(void)
> @@ -502,6 +504,9 @@ 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 void audit_seccomp_actions_logged(const char *names,
> + const char *old_names, int res)
> +{ }
> static inline int auditsc_get_stamp(struct audit_context *ctx,
> struct timespec64 *t, unsigned int *serial)
> {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4e0a4ac..5a0b770 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2478,6 +2478,31 @@ void __audit_seccomp(unsigned long syscall, long
> signr, int code) audit_log_end(ab);
> }
>
> +void audit_seccomp_actions_logged(const char *names, const char
> *old_names, + int res)
> +{
> + struct audit_buffer *ab;
> +
> + if (!audit_enabled)
> + return;
> +
> + ab = audit_log_start(current->audit_context, GFP_KERNEL,
> + AUDIT_CONFIG_CHANGE);
> + if (unlikely(!ab))
> + return;
> +
> + audit_log_format(ab, "op=seccomp-logging");
> +
> + if (names)
> + audit_log_format(ab, " actions=%s", names);
> +
> + if (
Re: [RFC PATCH v1 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
On Tuesday, April 16, 2019 7:49:39 AM EDT Florian Weimer wrote: > * Steve Grubb: > > This flag that is being proposed means that you would have to patch all > > interpreters to use it. If you are sure that upstreams will accept that, > > why not just change the policy to interpreters shouldn't execute > > anything unless the execute bit is set? That is simpler and doesn't need > > a kernel change. And setting the execute bit is an auditable event. > > I think we need something like O_MAYEXEC so that security policies can > be enforced and noexec mounts can be detected. Application whitelisting can already today stop unknown software without needing O_MAYEXEC. > I don't think it's a good idea to do this in userspace, especially the > latter. The problem is that passing O_MAYEXEC is opt-in. You can use ptrace/seccomp/ bpf/LD_PRELOAD/LD_AUDIT to remove that bit from an otherwise normal program. This does not require privs to do so. But let's consider that this comes to pass and every interpreter is updated and IMA can see the O_MAYEXEC flag. Attackers now simply pivot to running programs via stdin. It never touches disk and therefore nothing enforces security policy. This already is among the most common ways that malware runs today to evade detection. -Steve
Re: [PATCH ghak124 v3] audit: log nftables configuration change events
On Thursday, February 11, 2021 11:29:34 AM EST Paul Moore wrote: > > If I'm not mistaken, iptables emits a single audit log per table, ipset > > doesn't support audit at all. So I wonder how much audit logging is > > required at all (for certification or whatever reason). How much > > granularity is desired? > I believe the netfilter auditing was mostly a nice-to-have bit of > functionality to help add to the completeness of the audit logs, but I > could very easily be mistaken. Richard put together those patches, he > can probably provide the background/motivation for the effort. There are certifications which levy requirements on information flow control. The firewall can decide if information should flow or be blocked. Information flow decisions need to be auditable - which we have with the audit target. That then swings in requirements on the configuration of the information flow policy. The requirements state a need to audit any management activity - meaning the creation, modification, and/or deletion of a "firewall ruleset". Because it talks constantly about a ruleset and then individual rules, I suspect only 1 summary event is needed to say something happened, who did it, and the outcome. This would be in line with how selinux is treated: we have 1 summary event for loading/modifying/unloading selinux policy. Hope this helps... -Steve
Re: [PATCH ghak25 v6] audit: add subj creds to NETFILTER_CFG record to cover async unregister
On Wednesday, May 20, 2020 2:40:45 PM EDT Paul Moore wrote: > On Wed, May 20, 2020 at 12:55 PM Richard Guy Briggs wrote: > > On 2020-05-20 12:51, Richard Guy Briggs wrote: > > > Some table unregister actions seem to be initiated by the kernel to > > > garbage collect unused tables that are not initiated by any userspace > > > actions. It was found to be necessary to add the subject credentials > > > to cover this case to reveal the source of these actions. A sample > > > record: > > > > > > The uid, auid, tty, ses and exe fields have not been included since > > > they > > > are in the SYSCALL record and contain nothing useful in the non-user > > > context. > > > > > > type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat > > > family=bridge entries=0 op=unregister pid=153 > > > subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 > > FWIW, that record looks good. It's severely broken cat log.file type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 ausearch -if log.file --format text At 19:33:40 12/31/1969 did-unknown ausearch -if log.file --format csv NODE,EVENT,DATE,TIME,SERIAL_NUM,EVENT_KIND,SESSION,SUBJ_PRIME,SUBJ_SEC,SUBJ_KIND,ACTION,RESULT,OBJ_PRIME,OBJ_SEC,OBJ_KIND,HOW error normalizing NETFILTER_CFG ,NETFILTER_CFG,12/31/1969,19:33:40,0,, This is unusable. This is why the bug was filed in the first place. -Steve > > > Signed-off-by: Richard Guy Briggs > > > > Self-NACK. I forgot to remove cred and tty declarations.
Re: race in audit_log_untrusted_string for task_struct::comm
On Saturday, March 15, 2014 07:28:46 PM Richard Guy Briggs wrote: > I'm inclined to go get_task_comm() in all 5 locations, but if we care > more about locking overhead, I'll switch to memcpy(). > > Steve, do we care about the integrity of the comm field? In the case of interpreters, its about the only thing we know about the application being executed. For example, a shell script will have exe=/bin/sh, so comm= is our only clue. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] audit: Simplify by assuming the callers socket buffer is large enough
On Tuesday, March 04, 2014 07:21:52 PM David Miller wrote: > From: ebied...@xmission.com (Eric W. Biederman) > Date: Tue, 04 Mar 2014 14:41:16 -0800 > > > If we really want the ability to always appened to the queue of skb's > > is to just have a version of netlink_send_skb that ignores the queued > > limits. Of course an evil program then could force the generation of > > enough audit records to DOS the kernel, but we seem to be in that > > situation now. Shrug. > > There is never a valid reason to bypass the socket limits. > > It protects the system from things going out of control. > > Netlink packet sends can fail, and audit should cope with that > event instead of trying to bludgeon it into not happening. I am not sure what exactly is the problem with the code that was being patched. The audit code has been stable and working pretty well for everyone for the last 5-6 years. But lately there has been a lot of kernel code churn and I am concerned that the changes are being made without a complete understanding of the design goals. The audit system has to be very reliable. It can't lose any event or record. The people that really depend on it would rather have access denied to the system than lose any event. This is the reason it goes to such lengths. If I understand the patch, it looks like its affecting code that adds, deletes, or lists audit rules. This code is quite old and working well. It didn't at first. We found a lot of problems by writing scripts like: #!/bin/bash while [ 1 ] ; do echo "Inserting syscalls..." sys=1 while [ $sys -lt 100 ] do sys=`expr $sys + 1` echo "$sys" auditctl -a entry,always -S $sys done echo "Listing..." auditctl -l echo "Deleting..." auditctl -D done with another shell running: #!/bin/bash while [ 1 ] ; do echo "Listing..." auditctl -l done and another shell running: #!/bin/bash while [ 1 ] ; do echo "Disabling..." auditctl -e 0 echo "Enabling..." auditctl -e 1 done You can probably imagine more stress tests. But the proposed code should be well tested similar to this. Thanks, -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] audit: Turn off TIF_SYSCALL_AUDIT when there are no rules
On Monday, February 10, 2014 09:29:19 AM Andy Lutomirski wrote: > Grr. Why is all this crap tied up with syscall auditing anyway? ISTM > it would have been a lot nicer if audit calls just immediately emitted > audit records, completely independently of the syscall machinery. Because the majority of people needing audit need syscall records for it to make any sense. The auxiliary records generally report on the object of the syscall. We still require information about who was doing something, what they were doing, and what the result was. Even if you just get the AVC's, you still don't know what happened. If you get a deny record, was it really denied? The system could have been in permissive mode and the syscall succeeded. You only get the real decision when you have syscall records. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] audit: Turn off TIF_SYSCALL_AUDIT when there are no rules
On Monday, February 10, 2014 11:01:36 AM Andy Lutomirski wrote: > >> And I still think this needs more changes. Once again, I do not think > >> that, say, __audit_log_bprm_fcaps() should populate context->aux if > >> !TIF_SYSCALL_AUDIT, this list can grow indefinitely. Or > >> __audit_signal_info()... > >> > >> Perhaps __audit_syscall_exit() should also set context->dummy? > > > > That would work. > > > > I'm still torn between trying to make it possible for things like > > __audit_log_bprm_fcaps to start a syscall audit record in the middle > > of a syscall or to just try to tighten up the current approach to the > > point where it will work correctly. > > This is worse than I thought. Things like signal auditing can enter > the audit system from outside of a syscall. I don't think there's > currently any way to tell whether you're in a syscall (when > TIF_SYSCALL_AUDIT is clear) so getting this to work right would > require arch help. > > I'll ask what people on the Fedora list think about just changing the > default to -t task,never. I can't recall ever seeing the task filter used in real life. But assuming you wanted to audit no tasks, what is the difference between using that filter and never setting audit_enable in the first place? -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: add arch field to seccomp event log
On Tuesday, February 18, 2014 03:50:44 PM Richard Guy Briggs wrote:
> > missing '=' but this isn't what audit_get_context() does... it's
> > crappy naming...I'd think a combo of audit_dummy_context() and
> > current->audit_context would be most appropriate.
>
> Ok. I think I finally understand audit_dummy_context(). Thanks for the
> hint. However, it appears it is not useful in this sitation, since if
> there is an audit_context, even a dummy context, it appears arch is
> filled in.
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> @@ -2406,12 +2406,18 @@ void audit_core_dumps(long signr)
> void __audit_seccomp(unsigned long syscall, long signr, int code)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = current->audit_context;
>
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld", signr);
> + audit_log_format(ab, " arch=");
> + if (context)
> + audit_log_format(ab, "%x", context->arch);
> + else
> + audit_log_format(ab, "(null)");
> audit_log_format(ab, " syscall=%ld", syscall);
> audit_log_format(ab, " compat=%d", is_compat_task());
> audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current));
Is there anything that could be passed by the caller that might identify the
syscall ABI when this call was blocked? '(null)' still makes syscall number
uninterpretable.
-Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] integrity: get comm using lock to avoid race in string printing
Hello Mimi, On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > This change is already being upstreamed as commit 73a6b44 "Integrity: > Pass commname via get_task_comm()". While I was looking at Richard's patch, I noticed a few places where cause and op are logged and the string isn't tied together with a _ or -. These are in ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are these fixed upstream? Or should a patch be made? Thanks, -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] audit: Audit proc cmdline value
On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote:
> During an audit event, cache and print the value of the process's
> cmdline value (proc//cmdline). This is useful in situations
> where processes are started via fork'd virtual machines where the
> comm field is incorrect. Often times, setting the comm field still
> is insufficient as the comm width is not very wide and most
> virtual machine "package names" do not fit. Also, during execution,
> many threads have their comm field set as well. By tying it back to
> the global cmdline value for the process, audit records will be more
> complete in systems with these properties. An example of where this
> is useful and applicable is in the realm of Android. With Android,
> their is no fork/exec for VM instances. The bare, preloaded Dalvik
> VM listens for a fork and specialize request. When this request comes
> in, the VM forks, and the loads the specific application (specializing).
> This was done to take advantage of COW and to not require a load of
> basic packages by the VM on very app spawn. When this spawn occurs,
> the package name is set via setproctitle() and shows up in procfs.
> Many of these package names are longer then 16 bytes, the historical
> width of task->comm. Having the cmdline in the audit records will
> couple the application back to the record directly. Also, on my
> Debian development box, some audit records were more useful then
> what was printed under comm.
>
> The cached cmdline is tied to the life-cycle of the audit_context
> structure and is built on demand.
I don't think its a good idea to do this for a number of reasons.
1) don't we have a record type for command line and its arguments? Shouldn't
we use that auxiliary record if we do this?
2) we don't want each and every syscall record to grow huge(r). Remember the
command line can be virtually unlimited in length. Adding this will consume
disk space and we will be able to keep less records than we currently do.
3) User space will now have to parse this field, too. If everything is in 1
field, how can you tell the command from its arguments considering the command
name could have spaces in it. What if the arguments have spaces in them?
Its far better to fix cmd to be bigger than 16 characters than add all this
extra information that is not needed in the audit logs.
-Steve
> Example denial prior to patch (Ubuntu):
> CALL msg=audit(1387828084.070:361): arch=c03e syscall=82 success=yes
> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> ses=4294967295 tty=(none) comm="console-kit-dae"
> exe="/usr/sbin/console-kit-daemon"
> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
>
> After Patches (Ubuntu):
> type=SYSCALL msg=audit(1387828084.070:361): arch=c03e syscall=82
> success=yes exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> ses=4294967295 tty=(none) comm="console-kit-dae"
> exe="/usr/sbin/console-kit-daemon"
> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255
> cmdline="/usr/lib/dbus-1.0/dbus-daemon-launch-helper" key=(null)
>
> Example denial prior to patch (Android):
> type=1300 msg=audit(248323.940:247): arch=4028 syscall=54 per=84
> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> exe="/system/bin/app_process" subj=u:r:bluetooth:s0 key=(null)
>
> After Patches (Android):
> type=1300 msg=audit(248323.940:247): arch=4028 syscall=54 per=84
> success=yes exit=0 a0=39 a1=540b a2=2 a3=750eecec items=0 ppid=224 pid=1858
> auid=4294967295 uid=1002 gid=1002 euid=1002 suid=1002 fsuid=1002 egid=1002
> sgid=1002 fsgid=1002 tty=(none) ses=4294967295 comm="bt_hc_worker"
> exe="/system/bin/app_process" cmdline="com.android.bluetooth"
> subj=u:r:bluetooth:s0 key=(null)
>
> Signed-off-by: William Roberts
> ---
> kernel/audit.h |1 +
> kernel/auditsc.c | 46 ++
> 2 files changed, 47 insertions(+)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index b779642..bd6211f 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -202,6 +202,7 @@ struct audit_context {
> } execve;
> };
> int fds[2];
> + char *cmdline;
>
> #if AUDIT_DEBUG
> int put_count;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 90594c9..cadee2b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -79,6 +79,9 @@
> /* no execve audit message should be longer than this (userspace limits) */
> #define MAX_EXECVE_AUDIT_LEN 7500
>
> +/* max length to print of cmdline value during audit */
> +#define MAX_CMDLINE_AUDIT_LEN 128
> +
> /* number of audit rules */
> int audit_n_rules;
>
> @@
Re: [PATCH v3 3/3] audit: Audit proc cmdline value
On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote: > On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb wrote: > > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote: > >> During an audit event, cache and print the value of the process's > >> cmdline value (proc//cmdline). This is useful in situations > >> where processes are started via fork'd virtual machines where the > >> comm field is incorrect. Often times, setting the comm field still > >> is insufficient as the comm width is not very wide and most > >> virtual machine "package names" do not fit. Also, during execution, > >> many threads have their comm field set as well. By tying it back to > >> the global cmdline value for the process, audit records will be more > >> complete in systems with these properties. An example of where this > >> is useful and applicable is in the realm of Android. With Android, > >> their is no fork/exec for VM instances. The bare, preloaded Dalvik > >> VM listens for a fork and specialize request. When this request comes > >> in, the VM forks, and the loads the specific application (specializing). > >> This was done to take advantage of COW and to not require a load of > >> basic packages by the VM on very app spawn. When this spawn occurs, > >> the package name is set via setproctitle() and shows up in procfs. > >> Many of these package names are longer then 16 bytes, the historical > >> width of task->comm. Having the cmdline in the audit records will > >> couple the application back to the record directly. Also, on my > >> Debian development box, some audit records were more useful then > >> what was printed under comm. > >> > >> The cached cmdline is tied to the life-cycle of the audit_context > >> structure and is built on demand. > > > > I don't think its a good idea to do this for a number of reasons. > > 1) don't we have a record type for command line and its arguments? > > Shouldn't we use that auxiliary record if we do this? > > Doing this in userspace means each and every user-space would have to be > patched to support this. Other people from various systems have jumped in > adding how this would be beneficial to their cause. The data is right here in > the kernel. I don't mean doing it in user space, I was thinking of perhaps using the EXECVE auxiliary record type. It looks something like this: type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping" a1="koji.fedoraproject.org" Its a record type we already have with a format that can be parsed. > > 2) we don't want each and every syscall record to grow huge(r). Remember > > the command line can be virtually unlimited in length. Adding this will > > consume disk space and we will be able to keep less records than we > > currently do. > We cap it at 128 chars in v3 patch, and then this value can be altered out > of tree and tuned for various systems. That still adds up on systems where people really do use audit. > > 3) User space will now have to parse this field, too. If everything is in > > 1 > > field, how can you tell the command from its arguments considering the > > command name could have spaces in it. What if the arguments have spaces > > in them? > > How did bash figure this out to run the command? It was shell escaped and quoted when bash saw it. Its not when the kernel sees it. > All the fields in audit are KVP based, the parsing is pretty straight > forward. Try this, cp /bin/ls 'test test test' auditctll -a always,exit -F arch=b64 -S stat -k test ./test\ test\ test './test\ test\ test' auditctl -D ausearch --start recent --key test > On the event of weird chars, it gets hex escaped. and its all in 1 lump with no escaping to figure out what is what. > > Its far better to fix cmd to be bigger than 16 characters than add all > > this > > extra information that is not needed in the audit logs. > > Rather then use some transient noon-pageable kernel memory for this, > you are suggesting using static, > non-page-able kernel memory for the whole life of every process? What about > cases where audit is disabled. This will greatly bloat the kernel. I > have brought up cranking up the > width but the general reaction is, this is not a good idea. That is what the problem is. 16 bytes is useless for anything. -Steve > >> Example denial prior to patch (Ubuntu): > >> CALL msg=audit(1387828084.070:361): arch=c03e syscall=82 success=yes > >> exit=0 a0=4184bf a1=418547 a2=0 a3=0 items=0 ppid=1 pid=1329 > &g
Re: [PATCH 0/6][v2] audit: implement multicast socket for journald
On Tuesday, April 22, 2014 09:31:52 PM Richard Guy Briggs wrote: > This is a patch set Eric Paris and I have been working on to add a > restricted capability read-only netlink multicast socket to kernel audit to > enable userspace clients such as systemd/journald to receive audit logs, in > addition to the bidirectional auditd userspace client. Do have the ability to separate of secadm_r and sysadm_r? By allowing this, we will leak to a sysadmin that he is being audited by the security officer. In a lot of cases, they are one in the same person. But for others, they are not. I have a feeling this will cause problems for MLS systems. Also, shouldn't we have an audit event for every attempt to connect to this socket? We really need to know where this information is getting leaked to. -Steve > Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities > (but uses CAP_NET_ADMIN). The CAP_AUDIT_READ capability will be added for > use by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit > subsystem. This will remove the dependence on CAP_NET_ADMIN for the > multicast read-only socket. > > > Patches 1-3 provide a way for per-protocol bind functions to > signal an error and to be able to clean up after themselves. > > The first netfilter cleanup patch has already been accepted by a netfilter > maintainer, though I don't see it upstream yet, so it is included for > completeness. > > The second patch adds the per-protocol bind function return code to signal > to the netlink code that no further processing should be done and to undo > the work already done. > V1: This rev fixes a bug introduced by flattening the code in the last > posting. *V2: This rev moves the per-protocol bind call above the socket > exposure call and refactors out the unbind procedure. > > The third provides a way per protocol to undo bind actions on DROP. > > > Patches 4-6 implement the audit multicast socket with capability checking. > > The fourth patch adds the bind function capability check to multicast join > requests for audit. > > The fifth patch adds the audit log read multicast group. An assumption has > been made that systemd/journald reside in the initial network namespace. > This could be changed to check the actual network namespace of > systemd/journald should this assumption no longer be true since audit now > supports all network namespaces. This version of the patch now directly > sends the broadcast when the packet is ready rather than waiting until it > passes the queue. > > The sixth checks if any clients actually exist before sending. > > > Since the net tree is busier than the audit tree, conflicts are more likely > and the audit patches depend on the net patches, it is proposed to have the > net tree carry this entire patchset for 3.16. Are the net maintainers ok > with this? > > > https://bugzilla.redhat.com/show_bug.cgi?id=887992 > > First posted: > https://www.redhat.com/archives/linux-audit/2013-January/msg8.html > https://lkml.org/lkml/2013/1/27/279 > > Please find source for a test program at: > http://people.redhat.com/rbriggs/audit-multicast-listen/ > > > Richard Guy Briggs (6): > netlink: simplify nfnetlink_bind > netlink: have netlink per-protocol bind function return an error > code. > netlink: implement unbind to netlink_setsockopt > NETLINK_DROP_MEMBERSHIP > audit: add netlink audit protocol bind to check capabilities on > multicast join > audit: add netlink multicast group for log read > audit: send multicast messages only if there are listeners > > include/linux/netlink.h |3 +- > include/uapi/linux/audit.h |8 > include/uapi/linux/capability.h |7 +++- > kernel/audit.c | 64 ++-- > net/netfilter/nfnetlink.c | 10 ++--- > net/netlink/af_netlink.c| 70 > +-- net/netlink/af_netlink.h| > 6 ++- > security/selinux/include/classmap.h |2 +- > 8 files changed, 135 insertions(+), 35 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] audit: Audit proc cmdline value
On Wednesday, January 15, 2014 05:44:29 PM William Roberts wrote: > On Wed, Jan 15, 2014 at 5:33 PM, Steve Grubb wrote: > > On Wednesday, January 15, 2014 05:08:13 PM William Roberts wrote: > >> On Wed, Jan 15, 2014 at 4:54 PM, Steve Grubb wrote: > >> > On Wednesday, January 15, 2014 01:02:14 PM William Roberts wrote: > >> >> During an audit event, cache and print the value of the process's > >> >> cmdline value (proc//cmdline). This is useful in situations > >> >> where processes are started via fork'd virtual machines where the > >> >> comm field is incorrect. Often times, setting the comm field still > >> >> is insufficient as the comm width is not very wide and most > >> >> virtual machine "package names" do not fit. Also, during execution, > >> >> many threads have their comm field set as well. By tying it back to > >> >> the global cmdline value for the process, audit records will be more > >> >> complete in systems with these properties. An example of where this > >> >> is useful and applicable is in the realm of Android. With Android, > >> >> their is no fork/exec for VM instances. The bare, preloaded Dalvik > >> >> VM listens for a fork and specialize request. When this request comes > >> >> in, the VM forks, and the loads the specific application > >> >> (specializing). > >> >> This was done to take advantage of COW and to not require a load of > >> >> basic packages by the VM on very app spawn. When this spawn occurs, > >> >> the package name is set via setproctitle() and shows up in procfs. > >> >> Many of these package names are longer then 16 bytes, the historical > >> >> width of task->comm. Having the cmdline in the audit records will > >> >> couple the application back to the record directly. Also, on my > >> >> Debian development box, some audit records were more useful then > >> >> what was printed under comm. > >> >> > >> >> The cached cmdline is tied to the life-cycle of the audit_context > >> >> structure and is built on demand. > >> > > >> > I don't think its a good idea to do this for a number of reasons. > >> > 1) don't we have a record type for command line and its arguments? > >> > Shouldn't we use that auxiliary record if we do this? > >> > >> Doing this in userspace means each and every user-space would have to be > >> patched to support this. Other people from various systems have jumped in > >> adding how this would be beneficial to their cause. The data is right > >> here in the kernel. > > > > I don't mean doing it in user space, I was thinking of perhaps using the > > EXECVE auxiliary record type. It looks something like this: > > > > type=EXECVE msg=audit(1303335094.212:83): argc=2 a0="ping" > > a1="koji.fedoraproject.org" > > > > Its a record type we already have with a format that can be parsed. > > Their is no execve in my use case, so this record would never, ever > be emitted. Well, that record has mostly the same fields that you are publishing except they are formatted in a way that is usable. What I was suggesting is populate the fields and add the auxiliary record. > >> > 2) we don't want each and every syscall record to grow huge(r). > >> > Remember > >> > the command line can be virtually unlimited in length. Adding this will > >> > consume disk space and we will be able to keep less records than we > >> > currently do. > >> > >> We cap it at 128 chars in v3 patch, and then this value can be altered > >> out > >> of tree and tuned for various systems. > > > > That still adds up on systems where people really do use audit. > > Yes, but is less then setting comm width to 128, and its only > transient use, not static and un yielding in its use. I think that would be more efficient in terms of disk space usage. We don't need all the arguments for every syscall audit event. We just need the program name in full. > >> > 3) User space will now have to parse this field, too. If everything is > >> > in > >> > 1 > >> > field, how can you tell the command from its arguments considering the > >> > command name could have spaces in it. What if the arguments have spaces > >> > in them? > >> > >> How did bash figure this out to
Re: [PATCH v3 3/3] audit: Audit proc cmdline value
On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote: > >> > Try this, > >> > > >> > cp /bin/ls 'test test test' > >> > auditctll -a always,exit -F arch=b64 -S stat -k test > >> > ./test\ test\ test './test\ test\ test' > >> > auditctl -D > >> > ausearch --start recent --key test > >> > > >> >> On the event of weird chars, it gets hex escaped. > >> > > >> > and its all in 1 lump with no escaping to figure out what is what. > >> > >> Un-escape it. ausearch does this with paths. Then if you need to parse > >> it, do it. > > > > How can you? When you unescape cmdline for the example I gave, you will > > have "./test test test ./test test test". Which program ran and how many > > arguments were passed? If we are trying to improve on what comm= provides > > by having the full information, I have to be able to find out exactly > > what the program name was so it can be used for searching. If that can't > > be done, then we don't need this addition in its current form. > > In your example, you will have an execve record, with it parsed, will you > not? Only if you change your patch. > cmdline does not necessarily represent the arguments or process name. > Sometimes it does, sometimes it doesn't. Just treat the thing as one > string, perhaps do some form of substring matching in a tool. You are missing the point. The point is that you are trying to place trust in something that can be gamed. The audit system is designed such that it cannot be fooled very easily. Each piece of the subject and object are separated so that programs can be written to analyze events. What I am trying to say is now you are making something that concatenates fields with no way to regroup them later to reconstruct what really happened, > To make this clear, I am not trying to improve on what comm provides. > comm provides > 16 chars for per thread name. The key is, its per thread, and can be > anything. The > "cmdline" value, is an arbitrary spot that is a global entity for the > process. So in my change, all things coming into these events will have a > similar cmdline audit. Which may help in narrowing down on whats going on > in the system It needs to be more trustworthy than this. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] audit: Audit proc cmdline value
On Thursday, January 16, 2014 07:03:34 AM William Roberts wrote: > On Thu, Jan 16, 2014 at 6:02 AM, Steve Grubb wrote: > > On Wednesday, January 15, 2014 09:08:39 PM William Roberts wrote: > >> >> > Try this, > >> >> > > >> >> > cp /bin/ls 'test test test' > >> >> > auditctll -a always,exit -F arch=b64 -S stat -k test > >> >> > ./test\ test\ test './test\ test\ test' > >> >> > auditctl -D > >> >> > ausearch --start recent --key test > >> >> > > >> >> >> On the event of weird chars, it gets hex escaped. > >> >> > > >> >> > and its all in 1 lump with no escaping to figure out what is what. > >> >> > >> >> Un-escape it. ausearch does this with paths. Then if you need to parse > >> >> it, do it. > >> > > >> > How can you? When you unescape cmdline for the example I gave, you will > >> > have "./test test test ./test test test". Which program ran and how > >> > many > >> > arguments were passed? If we are trying to improve on what comm= > >> > provides > >> > by having the full information, I have to be able to find out exactly > >> > what the program name was so it can be used for searching. If that > >> > can't > >> > be done, then we don't need this addition in its current form. > >> > >> In your example, you will have an execve record, with it parsed, will you > >> not? > > > > Only if you change your patch. > > My patch has nothing to do with the emitting of an execve record. You > will get an > execve record with the arguments parsed out. Its not even really > "parsing" as each > element is in a NULL terminated char * array. That is what I am telling you is wrong. We can't have a string that can't be parsed later. If you reformat the output as an execve record, then we have something that is trustworthy. > >> cmdline does not necessarily represent the arguments or process name. > >> Sometimes it does, sometimes it doesn't. Just treat the thing as one > >> string, perhaps do some form of substring matching in a tool. > > > > You are missing the point. The point is that you are trying to place trust > > in something that can be gamed. The audit system is designed such that it > > cannot be fooled very easily. Each piece of the subject and object are > > separated so that programs can be written to analyze events. What I am > > trying to say is now you are making something that concatenates fields > > with no way to regroup them later to reconstruct what really happened, > > > >> To make this clear, I am not trying to improve on what comm provides. > >> comm provides > >> 16 chars for per thread name. The key is, its per thread, and can be > >> anything. The > >> "cmdline" value, is an arbitrary spot that is a global entity for the > >> process. So in my change, all things coming into these events will have a > >> similar cmdline audit. Which may help in narrowing down on whats going on > >> in the system > > > > It needs to be more trustworthy than this. > > Its as trustworthy as comm, its as trustworthy as path, etc. The audit > subsystem already prints many > untrusted values to aid in narrowing down the process, or to observe a > running processes behavior; this > is no different. Sure it is. comm is 1 entity on the value side of the name value pair. If it is detected as being special, its encoded so that it can be correctly dissected later. What you are creating cannot be correctly dissected. Please try the example I gave you and think about it a bit. Thanks, -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] audit: Simplify by assuming the callers socket buffer is large enough
On Friday, March 07, 2014 07:48:01 PM David Miller wrote: > From: Eric Paris > Date: Fri, 07 Mar 2014 17:52:02 -0500 > > > Audit is non-tolerant to failure and loss. > > Netlink is not a loss-less transport. Perhaps. But in all our testing over the years its been very good. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH for v3.14] AUDIT: Allow login in non-init namespaces
On Sunday, March 30, 2014 07:07:54 PM Eric Paris wrote:
> It its possible to configure your PAM stack to refuse login if
> audit messages (about the login) were unable to be sent. This is common
> in many distros and thus normal configuration of many containers. The
> PAM modules determine if audit is enabled/disabled in the kernel based
> on the return value from sending an audit message on the netlink socket.
> If userspace gets back ECONNREFUSED it believes audit is disabled in the
> kernel. If it gets any other error else it refuses to let the login
> proceed.
This is a requirement. I do not advocate "tricking" user space. If you do, I
might have to fix the bug you created. What should be done is have some
discussion about the problem so that everyone involved has some chance to
discuss the problem.
-Steve
> Just about ever since the introduction of namespaces the kernel audit
> subsystem has returned EPERM if the task sending a message was not in
> the init user or pid namespace. So many forms of containers have never
> worked if audit was enabled in the kernel.
>
> BUT if the container was not in net_init then the kernel network code
> would send ECONNREFUSED (instead of the audit code sending EPERM). Thus
> by pure accident/dumb luck/bug if an admin configured the PAM stack to
> reject all logins that didn't talk to audit, but then ran the login
> untility in the non-init_net namespace, it would work!! Clearly this
> was a bug, but it is a bug some people expected.
>
> With the introduction of network namespace support in 3.14-rc1 the two
> bugs stopped cancelling each other out. Now, containers in the
> non-init_net namespace refused to let users log in (just like PAM was
> configfured!) Obviously some people were not happy that what used to
> let users log in, now didn't!
>
> This fix is kinda hacky. We return ECONNREFUSED for all non-init
> relevant namespaces. That means that not only will the old broken
> non-init_net setups continue to work, now the broken non-init_pid or
> non-init_user setups will 'work'. They don't really work, since audit
> isn't logging things. But it's what most users want.
>
> In 3.15 we should have patches to support not only the non-init_net
> (3.14) namespace but also the non-init_pid and non-init_user namespace.
> So all will be right in the world. This just opens the doors wide open
> on 3.14 and hopefully makes users happy, if not the audit system...
>
> Reported-by: Andre Tomt
> Reported-by: Adam Richter
> Signed-off-by: Eric Paris
> ---
> kernel/audit.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3392d3e..95a20f3 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -608,9 +608,19 @@ static int audit_netlink_ok(struct sk_buff *skb, u16
> msg_type) int err = 0;
>
> /* Only support the initial namespaces for now. */
> + /*
> + * We return ECONNREFUSED because it tricks userspace into thinking
> + * that audit was not configured into the kernel. Lots of users
> + * configure their PAM stack (because that's what the distro does)
> + * to reject login if unable to send messages to audit. If we return
> + * ECONNREFUSED the PAM stack thinks the kernel does not have audit
> + * configured in and will let login proceed. If we return EPERM
> + * userspace will reject all logins. This should be removed when we
> + * support non init namespaces!!
> + */
> if ((current_user_ns() != &init_user_ns) ||
> (task_active_pid_ns(current) != &init_pid_ns))
> - return -EPERM;
> + return -ECONNREFUSED;
>
> switch (msg_type) {
> case AUDIT_LIST:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text
On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote: > >> - It assumes that syscall numbers are between 0 and 2048. > >> > > There could well be a bug here. Not questioning that. Although that > > would be patch 1/2 > > Even with patch 1, it still doesn't handle large syscall numbers -- it > just assumes they're not audited. All syscalls must be auditable. Meaning that if an arch goes above 2048, then we'll need to do some math to get it to fall back within the range. > >> - It's unclear whether it's supposed to be reliable. > >> > > Unclear to whom? > > To me. > > If some inode access or selinux rule triggers an audit, is the auditsc > code guaranteed to write an exit record? And see below... It should or indicate that it could not. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text
On Thursday, May 29, 2014 09:04:10 AM Andy Lutomirski wrote: > On Thu, May 29, 2014 at 6:05 AM, Steve Grubb wrote: > > On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote: > >> >> - It assumes that syscall numbers are between 0 and 2048. > >> >> > >> > There could well be a bug here. Not questioning that. Although that > >> > would be patch 1/2 > >> > >> Even with patch 1, it still doesn't handle large syscall numbers -- it > >> just assumes they're not audited. > > > > All syscalls must be auditable. Meaning that if an arch goes above 2048, > > then we'll need to do some math to get it to fall back within the range. > > Off the top of my head, x32, some ARM variants, and some MIPS variants > have syscalls above 2048. That could be fixed by putting a subtraction in place to get the bit mask to map correctly. User space audit rules would need to fix that also. > auditsc has been disabled on the offending > ARM variant (because I noticed that the same issue affects seccomp, > and no one particularly wants to fix it), but I suspect that auditsc > is enabled but completely broken on whichever MIPS ABI has the issue. > I haven't checked. > > TBH, I think that it's silly to let the auditsc design be heavily > constrained by ia64 considerations -- I still think that the syscall > entry hooks could be removed completely with some effort on most > architectures and that performance would improve considerably. For > users who don't have syscall filter rules, performance might even > improve on ia64 -- I don't care how expensive syscall_get_args is, the > actual printk/auditd thing will dominate in cases where anything is > written. The last time I heard of benchmarking being done, audit's performance hit was negligible. That was about 4 years ago and there has been a whole lot of code churn since then. But it should in general be the cost of an 'if' statement unless auditing is enabled. If it is enabled, then you necessarily get the full treatment in case you trigger an event. > I wonder whether the syscall restart mechanism can be used to > thoroughly confused auditsc. I don't really know how syscall restarts > work. > > My point is: I don't know what guarantees auditsc is supposed to > provide, nor do I know how to evaluate whether the code is correct. There is an audit test suite that is used to verify that its working. Its not an exhaustive test suite, but it provides good coverage. > For users who aren't bound by Common Criteria or related things, I > suspect that syscall auditing (as opposed to, say, LSM-based auditing) > will provide dubious value at best. No, it works pretty good. You can see who is accessing what files pretty clearly. > Keep in mind that many syscalls take pointer arguments, and the auditsc code > can't really do anything sensible with them. All security relevant information is collected in auxiliary records if they are not directly readable as a syscall argument. This is a basic requirement. We are not required to capture all possible arguments. If you know of any security relevant parameters not captured, please let us know. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Why is syscall auditing on with no rules?
On Saturday, February 01, 2014 06:51:56 PM Andy Lutomirski wrote: > On Sat, Feb 1, 2014 at 6:32 PM, Andy Lutomirski wrote: > > On a stock Fedora installation: > > > > $ sudo auditctl -l > > No rules What rules would you want? The audit package ships with several which affects performance to varying degrees. The default one affects it the least. If you don't want auditing, don't install the audit package. > > Nonetheless TIF_SYSCALL_AUDIT is set and the __audit_syscall_entry and > > __audit_syscall_exit account for >20% of syscall overhead according to > > perf. > > > > This sucks. Unless I'm missing something, syscall auditing is *off*. The audit daemon enables auditing unless you add -s=nochange to the daemon's commandline parameters. The rules are loaded by a separate process so the deamon just enables auditing so that any selinux AVCs have more information and in case auditctl loads rules. If you don't want auditing, simply don't install it and things will be OK. > > How hard would it be to arrange for TIF_SYSCALL_AUDIT to be cleared > > when there are no syscall rules? This only gets set if auditing is enabled. What if in the future someone loads rules? For example, what if you reload audit rules? The first thing that happens is it clears any previous rules. If we did what you suggested, then any process that runs between the time the rules were deleted and a rule gets loaded will never be auditable. We can't have that. Sometimes admins stop the audit daemon to do some looking around. Usually audit rules are cleared when its stopped. Once again you have a window where processes will become inauditable. We take the point of view that if you want auditing and all that it brings with it, this will be setup by the audit daemon. If you want no auditing and no performance hit, simply don't install it or disable it from starting and all will be fine. > > (This is extra bad in kernels before 3.13, where the clear call for > > TIF_SYSCALL_AUDIT was completely missing.) > > The current code seems to have really odd effects. For example, > processes that are created before the very first auditctl -e 1 (or > auditd) invocation will never be subject to syscall auditing. This is correct. Its also why we have a audit=1 boot command. Anyone needing audit must have that boot configuration or they will have inauditable processes. This is documented in the auditd man page. > But auditctl -e 1; auditctl -e 0 will cause all subsequently started > processes to have audit contexts allocated and therefore to be subject > to syscall auditing. > > I doubt that this behavior is considered desirable. What you are describing is the compromise between no performance hit and auditing. If you want it to work right, you have to set it up right. The audit=1 commandline prompt is in security docs like the NSA SNAC guide and encoded into security scanners that process STIG or USGCB content. I also believe its mentioned in Common criteria docs that any distribution that has been certified publishes. HTH... -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: Only use the syscall slowpath when syscall audit rules exist
On Monday, February 03, 2014 09:53:23 AM Andy Lutomirski wrote: > This toggles TIF_SYSCALL_AUDIT as needed when rules change instead of > leaving it set whenever rules might be set in the future. This reduces > syscall latency from >60ns to closer to 40ns on my laptop. Does this mean that we have processes that don't have the TIF_SYSCALL_AUDIT flag set? When rules get loaded, how do we get the flag put back into all processes? The theory of ops is supposed to be that for anyone not needing audit, there is only the cost of "if (tif & TIF_SYSCALL_AUDIT)". That should be it. If you have audit enabled or had it enabled (which means it might be loaded with new rules), we want to inspect the syscall. There should be a short circuit based on checking that any rules has ever been loaded or are currently loaded before doing any real collection. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/3] audit: Audit proc//cmdline aka proctitle
On Thursday, February 06, 2014 10:15:28 AM William Roberts wrote:
> During an audit event, cache and print the value of the process's
> proctitle value (proc//cmdline). This is useful in situations
> where processes are started via fork'd virtual machines where the
> comm field is incorrect. Often times, setting the comm field still
> is insufficient as the comm width is not very wide and most
> virtual machine "package names" do not fit. Also, during execution,
> many threads have their comm field set as well. By tying it back to
> the global cmdline value for the process, audit records will be more
> complete in systems with these properties. An example of where this
> is useful and applicable is in the realm of Android. With Android,
> their is no fork/exec for VM instances. The bare, preloaded Dalvik
> VM listens for a fork and specialize request. When this request comes
> in, the VM forks, and the loads the specific application (specializing).
> This was done to take advantage of COW and to not require a load of
> basic packages by the VM on very app spawn. When this spawn occurs,
> the package name is set via setproctitle() and shows up in procfs.
> Many of these package names are longer then 16 bytes, the historical
> width of task->comm. Having the cmdline in the audit records will
> couple the application back to the record directly. Also, on my
> Debian development box, some audit records were more useful then
> what was printed under comm.
>
> The cached proctitle is tied to the life-cycle of the audit_context
> structure and is built on demand.
>
> Proctitle is controllable by userspace, and thus should not be trusted.
> It is meant as an aid to assist in debugging. The proctitle event is
> emitted during syscall audits, and can be filtered with auditctl.
Ack wrt record format and contents.
-Steve
> Example:
> type=AVC msg=audit(1391217013.924:386): avc: denied { getattr } for
> pid=1971 comm="mkdir" name="/" dev="selinuxfs" ino=1
> scontext=system_u:system_r:consolekit_t:s0-s0:c0.c255
> tcontext=system_u:object_r:security_t:s0 tclass=filesystem type=SYSCALL
> msg=audit(1391217013.924:386): arch=c03e syscall=137 success=yes exit=0
> a0=7f019dfc8bd7 a1=7fffa6aed2c0 a2=fff4bd25 a3=7fffa6aed050 items=0
> ppid=1967 pid=1971 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
> sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="mkdir" exe="/bin/mkdir"
> subj=system_u:system_r:consolekit_t:s0-s0:c0.c255 key=(null)
> type=UNKNOWN[1327] msg=audit(1391217013.924:386):
> proctitle=6D6B646972002D70002F7661722F72756E2F636F6E736F6C65
>
> Signed-off-by: William Roberts
> ---
> include/uapi/linux/audit.h |1 +
> kernel/audit.h |6
> kernel/auditsc.c | 67
> 3 files changed, 74
> insertions(+)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 2d48fe1..4315ee9 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -109,6 +109,7 @@
> #define AUDIT_NETFILTER_PKT 1324/* Packets traversing netfilter chains
*/
> #define AUDIT_NETFILTER_CFG 1325/* Netfilter chain modifications */
> #define AUDIT_SECCOMP1326/* Secure Computing event */
> +#define AUDIT_PROCTITLE 1327/* Proctitle emit event */
>
> #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 57cc64d..38c967d 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -106,6 +106,11 @@ struct audit_names {
> boolshould_free;
> };
>
> +struct audit_proctitle {
> + int len;/* length of the cmdline field. */
> + char*value; /* the cmdline field */
> +};
> +
> /* The per-task audit context. */
> struct audit_context {
> int dummy; /* must be the first element */
> @@ -202,6 +207,7 @@ struct audit_context {
> } execve;
> };
> int fds[2];
> + struct audit_proctitle proctitle;
>
> #if AUDIT_DEBUG
> int put_count;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 10176cd..e342eb0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -68,6 +68,7 @@
> #include
> #include
> #include
> +#include
>
> #include "audit.h"
>
> @@ -79,6 +80,9 @@
> /* no execve audit message should be longer than this (userspace limits) */
> #define MAX_EXECVE_AUDIT_LEN 7500
>
> +/* max length to print of cmdline/proctitle value during audit */
> +#define MAX_PROCTITLE_AUDIT_LEN 128
> +
> /* number of audit rules */
> int audit_n_rules;
>
> @@ -842,6 +846,13 @@ static inline struct audit_context
> *audit_get_context(struct task_struct *tsk, return context;
> }
>
> +static inline void audit_proctitle_free(struct audit_context *context)
> +{
> + kfree(context->proctitle.value);
Re: [RFC][PATCH] audit: log join and part events to the read-only multicast log socket
On Tuesday, October 07, 2014 03:03:14 PM Eric Paris wrote:
> On Tue, 2014-10-07 at 14:23 -0400, Richard Guy Briggs wrote:
> > Log the event when a client attempts to connect to the netlink audit
> > multicast socket, requiring CAP_AUDIT_READ capability, binding to the
> > AUDIT_NLGRP_READLOG group. Log the disconnect too.
> >
> >
> >
> > Sample output:
> > time->Tue Oct 7 14:15:19 2014
> > type=UNKNOWN[1348] msg=audit(1412705719.316:117): auid=0 uid=0 gid=0 ses=1
> > pid=3552 comm="audit-multicast"
> > exe="/home/rgb/rgb/git/audit-multicast-listen/audit-multicast-listen"
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 group=0
> > op=connect res=1>
> >
> >
> > Signed-off-by: Richard Guy Briggs
> > ---
> > For some reason unbind isn't being called on disconnect. I suspect
> > missing
> > plumbing in netlink. Investigation needed...
> >
> >
> > include/uapi/linux/audit.h |1 +
> > kernel/audit.c | 46
> >++- 2 files changed, 45
> >insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 4d100c8..7fa6e8f 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -110,6 +110,7 @@
> >
> > #define AUDIT_SECCOMP1326/* Secure Computing event */
> > #define AUDIT_PROCTITLE 1327/* Proctitle emit event */
> > #define AUDIT_FEATURE_CHANGE 1328/* audit log listing feature changes
> >*/>
> > +#define AUDIT_EVENT_LISTENER 1348/* task joined multicast read socket
> > */>
> >
> > #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> > #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 53bb39b..74c81a7 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1108,13 +1108,54 @@ static void audit_receive(struct sk_buff *skb)
> >
> > mutex_unlock(&audit_cmd_mutex);
> > }
> >
> >
> > +static void audit_log_bind(int group, char *op, int err)
> > +{
> > + struct audit_buffer *ab;
> > + char comm[sizeof(current->comm)];
> > + struct mm_struct *mm = current->mm;
> > +
> > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> > + if (!ab)
> > + return;
> > +
> > + audit_log_format(ab, "auid=%d",
> > + from_kuid(&init_user_ns,
> > audit_get_loginuid(current))); + audit_log_format(ab, " uid=%d",
> > + from_kuid(&init_user_ns, current_uid()));
> > + audit_log_format(ab, " gid=%d",
> > + from_kgid(&init_user_ns, current_gid()));
> > + audit_log_format(ab, " ses=%d", audit_get_sessionid(current));
> > + audit_log_format(ab, " pid=%d", task_pid_nr(current));
> > + audit_log_format(ab, " comm=");
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > + if (mm) {
> > + down_read(&mm->mmap_sem);
> > + if (mm->exe_file)
> > + audit_log_d_path(ab, " exe=",
> > &mm->exe_file->f_path);
> > + up_read(&mm->mmap_sem);
> > + } else
> > + audit_log_format(ab, " exe=(null)");
> > + audit_log_task_context(ab); /* subj= */
>
> super crazy yuck. audit_log_task_info() ??
audit_log_task_info logs too much information for typical use. There are times
when you might want to know everything about what's connecting. But in this
case, we don't need anything about groups, saved uids, fsuid, or ppid.
Its a shame we don't have a audit_log_task_info_light function which only
records:
pid= auid= uid= subj= comm= exe= ses= tty=
> > + audit_log_format(ab, " group=%d", group);
>
> group seems like too easily confused a name.
nlnk-grp is better if its what I think it is.
-Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] audit: log join and part events to the read-only multicast log socket
On Tuesday, October 21, 2014 05:08:22 PM Richard Guy Briggs wrote: > On 14/10/21, Steve Grubb wrote: > > > super crazy yuck. audit_log_task_info() ?? > > > > audit_log_task_info logs too much information for typical use. There are > > times when you might want to know everything about what's connecting. But > > in this case, we don't need anything about groups, saved uids, fsuid, or > > ppid. > > > > Its a shame we don't have a audit_log_task_info_light function which only > > records: > > > > pid= auid= uid= subj= comm= exe= ses= tty= > > We already have audit_log_task() which gives: > auid= > uid= > gid= > ses= > subj= > pid= > comm= > exe= > This is missing tty=, but has gid=. Can we please use that function > instead and add tty=? gid is important for things that might allow access by file permissions. But the syscall logging is going to have that and much more. In this case, access is granted by having a posix capability. So, all we want is what's the process, who's the user, which session/tty is this coming from to find all events that might be related. > And while we are at it, refactor audit_log_task_info() to call > audit_log_task()? That will cause problems at this point. > Is this standard set above what should be used for certain classes of > log messages? Its hard to say if that is sufficient for all cases. When access is granted by posix capability, sure. This is probably good for most kernel originating events. But there are times extra info is needed. > Yes, it will be in a different order because we don't have a canonical > order yet. Can we accept two orders of keywords so we can start > canonicalizing, please? I don't understand what you are getting at. -Steve > > > > + audit_log_format(ab, " group=%d", group); > > > > > > group seems like too easily confused a name. > > > > nlnk-grp is better if its what I think it is. > > Where did you find that name? That could work and it is shorter, but it > seems awkwardly optimized. "nlnk" doesn't appear once in the kernel. > "nl" is already recognized for netlink, "mcgrp" is already used for > "multicast group(s)", so I would suggest "nl-mcgrp". > > > -Steve > > - RGB > > -- > Richard Guy Briggs > Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, > Red Hat Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 0/5] audit by executable name
On Tuesday, October 21, 2014 05:56:36 PM Paul Moore wrote: > On Monday, October 20, 2014 07:33:39 PM Steve Grubb wrote: > > On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote: > > > On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote: > > > > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote: > > > > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote: > > > > > > This is a part of Peter Moody, my and Eric Paris' work to > > > > > > implement > > > > > > audit by executable name. > > > > > > > > > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set > > > > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel > > > > > supports it when issuing commands. Also, if its conceivable that > > > > > kernels > > > > > may pick and choose what features could be backported to a curated > > > > > kernel, should AUDIT_VERSION_ be a number that is incremented or a > > > > > bit > > > > > mask? > > > > > > > > Right now the value is 2. So this is your last hope if you want to > > > > make > > > > it a bitmask. I'll leave that up to paul/richard to (over) design. > > > > > > Audit is nothing if not over-designed. I want to make sure we're > > > consistent with the previous design methodologies ;) > > > > > > I've been thinking about this for about the past half-hour while I've > > > been > > > going through some other mail and I'm not really enthused about using > > > the > > > version number to encode capabilities. What sort of problems would we > > > have if we introduced a new audit netlink command to query the kernel > > > for > > > audit capabilities? > > > > I thought that is what we were getting in this patch: > > https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html > > That patch, and the simple name "version", looks more like a version number > and not a capabilities bitmap. However, as Eric previously pointed out, > since we are only at version 2, all is not lost. > > > As I understood it, I send an AUDIT_GET command on netlink and then look > > in > > status.version to see what we have. I really think that in the mainline > > kernel, there will be a steady increment of capabilities. However, for > > distributions, they may want to pick and choose which capabilities to > > backport to their shipping kernel. Meaning in practice, a bitmap may be > > better to allow cherry picking capabilities and user space being able to > > make informed decisions. > > If we are intending to use this as a way of checking for functionality then > it really should be a bitmap and not a version number. Regardless of if we > are talking about an upstream or distribution kernel. > > > I really don't mind if this is done by a new netlink command (but if we > > do, > > what happens to status.version?) or if we just keep going with > > status.version. Just tell me which it is. > > No, let's stick with what we have now. I mistakenly assumed that since the > struct field and userspace #defines included "version" that the value was > actually a version number ... silly me, I have no idea why I thought that. > > So, with this in mind, I think a couple of small tweaks are in order (sorry > Richard), in no particular order: > > * Change the audit_status.version field comment in > include/uapi/linux/audit.h to "/* audit functionality bitmap */", or > similar. We can't really change the structure now, but the comment is fair > game. > > * Change AUDIT_VERSION_LATEST to a bitmask instead of a number. For > example, it should be 3 given the current code, not 2. In a perfect world > this wouldn't even be in the uapi header, but it is so we need to keep it > updated. Bumping it higher should be backwards compatible. > > Can anyone think of anything else that might be affected by this? This plan sounds good to me. Thanks, -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values
On Wednesday, August 05, 2015 03:16:58 PM Paul Moore wrote: > On Wednesday, August 05, 2015 02:30:14 AM Richard Guy Briggs wrote: > > On 15/08/04, Paul Moore wrote: > > > On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote: > > > > Signed-off-by: Richard Guy Briggs > > > > --- > > > > > > > > include/uapi/linux/audit.h |2 ++ > > > > kernel/audit.c |2 +- > > > > kernel/audit_watch.c |8 > > > > kernel/auditsc.c |6 +++--- > > > > 4 files changed, 10 insertions(+), 8 deletions(-) > > > > > > Yipee, less magic numbers! > > > > > > However, one question for you ... are we ever going to see a device or > > > inode set to -1 in the userspace facing API? In other words, should the > > > new #defines go in the uapi headers or simply in kernel/audit.h? Unless > > > it is part of the API, let's leave it out of uapi as we have to be very > > > careful about that stuff and I'd prefer to keep it minimal. > > > > This is a good point. I did briefly thing about this at one point. > > Perhaps Steve can answer this. It would be trivial to move it back to > > uapi if needed. Would you be ok with it in include/linux/audit.h for > > now? > > I have no problem with it in include/linux/audit.h, that is a kernel-only > include that we can change at anytime. My concern is putting it into a uapi > header which makes it very hard to change. > > I'm thinking we should just go ahead and put it in include/linux/audit.h for > now as I can't think of a reason why userspace should be passing in an > invalid dev/inode value, it just doesn't make sense. If the invalid tokens > prove to be valuable for userspace, we can always move the #defines. I can't imagine anyone auditing against a specific device or inode. Its like auditing a pid when you really want the program name. Its much more useful to audit by filename or directory and not inode/device. So, do whatever you want. The only unset value that people actually use is the auid because deamons have it unset. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V9 3/3] audit: add audit by children of executable path
On Thursday, August 06, 2015 04:24:58 PM Paul Moore wrote: > On Wednesday, August 05, 2015 04:29:38 PM Richard Guy Briggs wrote: > > This adds the ability to audit the actions of children of a > > not-yet-running > > process. > > > > > > > > This is a split-out of a heavily modified version of a patch originally > > submitted by Eric Paris with some ideas from Peter Moody. > > > > > > > > Cc: Peter Moody > > Cc: Eric Paris > > Signed-off-by: Richard Guy Briggs > > --- > > > > include/uapi/linux/audit.h |1 + > > kernel/auditfilter.c |5 + > > kernel/auditsc.c | 11 +++ > > 3 files changed, 17 insertions(+), 0 deletions(-) > > I'm still not really comfortable with that loop and since there hasn't been > a really convincing use case I'm going to pass on this patch for right > now. If someone comes up with a *really* compelling case in the future > I'll reconsider it. Its the same reason strace has a -f option. Sometimes you need to also see what the children did. For example, maybe you want to audit file access to a specific directory and several cgi-bin programs can get there. You could write a rule for apache and be done. Or maybe, you have an app that lets people have shell access and you need to see files accessed or connections opened. Or maybe its a control panel application with helper scripts and you need to see changes that its making. Or maybe you have a program that is at risk of being compromised and you want to see if someone gets a shell from it. There are a lot of cases where it could be useful. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd
Hello Richard,
Public reply this time. :-)
On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving out
> the old auditd since audit_pid no longer points to the old valid auditd.
I guess the first question is why do we allow something to start up a new
auditd without killing off the old one? Would that be a simpler fix?
> If no message to auditd has been attempted since auditd died unnaturally
> or got killed, audit_pid will still indicate it is alive. There isn't
> an easy way to detect if an old auditd is still running on the existing
> audit_pid other than attempting to send a message to see if it fails.
> An -ECONNREFUSED almost certainly means it disappeared and can be
> replaced. Other errors are not so straightforward and may indicate
> transient problems that will resolve themselves and the old auditd will
> recover. Yet others will likely need manual intervention for which a
> new auditd will not solve the problem.
>
> Send a new message type (AUDIT_REPLACE) to the old auditd containing a
> u32 with the PID of the new auditd. If the audit replace message
> succeeds (or doesn't fail with certainty), fail to register the new
> auditd and return an error (-EEXIST).
>
> This is expected to make the patch preventing an old auditd orphaning a
> new auditd redundant.
>
> V2: Rename audit_ping to audit_replace, set seq and portid to 0 in
> the call to audit_make_reply().
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/uapi/linux/audit.h |1 +
> kernel/audit.c | 16 +++-
> 2 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 843540c..cf84991 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -70,6 +70,7 @@
> #define AUDIT_TTY_SET1017/* Set TTY auditing status */
> #define AUDIT_SET_FEATURE1018/* Turn an audit feature on or off */
> #define AUDIT_GET_FEATURE1019/* Get which features are enabled */
> +#define AUDIT_REPLACE1020/* Replace auditd if this
> packet
unanswerd */
In every case, events in the 1000 block are to configure the kernel or to ask
the kernel a question. This is user space initiating. Kernel initiating events
are in the 1300 block of numbers.
-Steve
> #define AUDIT_FIRST_USER_MSG 1100/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC 1107/* We
> filter
this
> differently */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 36989a1..0368be2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -809,6 +809,16 @@ static int audit_set_feature(struct sk_buff *skb)
> return 0;
> }
>
> +static int audit_replace(pid_t pid)
> +{
> + struct sk_buff *skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0,
> +&pid, sizeof(pid));
> +
> + if (!skb)
> + return -ENOMEM;
> + return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> +}
> +
> static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> {
> u32 seq;
> @@ -870,9 +880,13 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) }
> if (s.mask & AUDIT_STATUS_PID) {
> int new_pid = s.pid;
> + pid_t requesting_pid = task_tgid_vnr(current);
>
> - if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> + if ((!new_pid) && (requesting_pid != audit_pid))
> return -EACCES;
> + if (audit_pid && new_pid &&
> + audit_replace(requesting_pid) != -ECONNREFUSED)
> + return -EEXIST;
> if (audit_enabled != AUDIT_OFF)
> audit_log_config_change("audit_pid", new_pid,
> audit_pid,
1);
> audit_pid = new_pid;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: convert status version to a feature bitmap
On Thursday, November 13, 2014 08:08:52 PM Richard Guy Briggs wrote: > > So what terrible things happen to userspace if > > AUDIT_VERSION_BACKLOG_WAIT_TIME becomes 0x03 instead of 0x02? > > But it won't. It gets the value of > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x0002. > > I think you meant to ask about AUDIT_VERSION_LATEST, which would become 3. > > You *did* already ask that question in a previous thread, and there > didn't seem to be a concern. Steve Grubb could likely answer this > question better than me. The audit 2.4.1 package has been pushed to everything from F20 -> rawhide. If you don't see any problems, then its safe. But check carefully around the things that you did change. Right now, we only are caring about only one kernel feature, --loginuid-immutable. Check that it still works, auditctl -s. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 0/5] audit by executable name
On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote: > This is a part of Peter Moody, my and Eric Paris' work to implement > audit by executable name. Does this patch set define an AUDIT_VERSION_SOMETHING and then set AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel supports it when issuing commands. Also, if its conceivable that kernels may pick and choose what features could be backported to a curated kernel, should AUDIT_VERSION_ be a number that is incremented or a bit mask? -Steve > Please see the accompanying userspace patch: > https://www.redhat.com/archives/linux-audit/2014-May/msg00019.html > The userspace interface is not expected to change appreciably unless > something important has been overlooked. Setting and deleting rules works > as expected. > > If the path does not exist at rule creation time, it will be re-evaluated > every time there is a change to the parent directory at which point the > change in device and inode will be noted. > > > Here's a sample run: > > # /usr/local/sbin/auditctl -a always,exit -F dir=/tmp -F exe=/bin/touch -F > key=touch_tmp # /usr/local/sbin/ausearch --start recent -k touch_tmp > time->Mon Jun 30 14:15:06 2014 > type=CONFIG_CHANGE msg=audit(1404152106.683:149): auid=0 ses=1 > subj=unconfined_u :unconfined_r:auditctl_t:s0-s0:c0.c1023 op="add_rule" > key="touch_tmp" list=4 res =1 > > # /usr/local/sbin/auditctl -l > -a always,exit -S all -F dir=/tmp -F exe=/bin/touch -F key=touch_tmp > > # touch /tmp/test > > # /usr/local/sbin/ausearch --start recent -k touch_tmp > time->Wed Jul 2 12:18:47 2014 > type=UNKNOWN[1327] msg=audit(1404317927.319:132): > proctitle=746F756368002F746D702F74657374 type=PATH > msg=audit(1404317927.319:132): item=1 name="/tmp/test" inode=25997 > dev=00:20 mode=0100644 ouid=0 ogid=0 rdev=00:00 > obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE type=PATH > msg=audit(1404317927.319:132): item=0 name="/tmp/" inode=11144 dev=00:20 > mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 > nametype=PARENT type=CWD msg=audit(1404317927.319:132): cwd="/root" > type=SYSCALL msg=audit(1404317927.319:132): arch=c03e syscall=2 > success=yes exit=3 a0=7a403dd5 a1=941 a2=1b6 a3=34b65b2c6c items=2 > ppid=4321 pid=6436 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 > fsgid=0 tty=ttyS0 ses=1 comm="touch" exe="/usr/bin/touch" > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="touch_tmp" > > > Revision history: > v5: Revert patch "Let audit_free_rule() take care of calling > audit_remove_mark()." since it caused a group mark deadlock. > > v4: Re-order and squash down fixups > Fix audit_dup_exe() to copy pathname string before calling > audit_alloc_mark(). > > v3: Rationalize and rename some function names and clean up get/put and free > code. Rename several "watch" references to "mark". > Rename audit_remove_rule() to audit_remove_mark_rule(). > Let audit_free_rule() take care of calling audit_remove_mark(). > Put audit_alloc_mark() arguments in same order as watch, tree and inode. > Move the access to the entry for audit_match_signal() to the beginning of > the function in case the entry found is the same one passed in. This will > enable it to be used by audit_remove_mark_rule(). > https://www.redhat.com/archives/linux-audit/2014-July/msg0.html > > v2: Misguided attempt to add in audit_exe similar to watches > https://www.redhat.com/archives/linux-audit/2014-June/msg00066.html > > v1.5: eparis' switch to fsnotify > https://www.redhat.com/archives/linux-audit/2014-May/msg00046.html > https://www.redhat.com/archives/linux-audit/2014-May/msg00066.html > > v1: Change to path interface instead of inode > https://www.redhat.com/archives/linux-audit/2014-May/msg00017.html > > v0: Peter Moodie's original patches > https://www.redhat.com/archives/linux-audit/2012-August/msg00033.html > > > Next step: > Get full-path notify working. > > > Eric Paris (3): > audit: implement audit by executable > audit: clean simple fsnotify implementation > audit: convert audit_exe to audit_fsnotify > > Richard Guy Briggs (2): > audit: avoid double copying the audit_exe path string > Revert "fixup! audit: clean simple fsnotify implementation" > > include/linux/audit.h |1 + > include/uapi/linux/audit.h |2 + > kernel/Makefile|2 +- > kernel/audit.h | 39 +++ > kernel/audit_exe.c | 49 + > kernel/audit_fsnotify.c| 237 > kernel/auditfilter.c | > 52 +- > kernel/auditsc.c | 16 +++ > 8 files changed, 395 insertions(+), 3 deletions(-) > create mode 100644 kernel/audit_exe.c > create mode 100644 kernel/audit_fsnotify.c -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH V5 0/5] audit by executable name
On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote: > On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote: > > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote: > > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote: > > > > This is a part of Peter Moody, my and Eric Paris' work to implement > > > > audit by executable name. > > > > > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set > > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel > > > supports it when issuing commands. Also, if its conceivable that kernels > > > may pick and choose what features could be backported to a curated > > > kernel, should AUDIT_VERSION_ be a number that is incremented or a bit > > > mask? > > > > Right now the value is 2. So this is your last hope if you want to make > > it a bitmask. I'll leave that up to paul/richard to (over) design. > > Audit is nothing if not over-designed. I want to make sure we're consistent > with the previous design methodologies ;) > > I've been thinking about this for about the past half-hour while I've been > going through some other mail and I'm not really enthused about using the > version number to encode capabilities. What sort of problems would we have > if we introduced a new audit netlink command to query the kernel for audit > capabilities? I thought that is what we were getting in this patch: https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html As I understood it, I send an AUDIT_GET command on netlink and then look in status.version to see what we have. I really think that in the mainline kernel, there will be a steady increment of capabilities. However, for distributions, they may want to pick and choose which capabilities to backport to their shipping kernel. Meaning in practice, a bitmap may be better to allow cherry picking capabilities and user space being able to make informed decisions. I really don't mind if this is done by a new netlink command (but if we do, what happens to status.version?) or if we just keep going with status.version. Just tell me which it is. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V5 0/5] audit by executable name
On Monday, October 20, 2014 07:33:39 PM Steve Grubb wrote: > On Monday, October 20, 2014 07:02:33 PM Paul Moore wrote: > > On Monday, October 20, 2014 06:47:27 PM Eric Paris wrote: > > > On Mon, 2014-10-20 at 16:25 -0400, Steve Grubb wrote: > > > > On Thursday, October 02, 2014 11:06:51 PM Richard Guy Briggs wrote: > > > > > This is a part of Peter Moody, my and Eric Paris' work to implement > > > > > audit by executable name. > > > > > > > > Does this patch set define an AUDIT_VERSION_SOMETHING and then set > > > > AUDIT_VERSION_LATEST to it? If not, I need one to tell if the kernel > > > > supports it when issuing commands. Also, if its conceivable that > > > > kernels > > > > may pick and choose what features could be backported to a curated > > > > kernel, should AUDIT_VERSION_ be a number that is incremented or a bit > > > > mask? > > > > > > Right now the value is 2. So this is your last hope if you want to make > > > it a bitmask. I'll leave that up to paul/richard to (over) design. > > > > Audit is nothing if not over-designed. I want to make sure we're > > consistent with the previous design methodologies ;) > > > > I've been thinking about this for about the past half-hour while I've been > > going through some other mail and I'm not really enthused about using the > > version number to encode capabilities. What sort of problems would we > > have > > if we introduced a new audit netlink command to query the kernel for audit > > capabilities? > > I thought that is what we were getting in this patch: > https://www.redhat.com/archives/linux-audit/2014-January/msg00054.html > > As I understood it, I send an AUDIT_GET command on netlink and then look in > status.version to see what we have. I really think that in the mainline > kernel, there will be a steady increment of capabilities. However, for > distributions, they may want to pick and choose which capabilities to > backport to their shipping kernel. Meaning in practice, a bitmap may be > better to allow cherry picking capabilities and user space being able to > make informed decisions. > > I really don't mind if this is done by a new netlink command (but if we do, > what happens to status.version?) or if we just keep going with > status.version. Just tell me which it is. Further to the point of status.version, its declared as a __u32. So if it were a bit map, we can have 32 different features userspace needs to make support decisions on. I have a feeling that will last many years because I really can't see audit gaining too many more capabilities. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH] audit: log join and part events to the read-only multicast log socket
On Tue, 07 Oct 2014 18:06:51 -0400 Paul Moore wrote: > On Tuesday, October 07, 2014 03:39:51 PM Richard Guy Briggs wrote: > > I also thought of moving audit_log_task() from auditsc.c to audit.c > > and using that. For that matter, both audit_log_task() and > > audit_log_task_info() could use audit_log_session_info(), but they > > are in slightly different order of keywords which will upset > > sgrubb's parser. > > A bit of an aside from the patch, but in my opinion the parser should > be made a bit more robust so that it can handle fields in any > particular order. I agree that having fields in a "canonical > ordering" is helpful, both for tools and people, but the tools > shouldn't require it in my opinion. > > Steve, why exactly can't the userspace parser handle fields in any > order? How difficult would it be to fix? The issue is that people that really use audit, really get vast quanities of logs. The tools expect things in a specific order so that it can pick things out of events as quickly as possible. IOW, it knows when it can discard the line because its grabbed everything it needs. A casual audit user would never see this. I'm really optimizing for the people whose use ausearch and it takes 10 minutes to run. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Tuesday, May 12, 2015 03:57:59 PM Richard Guy Briggs wrote: > On 15/05/05, Steve Grubb wrote: > > I think there needs to be some more discussion around this. It seems like > > this is not exactly recording things that are useful for audit. > > It seems to me that either audit has to assemble that information, or > the kernel has to do so. The kernel doesn't know about containers > (yet?). Auditing is something that has a lot of requirements imposed on it by security standards. There was no requirement to have an auid until audit came along and said that uid is not good enough to know who is issuing commands because of su or sudo. There was no requirement for sessionid until we had to track each action back to a login so we could see if the login came from the expected place. What I am saying is we have the same situation. Audit needs to track a container and we need an ID. The information that is being logged is not useful for auditing. Maybe someone wants that info in syslog, but I doubt it. The audit trail's purpose is to allow a security officer to reconstruct the events to determine what happened during some security incident. What they would want to know is what resources were assigned; if two containers shared a resource, what resource and container was it shared with; if two containers can communicate, we need to see or control information flow when necessary; and we need to see termination and release of resources. Also, if the host OS cannot make sense of the information being logged because the pid maps to another process name, or a uid maps to another user, or a file access maps to something not in the host's, then we need the container to do its own auditing and resolve these mappings and optionally pass these to an aggregation server. Nothing else makes sense. > > On Friday, April 17, 2015 03:35:52 AM Richard Guy Briggs wrote: > > > Log the creation and deletion of namespace instances in all 6 types of > > > namespaces. > > > > > > Twelve new audit message types have been introduced: > > > AUDIT_NS_INIT_MNT 1330/* Record mount namespace instance > > > creation > > > */ AUDIT_NS_INIT_UTS 1331/* Record UTS namespace instance > > > creation */ AUDIT_NS_INIT_IPC 1332/* Record IPC namespace > > > instance creation */ AUDIT_NS_INIT_USER 1333/* Record USER > > > namespace instance creation */ AUDIT_NS_INIT_PID 1334/* Record > > > PID namespace instance creation */ AUDIT_NS_INIT_NET 1335/* > > > Record NET namespace instance creation */ AUDIT_NS_DEL_MNT1336 > > > /* Record mount namespace instance deletion */ AUDIT_NS_DEL_UTS > > > 1337 > > > > > >/* Record UTS namespace instance deletion */ AUDIT_NS_DEL_IPC > > > > > > 1338/* Record IPC namespace instance deletion */ AUDIT_NS_DEL_USER > > > > > > 1339/* Record USER namespace instance deletion */ AUDIT_NS_DEL_PID > > > > > >1340/* Record PID namespace instance deletion */ AUDIT_NS_DEL_NET > > > > > > 1341/* Record NET namespace instance deletion */ > > > > The requirements for auditing of containers should be derived from VPP. In > > it, it asks for selectable auditing, selective audit, and selective audit > > review. What this means is that we need the container and all its > > children to have one identifier that is inserted into all the events that > > are associated with the container. > > Is that requirement for the records that are sent from the kernel, or > for the records stored by auditd, or by another facility that delivers > those records to a final consumer? A little of both. Selective audit means that you can set rules to include or exclude an event. This is done in the kernel. Selectable review means that the user space tools need to be able to skip past records not of interest to a specific line of inquiry. Also, logging everything and letting user space work it out later is also not a solution because the needle is harder to find in a larger haystack. Or, the logs may rotate and its gone forever because the partition is filled. > > With this, its possible to do a search for all events related to a > > container. Its possible to exclude events from a container. Its possible > > to not get any events. > > > > The requirements also call out for the identification of the subject. This > > means that the event should be bound to a syscall such as clone, setns, or > > unshare. > > Is it useful to have a reference of the init namespace set from which > all others are spawned? For things directly observable by the init name space,
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Thursday, May 14, 2015 10:42:38 AM Eric W. Biederman wrote: > Steve Grubb writes: > > On Tuesday, May 12, 2015 03:57:59 PM Richard Guy Briggs wrote: > >> On 15/05/05, Steve Grubb wrote: > >> > I think there needs to be some more discussion around this. It seems > >> > like > >> > this is not exactly recording things that are useful for audit. > >> > >> It seems to me that either audit has to assemble that information, or > >> the kernel has to do so. The kernel doesn't know about containers > >> (yet?). > > > > Auditing is something that has a lot of requirements imposed on it by > > security standards. There was no requirement to have an auid until audit > > came along and said that uid is not good enough to know who is issuing > > commands because of su or sudo. There was no requirement for sessionid > > until we had to track each action back to a login so we could see if the > > login came from the expected place. > > Stop right there. > > You want a global identifier in a realm where only relative identifiers > exist, and make sense. Global to a name space for me is I guess relative for you. The ID is needed to tie events together to check for violations of the security policy of the container/namespace invoking child container/namespace. As a concrete example, suppose a container is to have its own /etc/shadow. If for some reason the container used the host's copy, then that would point to a misconfiguration or perhaps indicate an escape from the container. I would imagine that the next layer down has its own set of global identifiers so that it can verify enforcement of its own security assumptions. This does not need to be global to the system from top to 9 layers down. Each layer needs to have a way of locating events common to a child container instance. > I am sorry that isn't going to happen. EVER. Then I'd suggest we either scrap this set of patches and forget auditing of containers. (This would have the effect of disallowing them in a lot of environments because violations of security policy can't be detected.) Or someone please explain how what is proposed to be logged allows the tying together of events. Or even supports the requirements I stated in my last email. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Thursday, May 14, 2015 11:23:09 PM Andy Lutomirski wrote: > On Thu, May 14, 2015 at 7:32 PM, Richard Guy Briggs wrote: > > On 15/05/14, Paul Moore wrote: > >> * Look at our existing audit records to determine which records should > >> have > >> namespace and container ID tokens added. We may only want to add the > >> additional fields in the case where the namespace/container ID tokens are > >> not the init namespace. > > > > If we have a record that ties a set of namespace IDs with a container > > ID, then I expect we only need to list the containerID along with auid > > and sessionID. > > The problem here is that the kernel has no concept of a "container", and I > don't think it makes any sense to add one just for audit. "Container" is a > marketing term used by some userspace tools. No, its a real thing just like a login. Does the kernel have any concept of a login? Yet it happens. And it causes us to generate events describing who, where from, role, success, and time of day. :-) > I can imagine that both audit could benefit from a concept of a > namespace *path* that understands nesting (e.g. root/2/5/1 or > something along those lines). Mapping these to "containers" belongs > in userspace, I think. I don't doubt that just as user space sequences the actions that are a login. I just need the kernel to do some book keeping and associate the necessary attributes in the event record to be able to reconstruct what is actually happening. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Thursday, May 14, 2015 08:31:45 PM Eric W. Biederman wrote: > Paul Moore writes: > > As Eric, and others, have stated, the container concept is a userspace > > idea, not a kernel idea; the kernel only knows, and cares about, > > namespaces. This is unlikely to change. > > > > However, as Steve points out, there is precedence for the kernel to record > > userspace tokens for the sake of audit. Personally I'm not a big fan of > > this in general, but I do recognize that it does satisfy a legitimate > > need. Think of things like auid and the sessionid as necessary evils; > > audit is already chock full of evilness I doubt one more will doom us all > > to hell. > > > > Moving forward, I'd like to see the following: > > > > * Create a container ID token (unsigned 32-bit integer?), similar to > > auid/sessionid, that is set by userspace and carried by the kernel to be > > used in audit records. I'd like to see some discussion on how we manage > > this, e.g. how do handle container ID inheritance, how do we handle > > nested containers (setting the containerid when it is already set), do we > > care if multiple different containers share the same namespace config, > > etc.? > > > > Can we all live with this? If not, please suggest some alternate ideas; > > simply shouting "IT'S ALL CRAP!" isn't helpful for anyone ... it may be > > true, but it doesn't help us solve the problem ;) > > Without stopping and defining what someone means by container I think it > is pretty much nonsense. Maybe this is what's hanging everyone up? Its easy to get lost when your view is down at the syscall level and what is happening in the kernel. Starting a container is akin to the idea of login. Not every call to setresuid is a login. It could be a setuid program starting or a daemon dropping privileges. The idea of a container is a higher level concept that starting a name space. I think comparing a login with a container is a useful analogy because both are higher level concepts but employ low level ideas. A login is a collection of chdir, setuid, setgid, allocating a tty, associating the first 3 file descriptors, setting a process group, and starting a specific executable. All these low level concepts each by itself is not special. A container is what we need auditing events around not creation of namespaces. If we want creation of namespaces, we can audit the clone/unshare/setns syscalls. The container is when a managing program such as docker, lxc, or sometimes systemd creates a special operating environment for the express purpose of running programs disassociated in some way from the parent namespaces, cgroups, and security assumptions. Its this orchestration, just as sshd orchestrates a login, that makes it different. > Should every vsftp connection get a container every? Every chrome tab? No. Also, note that not every program that grants a user session constitutes a login. > At some of the connections per second numbers I have seen we might > exhaust a 32bit number in an hour or two. Will any of that make sense > to someone reading the audit logs? I would agree if we were auditing creation of name spaces. But going back to the concept of login, these could occur at a high rate. This is a bruteforce login attack. We put countermeasures in place to prevent it. But it is possible for the session id to wrap. But in our case, things like lxc or docker don't start hundreds of these a minute. > Without considerning that container creation is an unprivileged > operation I think it is pretty much nonsense. Do I get to say I am any > container I want? That would seem to invalidate the concept of > userspace setting a container id. It would need to be a privileged operation just as setuid is. > How does any of this interact with setns? AKA entering a container? We have to audit this. For the moment, auditing the setns syscall may be enough. I'd have to look at the lifecycle of the application that's doing this to determine if we need more. > I will go as far as looking at patches. If someone comes up with > a mission statement about what they are actually trying to achieve and a > mechanism that actually achieves that, and that allows for containers to > nest we can talk about doing something like that. Auditing wouldn't impose any restrictions on this. We just need a way to observe actions within and associate them as needed to investigate violations of security policy. > But for right now I just hear proposals for things that make no sense > and can not possibly work. Not least because it will require modifying > every program that creates a container and who knows how many of them > there are. We only care about a couple programs doing the orchestration. They will need to have the right support added to them. I'm hoping the analogy of a login helps demonstrate what we are after. -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in th
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Tuesday, May 05, 2015 09:56:03 AM Eric W. Biederman wrote: > Steve Grubb writes: > > The requirements for auditing of containers should be derived from VPP. In > > it, it asks for selectable auditing, selective audit, and selective audit > > review. What this means is that we need the container and all its > > children to have one identifier that is inserted into all the events that > > are associated with the container. > > That is technically impossible. Nested containers exist. OK, then lets talk about that, too. When something is 2 layers deep, the outside world cannot make sense of it. The inner one can be a loopback mounted file in the outer one. That means that I need the container itself to be responsible for events so that things are recorded using paths, uids, and pids that make sense to it. It can enrich the events and send them to the outer container. > That is when container G is nested in container F which is in turn > nested in container E which is in turn nested in container D which is in > turn nested in container C which is in turn nested in container B which > is nested in container A there is no one label you can put on audit > messages from container G which is the ``correct'' one. > > Or are you proposing that something in container G have labels > A B C D E F G included on every audit message? We need to have audit events to either be globally tagged so that the outside world understand what happening no matter how deep. Or we need each layer to be responsible for itself. This means having an audit rule match engine for each namespace like netfilter is to networking. > That introduces enough complexity in generating and parsing the messages I > wouldn't trust those messages as the least bug in generation and parsing > would be a security issue. That goes with the territory. > What is the world is VPP? Virtualization Protection Profile. Before people say it doesn't apply, it kind of does. It defines the necessary security mechanisms for either full blown virt like QEMU/Xen based or it gives enough wiggle room for containers and other types of VMs. Specifically, it defines the audit requirements needed for this kind of technology. > It sounds like something non-public thing. Certainly it has never been a > part of the public container discussion and as such it appears to be > completely ridiculous to bring up in a public discussion. No, its a public thing. Audit requirements start in section 5.2: https://www.niap-ccevs.org/pp/PP_SV_V1.0/ -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
On Tuesday, May 05, 2015 10:31:20 AM Aristeu Rozanski wrote: > Hi Steve, > > On Tue, May 05, 2015 at 10:22:32AM -0400, Steve Grubb wrote: > > The requirements for auditing of containers should be derived from VPP. In > > it, it asks for selectable auditing, selective audit, and selective audit > > review. What this means is that we need the container and all its > > children to have one identifier that is inserted into all the events that > > are associated with the container. > > > > With this, its possible to do a search for all events related to a > > container. Its possible to exclude events from a container. Its possible > > to not get any events. > > > > The requirements also call out for the identification of the subject. This > > means that the event should be bound to a syscall such as clone, setns, or > > unshare. > > > > Also, any user space events originating inside the container needs to have > > the container ID added to the user space event - just like auid and > > session id. > > > > Recording each instance of a name space is giving me something that I > > cannot use to do queries required by the security target. Given these > > events, how do I locate a web server event where it accesses a watched > > file? That authentication failed? That an update within the container > > failed? > > > > The requirements are that we have to log the creation, suspension, > > migration, and termination of a container. The requirements are not on > > the individual name space. > > > > Maybe I'm missing how these events give me that. But I'd like to hear how > > I would be able to meet requirements with these 12 events. > > what about cases you don't use lxc, libvirt to create namespaces? There's a pretty good chance that we don't care. We've had file system namespace for about 8 or 9 years and we never needed to have a namespace identifier added. > It's easier if the logging is done by namespaces and in case they're created > by any container manager, it can generate a new event notifying it > created a container named "foo" with these namespaces: x, y, z, w and > from that you can piece together everything that happened. OK, if they are emitted they should be an auxiliary record to clone, setns, or unshare system calls. But lets go down this path. We have 6 or so name spaces. These identifiers will need to be added to every single event in the system so that I can figure out what event belongs to which container. > Userspace tools can change to adapt to using namespaces and the idea of > container to make it easier to lookup for events instead of relying on a > number that might not be there (think someone using unshare, ip netns, ...). That's what I am trying to do...figure out how I can these identifiers to see if this actually solves the problem. This is why I wanted to state the actual requirements. Its easy to lose the overall view. Also, I am concerned about how much extra disk space this is going to eat up. > It was discussed in the past and having the concept of "container" in > kernel space and it's not going to happen, so userspace should deal with > it. This is what I am asking for help with. How do I locate an authentication event from container using the information in these events? -Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V6 05/10] audit: log creation and deletion of namespace instances
Hello, I think there needs to be some more discussion around this. It seems like this is not exactly recording things that are useful for audit. On Friday, April 17, 2015 03:35:52 AM Richard Guy Briggs wrote: > Log the creation and deletion of namespace instances in all 6 types of > namespaces. > > Twelve new audit message types have been introduced: > AUDIT_NS_INIT_MNT 1330/* Record mount namespace instance creation > */ AUDIT_NS_INIT_UTS 1331/* Record UTS namespace instance > creation */ AUDIT_NS_INIT_IPC 1332/* Record IPC namespace > instance creation */ AUDIT_NS_INIT_USER 1333/* Record USER > namespace instance creation */ AUDIT_NS_INIT_PID 1334/* Record > PID namespace instance creation */ AUDIT_NS_INIT_NET 1335/* > Record NET namespace instance creation */ AUDIT_NS_DEL_MNT1336 > /* Record mount namespace instance deletion */ AUDIT_NS_DEL_UTS1337 >/* Record UTS namespace instance deletion */ AUDIT_NS_DEL_IPC > 1338/* Record IPC namespace instance deletion */ AUDIT_NS_DEL_USER > 1339/* Record USER namespace instance deletion */ AUDIT_NS_DEL_PID >1340/* Record PID namespace instance deletion */ AUDIT_NS_DEL_NET > 1341/* Record NET namespace instance deletion */ The requirements for auditing of containers should be derived from VPP. In it, it asks for selectable auditing, selective audit, and selective audit review. What this means is that we need the container and all its children to have one identifier that is inserted into all the events that are associated with the container. With this, its possible to do a search for all events related to a container. Its possible to exclude events from a container. Its possible to not get any events. The requirements also call out for the identification of the subject. This means that the event should be bound to a syscall such as clone, setns, or unshare. Also, any user space events originating inside the container needs to have the container ID added to the user space event - just like auid and session id. Recording each instance of a name space is giving me something that I cannot use to do queries required by the security target. Given these events, how do I locate a web server event where it accesses a watched file? That authentication failed? That an update within the container failed? The requirements are that we have to log the creation, suspension, migration, and termination of a container. The requirements are not on the individual name space. Maybe I'm missing how these events give me that. But I'd like to hear how I would be able to meet requirements with these 12 events. -Steve > As suggested by Eric Paris, there are 12 message types, one for each of > creation and deletion, one for each type of namespace so that text searches > are easier in conjunction with the AUDIT_NS_INFO message type, being able > to search for all records such as "netns=4 " and to avoid fields > disappearing per message type to make ausearch more efficient. > > A typical startup would look roughly like: > > type=AUDIT_NS_INIT_UTS msg=audit(1408577534.868:5): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=kernel dev=00:03 old_utsns=(none) > utsns=-2 res=1 type=AUDIT_NS_INIT_USER msg=audit(1408577534.868:6): pid=1 > uid=0 auid=4294967295 ses=4294967295 subj=kernel dev=00:03 > old_userns=(none) userns=-3 res=1 type=AUDIT_NS_INIT_PID > msg=audit(1408577534.868:7): pid=1 uid=0 auid=4294967295 ses=4294967295 > subj=kernel dev=00:03 old_pidns=(none) pidns=-4 res=1 > type=AUDIT_NS_INIT_MNT msg=audit(1408577534.868:8): pid=1 uid=0 > auid=4294967295 ses=4294967295 subj=kernel dev=00:03 old_mntns=(none) > mntns=0 res=1 type=AUDIT_NS_INIT_IPC msg=audit(1408577534.868:9): pid=1 > uid=0 auid=4294967295 ses=4294967295 subj=kernel dev=00:03 old_ipcns=(none) > ipcns=-1 res=1 type=AUDIT_NS_INIT_NET msg=audit(1408577533.500:10): pid=1 > uid=0 auid=4294967295 ses=4294967295 subj=kernel dev=00:03 old_netns=(none) > netns=2 res=1 > > And a CLONE action would result in: > type=type=AUDIT_NS_INIT_NET msg=audit(1408577535.306:81): pid=481 uid=0 > auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 dev=00:03 > old_netns=2 netns=3 res=1 > > While deleting a namespace would result in: > type=type=AUDIT_NS_DEL_MNT msg=audit(1408577552.221:85): pid=481 uid=0 > auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 dev=00:03 > mntns=4 res=1 > > If not "(none)", old_XXXns lists the namespace from which it was cloned. > > Signed-off-by: Richard Guy Briggs > --- > fs/namespace.c | 13 + > include/linux/audit.h |8 + > include/uapi/linux/audit.h | 12 > ipc/namespace.c| 12 > kernel/audit.c | 64 > kernel/pid_namespace.c | > 13 + > kernel/user_namespace.c| 13 ++
Re: [PATCH V4] audit: add tty field to LOGIN event
On Thursday, April 21, 2016 09:29:57 PM Paul Moore wrote:
> On Thu, Apr 21, 2016 at 2:14 PM, Richard Guy Briggs wrote:
> > The tty field was missing from AUDIT_LOGIN events.
> >
> > Refactor code to create a new function audit_get_tty(), using it to
> > replace the call in audit_log_task_info() and to add it to
> > audit_log_set_loginuid(). Lock and bump the kref to protect it, adding
> > audit_put_tty() alias to decrement it.
> >
> > Signed-off-by: Richard Guy Briggs
> > ---
> >
> > V4: Add missing prototype for audit_put_tty() when audit syscall is not
> >
> > enabled (MIPS).
> >
> > V3: Introduce audit_put_tty() alias to decrement kref.
> >
> > V2: Use kref to protect tty signal struct while in use.
> >
> > ---
> >
> > include/linux/audit.h | 24
> > kernel/audit.c| 18 +-
> > kernel/auditsc.c |8 ++--
> > 3 files changed, 35 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index b40ed5d..32cdafb 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >
> > #include
> > #include
> > #include
> >
> > +#include
> >
> > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > #define AUDIT_DEV_UNSET ((dev_t)-1)
> >
> > @@ -343,6 +344,23 @@ static inline unsigned int audit_get_sessionid(struct
> > task_struct *tsk)>
> > return tsk->sessionid;
> >
> > }
> >
> > +static inline struct tty_struct *audit_get_tty(struct task_struct *tsk)
> > +{
> > + struct tty_struct *tty = NULL;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&tsk->sighand->siglock, flags);
> > + if (tsk->signal)
> > + tty = tty_kref_get(tsk->signal->tty);
> > + spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> > + return tty;
> > +}
> > +
> > +static inline void audit_put_tty(struct tty_struct *tty)
> > +{
> > + tty_kref_put(tty);
> > +}
>
> I'm generally not a big fan of defining functions as inlines in header
> files, with the general exception of dummy functions that will get
> compiled out. Although I guess there might be some performance
> argument to be made wrt audit_log_task_info().
>
> I guess I'm fine with this, but what was the idea behind the static
> inlines in audit.h? Performance, or something else?
I think that is normal to prevent multiple definitions at link time.
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 195ffae..71e14d8 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1980,6 +1980,7 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,>
> > {
> >
> > struct audit_buffer *ab;
> > uid_t uid, oldloginuid, loginuid;
> >
> > + struct tty_struct *tty;
> >
> > if (!audit_enabled)
> >
> > return;
> >
> > @@ -1987,14 +1988,17 @@ static void audit_log_set_loginuid(kuid_t
> > koldloginuid, kuid_t kloginuid,>
> > uid = from_kuid(&init_user_ns, task_uid(current));
> > oldloginuid = from_kuid(&init_user_ns, koldloginuid);
> > loginuid = from_kuid(&init_user_ns, kloginuid),
> >
> > + tty = audit_get_tty(current);
> >
> > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_LOGIN);
> > if (!ab)
> >
> > return;
> >
> > audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
> > audit_log_task_context(ab);
> >
> > - audit_log_format(ab, " old-auid=%u auid=%u old-ses=%u ses=%u
> > res=%d", -oldloginuid, loginuid, oldsessionid,
> > sessionid, !rc); + audit_log_format(ab, " old-auid=%u auid=%u
> > tty=%s old-ses=%u ses=%u res=%d", +oldloginuid,
> > loginuid, tty ? tty_name(tty) : "(none)", +
> > oldsessionid, sessionid, !rc);
> > + audit_put_tty(tty);
> >
> > audit_log_end(ab);
> >
> > }
>
> Because we are still using the crappy fixed string format for
> kernel<->userspace communication, this patch will likely "break the
> world" ... let's check with Steve but I believe the way to handle this
> is to add the tty information to the end of the record.
The placement is OK. Thanks for asking.
-Steve
Re: [RFC] Create an audit record of USB specific details
On Tuesday, April 05, 2016 07:02:48 PM Oliver Neukum wrote: > On Tue, 2016-04-05 at 18:40 +1000, Wade Mealing wrote: > > Consider the following scenario. Currently we have device drivers > > that emit text via a printk request which is eventually picked up by > > syslog like implementation (not the audit subsystem). > > We also have UEVENTs. The crucial question is why udevd feeding > back events to the audit subsystem is inferior to the kernel > itself generating audit events. If this was going to be done in user space, then we are talking about auditd growing the ability to monitor another netlink socket for events. The question that decides if this is feasible is whether or not UEVENTS are protected from loss if several occur in a short time before auditd can get around to reading them. The other issue that I'm curious about is if adding hardware can fail. Do the events coming out by UEVENTS have any sense of pass or fail? Or are they all implicitly successful? And then we get to the issue of whether or not UEVENTS can be filtered. If so, then we will also need to add auditing around the configuration of the filters to see if anything is impacting the audit trail. -Steve
Re: [RFC] Create an audit record of USB specific details
On Monday, April 04, 2016 05:56:26 AM Greg KH wrote: > On Mon, Apr 04, 2016 at 12:02:42AM -0400, wmealing wrote: > > From: Wade Mealing > > > > Gday, > > > > I'm looking to create an audit trail for when devices are added or removed > > from the system. > > Then please do it in userspace, as I suggested before, that way you > catch all types of devices, not just USB ones. > > Also I don't think you realize that USB interfaces are what are bound to > drivers, not USB devices, so that is going to mess with any attempted > audit trails here. How are you going to distinguish between the 5 > different devices that just got plugged in that all have / as > vid/pid for them because they are "cheap" devices from China, yet do > totally different things because they are different _types_ of devices? This sounds like vid/pid should be captured in the event. > Again, do this in userspace please, that is where it belongs. There is one issue that may need some clarification. The audit system has to do everything possible to make sure that an event is captured and logged. Does the uevent netlink protocol ever drop events because the user space queue is full? If the uevent interface drops events, then its not audit quality in terms of doing everything possible to prevent the loss of a record. If this were to happen, how would user space find out when a uevent gets dropped? I may have to panic the machine if that happens depending on the configured policy. So, we need to know when it happens. If on the otherhand it doesn't ever drop events, then it might be usable. -Steve
Re: [RFC] Create an audit record of USB specific details
On Monday, April 04, 2016 12:02:42 AM wmealing wrote: > I'm looking to create an audit trail for when devices are added or removed > from the system. > > The audit subsystem is a logging subsystem in kernel space that can be > used to create advanced filters on generated events. It has partnered > userspace utilities ausearch, auditd, aureport, auditctl which work > exclusively on audit records. > > These tools are able to set filters to "trigger" on specific in-kernel > events specified by privileged users. While the userspace tools can create > audit events these are not able to be handled intelligently > (decoded,filtered or ignored) as kernel generated audit events are. > > I have this working at the moment with the USB subsystem (as an example). > Its been suggested that I use systemd-udev however this means that the audit > tools (ausearch) will not be able to index these records. > > Here is an example of picking out the AUDIT_DEVICE record type for example. > > > # ausearch -l -i -ts today -m AUDIT_DEVICE > > > > type=AUDIT_DEVICE msg=audit(31/03/16 16:37:15.642:2) : action=add > > manufacturer=Linux 4.4.0-ktest ehci_hcd product=EHCI Host Controller > > serial=:00:06.7 major=189 minor=0 bus="usb" About this event's format...we can't have any spaces in the value side of the name=value fields unless its encoded as an untrusted string. You can replace spaces with an underscore or dash for readability. So, manufacturer and product would need this treatment. -Steve > Admittedly this is only the USB device type at the moment, but I'd like to > break this out into other bus types at some time in the future, gotta start > somewhere.
Re: [PATCH v2 0/5] Add support for O_MAYEXEC
On Friday, September 6, 2019 11:24:50 AM EDT Mickaël Salaün wrote:
> The goal of this patch series is to control script interpretation. A
> new O_MAYEXEC flag used by sys_open() is added to enable userspace
> script interpreter to delegate to the kernel (and thus the system
> security policy) the permission to interpret/execute scripts or other
> files containing what can be seen as commands.
The problem is that this is only a gentleman's handshake. If I don't tell the
kernel that what I'm opening is tantamount to executing it, then the security
feature is never invoked. It is simple to strip the flags off of any system
call without needing privileges. For example:
#define _GNU_SOURCE
#include
#include
#include
unsigned int
la_version(unsigned int version)
{
return version;
}
unsigned int
la_objopen(struct link_map *map, Lmid_t lmid, uintptr_t *cookie)
{
return LA_FLG_BINDTO | LA_FLG_BINDFROM;
}
typedef int (*openat_t) (int dirfd, const char *pathname, int flags, mode_t
mode);
static openat_t real_openat = 0L;
int my_openat(int dirfd, const char *pathname, int flags, mode_t mode)
{
flags &= ~O_CLOEXEC;
return real_openat(dirfd, pathname, flags, mode);
}
uintptr_t
la_symbind64(Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
uintptr_t *defcook, unsigned int *flags, const char *symname)
{
if (real_openat == 0L && strcmp(symname, "openat") == 0) {
real_openat = (openat_t) sym->st_value;
return (uintptr_t) my_openat;
}
return sym->st_value;
}
gcc -c -g -Wno-unused-parameter -W -Wall -Wundef -O2 -Wp,-D_GLIBCXX_ASSERTIONS
-fexceptions -fPIC test.c
gcc -o strip-flags.so.0 -shared -Wl,-soname,strip-flags.so.0 -ldl test.o
Now, let's make a test program:
#include
#include
#include
#include
int main(void)
{
int dir_fd, fd;
DIR *d = opendir("/etc");
dir_fd = dirfd(d);
fd = openat(dir_fd, "passwd", O_RDONLY|O_CLOEXEC);
close (fd);
closedir(d);
return 0;
}
gcc -g -W -Wall -Wundef test.c -o test
OK, let's see what happens.
$ strace ./test 2>&1 | grep passwd
openat(3, "passwd", O_RDONLY|O_CLOEXEC) = 4
Now with LD_AUDIT
$ LD_AUDIT=/home/sgrubb/test/openflags/strip-flags.so.0 strace ./test 2>&1 |
grep passwd
openat(3, "passwd", O_RDONLY) = 4
No O_CLOEXEC flag.
-Steve
Re: [PATCH v2 0/5] Add support for O_MAYEXEC
On Friday, September 6, 2019 2:57:00 PM EDT Florian Weimer wrote: > * Steve Grubb: > > Now with LD_AUDIT > > $ LD_AUDIT=/home/sgrubb/test/openflags/strip-flags.so.0 strace ./test > > 2>&1 | grep passwd openat(3, "passwd", O_RDONLY) = 4 > > > > No O_CLOEXEC flag. > > I think you need to explain in detail why you consider this a problem. Because you can strip the O_MAYEXEC flag from being passed into the kernel. Once you do that, you defeat the security mechanism because it never gets invoked. The issue is that the only thing that knows _why_ something is being opened is user space. With this mechanism, you can attempt to pass this reason to the kernel so that it may see if policy permits this. But you can just remove the flag. -Steve > With LD_PRELOAD and LD_AUDIT, you can already do anything, including > scanning other loaded objects for a system call instruction and jumping > to that (in case a security module in the kernel performs a PC check to > confer additional privileges). > > Thanks, > Florian
Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister
On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wrote:
> On 2020-04-28 18:25, Paul Moore wrote:
> > On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs
wrote:
> > > Some table unregister actions seem to be initiated by the kernel to
> > > garbage collect unused tables that are not initiated by any userspace
> > > actions. It was found to be necessary to add the subject credentials
> > > to cover this case to reveal the source of these actions. A sample
> > > record:
> > > type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat
> > > family=bridge entries=0 op=unregister pid=153 uid=root auid=unset
> > > tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0
> > > comm=kworker/u4:2 exe=(null)>
> > [I'm going to comment up here instead of in the code because it is a
> > bit easier for everyone to see what the actual impact might be on the
> > records.]
> >
> > Steve wants subject info in this case, okay, but let's try to trim out
> > some of the fields which simply don't make sense in this record; I'm
> > thinking of fields that are unset/empty in the kernel case and are
> > duplicates of other records in the userspace/syscall case. I think
> > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
>
> From the ghak28 discussion, this list and order was selected due to
> Steve's preference for the "kernel" record convention, so deviating from
> this will create yet a new field list. I'll defer to Steve on this. It
> also has to do with the searchability of fields if they are missing.
>
> I do agree that some fields will be superfluous in the kernel case.
> The most important field would be "subj", but then "pid" and "comm", I
> would think. Based on this contents of the "subj" field, I'd think that
> "uid", "auid", "tty", "ses" and "exe" are not needed.
We can't be adding deleting fields based on how its triggered. If they are
unset, that is fine. The main issue is they have to behave the same.
> > While "auid" is a potential target for removal based on the
> > dup-or-unset criteria, I think it falls under Steve's request for
> > subject info here, even if it is garbage in this case.
auid is always unset for daemons. We do not throw it away because of that.
-Steve
> If we keep auid, I'd say keep ses, since they usually go together,
> though they are separated by another field in this "kernel" record field
> ordering.
>
> I expect this orphan record to occur so infrequently that I don't think
> bandwidth or space are a serious concern.
>
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > >
> > > kernel/auditsc.c | 18 ++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d281c18d1771..d7a45b181be0 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2557,12 +2557,30 @@ void __audit_log_nfcfg(const char *name, u8 af,
> > > unsigned int nentries,> >
> > >enum audit_nfcfgop op)
> > >
> > > {
> > >
> > > struct audit_buffer *ab;
> > >
> > > + const struct cred *cred;
> > > + struct tty_struct *tty;
> > > + char comm[sizeof(current->comm)];
> > >
> > > ab = audit_log_start(audit_context(), GFP_KERNEL,
> > > AUDIT_NETFILTER_CFG);
> > > if (!ab)
> > >
> > > return;
> > >
> > > audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
> > >
> > > name, af, nentries, audit_nfcfgs[op].s);
> > >
> > > +
> > > + cred = current_cred();
> > > + tty = audit_get_tty();
> > > + audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u",
> > > +task_pid_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); /* subj= */
> > > + audit_log_format(ab, " comm=");
> > > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > > + audit_log_d_path_exe(ab, current->mm); /* exe= */
> > > +
> > >
> > > audit_log_end(ab);
> > >
> > > }
> > > EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
>
> - RGB
>
> --
> Richard Guy Briggs
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, 28 Jan 2019 11:26:51 -0500 Paul Moore wrote: > On Mon, Jan 28, 2019 at 10:38 AM Sverdlin, Alexander (Nokia - DE/Ulm) > wrote: > > Hello Paul, > > > > On 28/01/2019 15:52, Paul Moore wrote: > > > time also enables syscall auditing; this patch simplifies the > > > Kconfig menus by removing the option to disable syscall > > > auditing when audit is selected and the target arch supports > > > it. > > > > > > Signed-off-by: Paul Moore > > this patch is responsible for massive performance degradation > > for those who used only CONFIG_SECURITY_APPARMOR. > > > > And the numbers are, take the following test for instance: > > > > dd if=/dev/zero of=/dev/null count=2M > > > > ARM64: 500MB/s -> 350MB/s > > ARM:400MB/s -> 300MB/s > > >>> Hi there. > > >>> > > >>> Out of curiosity, what kernel/distribution are you running, or > > >>> is this a custom kernel compile? Can you also share the output > > >>> of 'auditctl > > >> This test was carried out with Linux 4.9. Custom built. > > > I suspected that was the case, thanks. > > > > > >>> -l' from your system? The general approach taken by everyone to > > >>> turn-off the per-syscall audit overhead is to add the "-a > > >>> never,task" rule to their audit configuration: > > >>> > > >>> # auditctl -a never,task > > >>> > > >>> If you are using Fedora/CentOS/RHEL, or a similarly configured > > >>> system, > > >> This is an embedded distribution. We are not using auditctl or > > >> any other audit-related user-space packages. > > >> > > >>> you can find this configuration in the /etc/audit/audit.rules > > >>> file (be warned, that file is automatically generated based on > > >>> /etc/audit/rules.d). > > >> I suppose in this case rule list must be empty. Is there a way > > >> to check this without extra user-space packages? > > > Yes, unless you are loading rules through some other method I > > > would expect that your audit rule list is empty. > > > > > > I'm not aware of any other tools besides auditctl to load audit > > > rules into the kernel, although I haven't ever had a need for > > > another tool so I haven't looked very hard. If you didn't want > > > to bring auditctl into your distribution, I expect it would be a > > > rather trivial task to create a small tool to load a single "-a > > > never,task" into the kernel. > > > > I've done a quick test on my x86_64 PC and got the following > > results: > > ... > > > Which brings me to an idea, that the subject patch should have been > > accompanied by a default "never,task" rule inside the kernel, > > otherwise you require an extra user-space package (audit) just to > > bring Linux 4.5+ to 4.4 performance levels. > > [NOTE: I dropped pmo...@redhat.com from the To/CC line, I left Red Hat > for greener pastures several months ago.] > > Well, it generally hasn't been an issue as 1) most people that enable > audit also enable syscall auditing and 2) most people that enable > audit have some sort of audit userspace tools to work with the audit > logs (and configure audit if necessary). I don't want to diminish > your report, but this change was made several years ago and we really > haven't heard of many issues surrounding the change. If we can ever > get all of the different arches to support syscall auditing, I'd love > to get rid of the syscall auditing Kconfig knob entirely. > > If you wanted to put together a patch that added a single "-a > never,task" rule on boot I could get behind that, just make it default > to off. That will make processes unauditable for everyone. That's how it gets a speedup is not entering into the audit machinery. I suppose its possible that people may want MAC hardwired events but no syscall events. I don't know if there are other kconfig audit options. but maybe getting it down to audit enabled and syscall auditing as the only choices is probably the most performant. -Steve
Re: [PATCH] audit: always enable syscall auditing when supported and audit is enabled
On Mon, 28 Jan 2019 15:08:56 -0500 Paul Moore wrote: > On Mon, Jan 28, 2019 at 3:03 PM Steve Grubb wrote: > > On Mon, 28 Jan 2019 11:26:51 -0500 > > Paul Moore wrote: > > > > > On Mon, Jan 28, 2019 at 10:38 AM Sverdlin, Alexander (Nokia - > > > DE/Ulm) wrote: > > > > Hello Paul, > > > > > > > > On 28/01/2019 15:52, Paul Moore wrote: > > > > >>>>> time also enables syscall auditing; this patch simplifies > > > > >>>>> the Kconfig menus by removing the option to disable > > > > >>>>> syscall auditing when audit is selected and the target > > > > >>>>> arch supports it. > > > > >>>>> > > > > >>>>> Signed-off-by: Paul Moore > > > > >>>> this patch is responsible for massive performance > > > > >>>> degradation for those who used only > > > > >>>> CONFIG_SECURITY_APPARMOR. > > > > >>>> > > > > >>>> And the numbers are, take the following test for instance: > > > > >>>> > > > > >>>> dd if=/dev/zero of=/dev/null count=2M > > > > >>>> > > > > >>>> ARM64: 500MB/s -> 350MB/s > > > > >>>> ARM:400MB/s -> 300MB/s > > > > >>> Hi there. > > > > >>> > > > > >>> Out of curiosity, what kernel/distribution are you running, > > > > >>> or is this a custom kernel compile? Can you also share the > > > > >>> output of 'auditctl > > > > >> This test was carried out with Linux 4.9. Custom built. > > > > > I suspected that was the case, thanks. > > > > > > > > > >>> -l' from your system? The general approach taken by > > > > >>> everyone to turn-off the per-syscall audit overhead is to > > > > >>> add the "-a never,task" rule to their audit configuration: > > > > >>> > > > > >>> # auditctl -a never,task > > > > >>> > > > > >>> If you are using Fedora/CentOS/RHEL, or a similarly > > > > >>> configured system, > > > > >> This is an embedded distribution. We are not using auditctl > > > > >> or any other audit-related user-space packages. > > > > >> > > > > >>> you can find this configuration in > > > > >>> the /etc/audit/audit.rules file (be warned, that file is > > > > >>> automatically generated based on /etc/audit/rules.d). > > > > >> I suppose in this case rule list must be empty. Is there a > > > > >> way to check this without extra user-space packages? > > > > > Yes, unless you are loading rules through some other method I > > > > > would expect that your audit rule list is empty. > > > > > > > > > > I'm not aware of any other tools besides auditctl to load > > > > > audit rules into the kernel, although I haven't ever had a > > > > > need for another tool so I haven't looked very hard. If you > > > > > didn't want to bring auditctl into your distribution, I > > > > > expect it would be a rather trivial task to create a small > > > > > tool to load a single "-a never,task" into the kernel. > > > > > > > > I've done a quick test on my x86_64 PC and got the following > > > > results: > > > > > > ... > > > > > > > Which brings me to an idea, that the subject patch should have > > > > been accompanied by a default "never,task" rule inside the > > > > kernel, otherwise you require an extra user-space package > > > > (audit) just to bring Linux 4.5+ to 4.4 performance levels. > > > > > > [NOTE: I dropped pmo...@redhat.com from the To/CC line, I left > > > Red Hat for greener pastures several months ago.] > > > > > > Well, it generally hasn't been an issue as 1) most people that > > > enable audit also enable syscall auditing and 2) most people that > > > enable audit have some sort of audit userspace tools to work with > > > the audit logs (and configure audit if necessary). I don't want > > > to diminish your report, but this change was made s
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Mon, 14 Jan 2019 17:58:58 -0500
Paul Moore wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> wrote:
> >
> > Tie syscall information to all CONFIG_CHANGE calls since they are
> > all a result of user actions.
Please don't tie syscall information to this. The syscall will be
sendto. We don't need that information, its implicit. Also, doing this
will possibly wreck things in libauparse. Please test the events with
ausearch --format csv and --format text. IFF the event looks better or
the same should we do this. If stuff disappears, the patch is
breaking things
-Steve
> > Exclude user records from syscall context:
> > Since the function audit_log_common_recv_msg() is shared by a
> > number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_*
> > record types, and since the AUDIT_CONFIG_CHANGE message type has
> > been converted to a syscall accompanied record type, special-case
> > the AUDIT_USER_* range of messages so they remain standalone
> > records.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs
> > ---
> > kernel/audit.c | 27 +++
> > kernel/audit_fsnotify.c | 2 +-
> > kernel/audit_tree.c | 2 +-
> > kernel/audit_watch.c| 2 +-
> > kernel/auditfilter.c| 2 +-
> > 5 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0e8026423fbd..a321fea94cc6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> > }
> >
> > +static inline void audit_log_user_recv_msg(struct audit_buffer
> > **ab, u16 msg_type) +{
> > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > +}
>
> This makes sense because this is used by "user" records ...
>
> > +static inline void audit_log_config_change_alt(struct audit_buffer
> > **ab) +{
> > + audit_log_common_recv_msg(audit_context(), ab,
> > AUDIT_CONFIG_CHANGE); +}
>
> ... and I don't believe this makes sense because there is no real
> logical grouping with the callers like there is for
> audit_log_user_recv_msg().
>
Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore wrote: > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb wrote: > > On Mon, 14 Jan 2019 17:58:58 -0500 > > Paul Moore wrote: > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs > > > wrote: > > > > > > > > Tie syscall information to all CONFIG_CHANGE calls since they > > > > are all a result of user actions. > > > > Please don't tie syscall information to this. The syscall will be > > sendto. We don't need that information, its implicit. Also, doing > > this will possibly wreck things in libauparse. Please test the > > events with ausearch --format csv and --format text. IFF the event > > looks better or the same should we do this. If stuff disappears, > > the patch is breaking things > > We've discussed this quite a bit already; Yes, and you still don't seem be listening. You have to cooperate on modifying events. We as a community need to respect each other's needs and work together to solve problems. What this is saying sounds a lot like I don't care if it breaks things, I'm gonna do it my way. Tough luck. You do not have to make sense of any of these events. So, what does it really matter to you how the event is formed? What I'm asking for is have these changes been vetted to ensure that they are not breaking things? > connecting associated records into a single event is something that > should happen, needs to happen, and will happen. Conceptually it > makes no sense to record the syscall (and any other associated > records) which triggers the audit configuration change, and the > configuration change record itself as two distinct events - they are > the same event. Except that they are not. The design of the audit system is to save disk space as much as possible by emitting single record events on certain event types that are simple. To add a syscall to it adds useless information (such as a socket address record), slows down processing, and wastes disk space. If you get a SYSCALL record, that indicates that you have triggered an event that the system admin has placed explicit rules on. That is different than the common criteria required configuration change event. > We've also heard from a prominent user that > associating records in this way is desirable. > > If the ausearch csv and text audit log transformations can't handle > this particular change, I would consider that a shortcoming of that > code. We have multi-record events now, and this is only going to > increase in the future. Isn't there some kind a guideline about not breaking user space? -Steve > Richard, if you can't make the requested changes to this patch and > resubmit by ... let's say the middle of next week? that should be > enough time, yes? ... please let me know and I'll make the changes and > get this merged. >
Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments
On Thursday, March 7, 2019 7:32:53 AM EST Ondrej Mosnacek wrote:
> Emit an audit record whenever the system clock is changed (i.e. shifted
> by a non-zero offset) by a syscall from userspace. The syscalls than can
> (at the time of writing) trigger such record are:
> - settimeofday(2), stime(2), clock_settime(2) -- via
> do_settimeofday64()
> - adjtimex(2), clock_adjtime(2) -- via do_adjtimex()
>
> The new records have type AUDIT_TIME_INJOFFSET and contain the following
> fields:
> - sec -- the 'seconds' part of the offset
> - nsec -- the 'nanoseconds' part of the offset
>
> For reference, running the following commands:
>
> auditctl -D
> auditctl -a exit,always -F arch=b64 -S adjtimex
> chronyd -q
>
> triggers (among others) a syscall that produces audit records like this:
>
> type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
> type=SYSCALL msg=audit(1530616049.652:13): arch=c03e syscall=159
> success=yes exit=5 a0=7fff57e78270 a1=1 a2=fff0
> a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385
> suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616049.652:13):
> proctitle=6368726F6E7964002D71 cd
> /home/omosnace/Dokumenty/Kernel/worktrees/audit/src/kernel/time s
This is needed for common criteria. Requirements are getting stricter in
certifications of IT products that are time stamp sensitive. The record format
looks fine to me.
Ack for the record format.
-Steve
> The above records have been produced by the following syscall from
> chronyd (as per strace output):
>
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433,
> maxerror=1600, esterror=1600, status=STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033,
> tv_usec=778717675}, tick=1, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
>
> (The struct timex fields above are from *after* the syscall was
> executed, so they contain the current (new) values as set from the
> kernel, except of the 'modes' field, which contains the original value
> sent by the caller.)
>
> Signed-off-by: Ondrej Mosnacek
> ---
> include/linux/audit.h | 15 +++
> include/uapi/linux/audit.h | 1 +
> kernel/auditsc.c | 8
> kernel/time/timekeeping.c | 6 ++
> 4 files changed, 30 insertions(+)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..43a60fbe74be 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -27,6 +27,7 @@
> #include
> #include /* LOOKUP_* */
> #include
> +#include
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -365,6 +366,7 @@ extern void __audit_log_capset(const struct cred *new,
> const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
> extern void __audit_log_kern_module(char *name);
> extern void __audit_fanotify(unsigned int response);
> +extern void __audit_tk_injoffset(struct timespec64 offset);
>
> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> {
> @@ -467,6 +469,16 @@ static inline void audit_fanotify(unsigned int
> response) __audit_fanotify(response);
> }
>
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{
> + /* ignore no-op events */
> + if (offset.tv_sec == 0 && offset.tv_nsec == 0)
> + return;
> +
> + if (!audit_dummy_context())
> + __audit_tk_injoffset(offset);
> +}
> +
> extern int audit_n_rules;
> extern int audit_signals;
> #else /* CONFIG_AUDITSYSCALL */
> @@ -580,6 +592,9 @@ static inline void audit_log_kern_module(char *name)
> static inline void audit_fanotify(unsigned int response)
> { }
>
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{ }
> +
> static inline void audit_ptrace(struct task_struct *t)
> { }
> #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 36a7e3f18e69..2167d55bc800 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -114,6 +114,7 @@
> #define AUDIT_REPLACE1329/* Replace auditd if this
> packet
unanswerd */
> #define AUDIT_KERN_MODULE1330/* Kernel Module events */
> #define AUDIT_FANOTIFY 1331/* Fanotify access decision */
> +#define AUDIT_TIME_INJOFFSET 1332/* Timekeeping offset injected */
>
> #define AUDIT_AVC1400/* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR1401/* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1eab1d4a930..781336d0f2de 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
>
