Re: [RFC PATCH ghak21 4/4] audit: add parent of refused symlink to audit_names

2018-03-08 Thread Paul Moore 
On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs  wrote:
> Audit link denied events for symlinks were missing the parent PATH
> record.  Add it.  Since the full pathname may not be available,
> reconstruct it from the path in the nameidata supplied.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/namei.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0edf133..bf1c046b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -923,6 +923,7 @@ static inline int may_follow_link(struct nameidata *nd)
> const struct inode *inode;
> const struct inode *parent;
> kuid_t puid;
> +   char *pathname;
>
> if (!sysctl_protected_symlinks)
> return 0;
> @@ -945,6 +946,14 @@ static inline int may_follow_link(struct nameidata *nd)
> if (nd->flags & LOOKUP_RCU)
> return -ECHILD;
>
> +   pathname = kmalloc(PATH_MAX + 1, GFP_KERNEL);
> +   if (!pathname)
> +   return -ENOMEM;

Two things here:

1. We need to allocate a buffer to feed to d_absolute_path(), and
getname_kernel() is just going to make another copy so we need to make
sure we clean up after ourselves.  Perhaps I missing it, but I'm not
seeing where we free the kmalloc'd buffer or call putname().  Feel
free to correct me if I'm missing something obvious.

2. While the audit_* calls are going to bail early in the cases where
audit is disabled, or not configured, we are going to pay the penalty
for the kmalloc() call above, as well as the getname_kernel() and
d_absolute_path() calls below.  I think it might be beneficial to
create a new function (audit_log_symlink_denied() perhaps?) which
encapsulates all the audit bits in may_follow_link() and exits early
when needed.  What do you think?

(Point #2 is why I didn't merge patch 3/4, just include it in this
revised patch)

> +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> pathname,
> +  PATH_MAX + 1)),
> +   nd->stack[0].link.dentry, 0);
> +   audit_inode(nd->name, nd->stack[0].link.dentry->d_parent, 
> LOOKUP_PARENT);
> +
> audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> audit_log_link_denied("follow_link", >stack[0].link);
>
> return -EACCES;
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 3/4] audit: add refused symlink to audit_names

2018-03-08 Thread Paul Moore 
On Thu, Mar 8, 2018 at 7:30 PM, Paul Moore  wrote:
> On Wed, Feb 14, 2018 at 11:18 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(+)
>
> Merged.

Scratch that, not merged, although only because I think we need to
refactor patch 4/4 and the refactoring can/should encompass this
patch.

See my comments on 4/4.

>> diff --git a/fs/namei.c b/fs/namei.c
>> index 9cc91fb..0edf133 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", >stack[0].link);
>> return -EACCES;
>>  }
>> --
>> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 3/4] audit: add refused symlink to audit_names

2018-03-08 Thread Paul Moore 
On Wed, Feb 14, 2018 at 11:18 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(+)

Merged.

> diff --git a/fs/namei.c b/fs/namei.c
> index 9cc91fb..0edf133 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", >stack[0].link);
> return -EACCES;
>  }
> --
> 1.8.3.1
>

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 2/4] audit: link denied should not directly generate PATH record

2018-03-08 Thread Paul Moore 
On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs  wrote:
> Audit link denied events generate duplicate PATH records which disagree
> in different ways from symlink and hardlink denials.
> audit_log_link_denied() should not directly generate PATH records.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/audit.c | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)

Merged, thanks.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 4c3fd24..683b249 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2259,31 +2259,19 @@ void audit_log_task_info(struct audit_buffer *ab, 
> struct task_struct *tsk)
>  void audit_log_link_denied(const char *operation, const struct path *link)
>  {
> struct audit_buffer *ab;
> -   struct audit_names *name;
>
> if (!audit_enabled || audit_dummy_context())
> return;
>
> -   name = kzalloc(sizeof(*name), GFP_NOFS);
> -   if (!name)
> -   return;
> -
> /* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
> ab = audit_log_start(current->audit_context, GFP_KERNEL,
>  AUDIT_ANOM_LINK);
> if (!ab)
> -   goto out;
> +   return;
> audit_log_format(ab, "op=%s", operation);
> audit_log_task_info(ab, current);
> audit_log_format(ab, " res=0");
> audit_log_end(ab);
> -
> -   /* Generate AUDIT_PATH record with object. */
> -   name->type = AUDIT_TYPE_NORMAL;
> -   audit_copy_inode(name, link->dentry, d_backing_inode(link->dentry));
> -   audit_log_name(current->audit_context, name, link, 0, NULL);
> -out:
> -   kfree(name);
>  }
>
>  /**
> --
> 1.8.3.1
>



-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

2018-03-08 Thread Paul Moore 
On Thu, Feb 15, 2018 at 5:51 PM, Paul Moore  wrote:
> On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook  wrote:
>> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs  wrote:
>>> On 2018-02-14 09:51, Kees Cook wrote:
 On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs  
 wrote:
 > Audit link denied events emit disjointed records when audit is disabled.
 > No records should be emitted when audit is disabled.
 >
 > See: https://github.com/linux-audit/audit-kernel/issues/21
 > Signed-off-by: Richard Guy Briggs 
 > ---
 >  kernel/audit.c | 3 +++
 >  1 file changed, 3 insertions(+)
 >
 > diff --git a/kernel/audit.c b/kernel/audit.c
 > index 227db99..4c3fd24 100644
 > --- a/kernel/audit.c
 > +++ b/kernel/audit.c
 > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, 
 > const struct path *link)
 > struct audit_buffer *ab;
 > struct audit_names *name;
 >
 > +   if (!audit_enabled || audit_dummy_context())
 > +   return;
 > +
 > name = kzalloc(sizeof(*name), GFP_NOFS);
 > if (!name)
 > return;

 Doesn't this means errors here would be silent if audit isn't enabled?
 I don't that; sysadmins should see this notification regardless of the
 audit state...
>>>
>>> This is a user error and not a system error, so I would think if system
>>> auditing is disabled, they don't care about this kind of error.
>>
>> It could indicate an attack attempt...
>
> We get beat up by several folks when we emit audit records with audit
> disabled, and they have a very valid point.
>
> I'm not arguing that the information isn't useful, I'm arguing that if
> you are interested in the sort of information that audit provides you
> should enable audit.  :)

FYI, merged into audit/next.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: add containerid support for IMA-audit

2018-03-08 Thread Mimi Zohar 
On Thu, 2018-03-08 at 06:21 -0500, Richard Guy Briggs wrote:
> On 2018-03-05 09:24, Mimi Zohar wrote:
> > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote:
> > > On 2018-03-05 08:43, Mimi Zohar wrote:
> > > > Hi Richard,
> > > > 
> > > > This patch has been compiled, but not runtime tested.
> > > 
> > > Ok, great, thank you.  I assume you are offering this patch to be
> > > included in this patchset?
> > 
> > Yes, thank you.
> > 
> > > I'll have a look to see where it fits in the
> > > IMA record.  It might be better if it were an AUDIT_CONTAINER_INFO
> > > auxiliary record, but I'll have a look at the circumstances of the
> > > event.  
> 
> I had a look at the context of this record to see if adding the contid
> field to it made sense.  I think the only records for which the contid
> field makes sense are the two newly proposed records, AUDIT_CONTAINER
> which introduces the container ID and the and AUDIT_CONTAINER_INFO which
> documents the presence of the container ID in a process event (or
> process-less network event).  All others should use the auxiliary record
> AUDIT_CONTAINER_INFO rather than include the contid field directly
> itself.  There are several reasons for this including record length, the
> ability to filter unwanted records, the difficulty of changing the order
> of or removing fields in the future.
> 
> Syscalls get this information automatically if the container ID is set
> for a task via the AUDIT_CONTAINER_INFO auxiliary record.  Generally a
> syscall event is one that uses the task's audit_context while a
> standalone event uses NULL or builds a local audit_context that is
> discarded immediately after the local use.
> 
> Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it
> appears that they should be split into two distinct audit record types.
> 
> The record created in ima_audit_measurement() is a syscall record that
> could possibly stand on its own since the subject attributes are
> present.  If it remains a syscall auxiliary record it will automatically
> have the AUDIT_CONTAINER_INFO record accompany it anyways.  If it is
> decided to detach it (which would save cpu/netlink/disk bandwidth but is
> not recommended due to not wanting to throw away any other syscall
> information or other involved records (PATH, CWD, etc...) then a local
> audit_context would be created for the AUDIT_INTEGRITY_RULE and
> AUDIT_CONTAINERID_INFO records only and immediately discarded.
> 
> The record created in ima_parse_rule() is not currently a syscall record
> since it is passed an audit_context of NULL and it has a very different
> format that does not include any subject attributes (except subj_*=).
> At first glance it appears this one should be a syscall accompanied
> auxiliary record.  Either way it should have an AUDIT_CONTAINER_INFO
> auxiliary record either by being converted to a syscall auxiliary record
> by using current->audit_context rather than NULL when calling
> audit_log_start(), or creating a local audit_context and calling
> 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
> 
> Can you briefly describe the circumstances under which these two
> different identically-numbered records are produced as a first step
> towards splitting them into two distict records?

Agreed, the two uses should really be separated.  ima_parse_rule()
generates audit messages, when the IMA policy is initially loaded,
replaced, or extended, the policy rules are included in the audit log.
When IMA is namespaced, there will be a host policy and namespace
policies.  We'll need to be able differentiate between the host policy
rules and IMA namespaced policy rules, and between IMA namespaced
policy rules.

The audit messages produced by ima_audit_measurement() were originally
upstreamed for forensics, and as seen by the FireEye blog are now used
to augment existing security analytics.  These records are probably
being used independently of any other audit records.  A single record
is generated per file, per system.  With IMA namespacing, these
records need to be generated once per file, per namespace as well.  In
order to differentiate the records between the host and namespace, and
between namespaces, these records should include the container id.

To disambiguate between these audit messages and the policy rule
messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA.

> The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records
> appear to be already properly covered for AUDIT_CONTAINER_INFO records
> by being syscall auxiliary records.  The AUDIT_INTEGRITY_HASH record
> appears to be unused.

Ok

Mimi

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

2018-03-08 Thread Andy Lutomirski 
On Wed, Mar 7, 2018 at 11:41 PM, Paul Moore  wrote:
> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina  wrote:
>> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
>>> Wow, this was a long time ago.
>>
>> Oh yeah; but it now resurfaced on our side, as we are of course receiving
>> a lot of requests with respect to making syscall performance great again
>> :)
>
> Ooof.  I'm not sure I can handle making more things "great again" ;)
>
>>> From memory and a bit of email diving, there are two reasons.
>>>
>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>>>-a task,never cause newly spawned tasks to not suck.  Yes, it's a
>>>very partial solution.  After considerable nagging, I got Fedora to
>>>default to -a task,never.
>>
>> Hm, right; that's a bit inconvenient, because it takes each and every
>> vendor having to realize this option, and put it in. Making kernel do the
>> right thing automatically sounds like a better option to me.
>
> This predates audit falling into my lap, but speaking generally I
> think it would be good if the kernel did The Right Thing, so long as
> it isn't too painful.
>
>>> 2. This patch, as is, may be a bit problematic.  In particular, if one
>>>task changes the audit rules while another task is in the middle of
>>>the syscall, then it's too late to audit that syscall correctly.
>>>This could be seen as a bug or it could be seen as being just fine.
>>
>> I don't think this should be a problem, given the fact that the whole
>> timing/ordering is not predictable anyway due to scheduling.
>>
>> Paul, what do you think?
>
> I'm not overly concerned about the race condition between configuring
> the audit filters and syscalls that are currently in-flight; after all
> we have that now and "fixing" it would be pretty much impractical
> (impossible maybe?).  Most serious audit users configure it during
> boot and let it run, frequent runtime changes are not common as far as
> I can tell.
>
> I just looked quickly at the patch and decided it isn't something I'm
> going to be able to carefully review in the time I've got left today,
> so it's going to have to wait until tomorrow and Friday ... however,
> speaking on general principle I don't have an objection to the ideas
> put forth here.
>
> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> add, that would be good to have.
>

It's sort of my patch, and I was reasonably happy with it, so
definitely no NACK from me.  The only caveat I have is that I wrote
the original version so long ago that we need to re-audit the code.
In particular, I want to make sure that the following two cases don't
result in warnings, oopses, or other misbehavior:

1. Do a syscall with TIF_AUDIT_SYSCALL clear.  Return with
TIF_AUDIT_SYSCALL set and that syscall enabled for auditing.

2.. Do a VFS syscall with TIF_AUDIT_CLEAR and have TIF_AUDIT_SYSCALL
set before we execute any VFS code.  The VFS code will call into the
audit code to log the paths it touches (IIRC).  Again, this shouldn't
warn or otherwise misbehave.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

2018-03-08 Thread Richard Guy Briggs 
On 2018-03-08 06:30, Andy Lutomirski wrote:
> 
> 
> > On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs  wrote:
> > 
> >> On 2018-03-07 18:43, Paul Moore wrote:
> >>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore  wrote:
>  On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina  wrote:
> > On Wed, 7 Mar 2018, Andy Lutomirski wrote:
> > Wow, this was a long time ago.
>  
>  Oh yeah; but it now resurfaced on our side, as we are of course receiving
>  a lot of requests with respect to making syscall performance great again
>  :)
> >>> 
> >>> Ooof.  I'm not sure I can handle making more things "great again" ;)
> >>> 
> > From memory and a bit of email diving, there are two reasons.
> > 
> > 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
> >   -a task,never cause newly spawned tasks to not suck.  Yes, it's a
> >   very partial solution.  After considerable nagging, I got Fedora to
> >   default to -a task,never.
>  
>  Hm, right; that's a bit inconvenient, because it takes each and every
>  vendor having to realize this option, and put it in. Making kernel do the
>  right thing automatically sounds like a better option to me.
> >>> 
> >>> This predates audit falling into my lap, but speaking generally I
> >>> think it would be good if the kernel did The Right Thing, so long as
> >>> it isn't too painful.
> >>> 
> > 2. This patch, as is, may be a bit problematic.  In particular, if one
> >   task changes the audit rules while another task is in the middle of
> >   the syscall, then it's too late to audit that syscall correctly.
> >   This could be seen as a bug or it could be seen as being just fine.
>  
>  I don't think this should be a problem, given the fact that the whole
>  timing/ordering is not predictable anyway due to scheduling.
>  
>  Paul, what do you think?
> >>> 
> >>> I'm not overly concerned about the race condition between configuring
> >>> the audit filters and syscalls that are currently in-flight; after all
> >>> we have that now and "fixing" it would be pretty much impractical
> >>> (impossible maybe?).  Most serious audit users configure it during
> >>> boot and let it run, frequent runtime changes are not common as far as
> >>> I can tell.
> > 
> > I'd agree the race condition here can't easily be fixed and isn't worth
> > fixing for the reasons already stated (rules don't change often and may
> > even be locked once in place relatively early, scheduling uncertainties).
> > 
> >>> I just looked quickly at the patch and decided it isn't something I'm
> >>> going to be able to carefully review in the time I've got left today,
> >>> so it's going to have to wait until tomorrow and Friday ... however,
> >>> speaking on general principle I don't have an objection to the ideas
> >>> put forth here.
> > 
> > The approach seems a bit draconian since it touches all tasks but only
> > when adding the first rule or deleting the last.
> > 
> > What we lose is the ability to set or clear individual task auditing and
> > have it stick to speed up non-audited tasks when there are rules
> > present, though this isn't currently used, in favour of audit_context
> > presence.
> > 
> >>> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> >>> add, that would be good to have.
> >> 
> >> ... and I just realized that linux-audit isn't on the To/CC line,
> >> adding them now.
> > 
> > (and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.
> > 
> 
> The mailing list bounces my emails. 

They'll get approved.

> >> Link to the patch is below.
> >> 
> >> * https://marc.info/?t=15204188763=1=2
> >> 
> >> 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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

2018-03-08 Thread Andy Lutomirski 


> On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs  wrote:
> 
>> On 2018-03-07 18:43, Paul Moore wrote:
>>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore  wrote:
 On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina  wrote:
> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
> Wow, this was a long time ago.
 
 Oh yeah; but it now resurfaced on our side, as we are of course receiving
 a lot of requests with respect to making syscall performance great again
 :)
>>> 
>>> Ooof.  I'm not sure I can handle making more things "great again" ;)
>>> 
> From memory and a bit of email diving, there are two reasons.
> 
> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
>   -a task,never cause newly spawned tasks to not suck.  Yes, it's a
>   very partial solution.  After considerable nagging, I got Fedora to
>   default to -a task,never.
 
 Hm, right; that's a bit inconvenient, because it takes each and every
 vendor having to realize this option, and put it in. Making kernel do the
 right thing automatically sounds like a better option to me.
>>> 
>>> This predates audit falling into my lap, but speaking generally I
>>> think it would be good if the kernel did The Right Thing, so long as
>>> it isn't too painful.
>>> 
> 2. This patch, as is, may be a bit problematic.  In particular, if one
>   task changes the audit rules while another task is in the middle of
>   the syscall, then it's too late to audit that syscall correctly.
>   This could be seen as a bug or it could be seen as being just fine.
 
 I don't think this should be a problem, given the fact that the whole
 timing/ordering is not predictable anyway due to scheduling.
 
 Paul, what do you think?
>>> 
>>> I'm not overly concerned about the race condition between configuring
>>> the audit filters and syscalls that are currently in-flight; after all
>>> we have that now and "fixing" it would be pretty much impractical
>>> (impossible maybe?).  Most serious audit users configure it during
>>> boot and let it run, frequent runtime changes are not common as far as
>>> I can tell.
> 
> I'd agree the race condition here can't easily be fixed and isn't worth
> fixing for the reasons already stated (rules don't change often and may
> even be locked once in place relatively early, scheduling uncertainties).
> 
>>> I just looked quickly at the patch and decided it isn't something I'm
>>> going to be able to carefully review in the time I've got left today,
>>> so it's going to have to wait until tomorrow and Friday ... however,
>>> speaking on general principle I don't have an objection to the ideas
>>> put forth here.
> 
> The approach seems a bit draconian since it touches all tasks but only
> when adding the first rule or deleting the last.
> 
> What we lose is the ability to set or clear individual task auditing and
> have it stick to speed up non-audited tasks when there are rules
> present, though this isn't currently used, in favour of audit_context
> presence.
> 
>>> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
>>> add, that would be good to have.
>> 
>> ... and I just realized that linux-audit isn't on the To/CC line,
>> adding them now.
> 
> (and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.
> 

The mailing list bounces my emails. 

>> Link to the patch is below.
>> 
>> * https://marc.info/?t=15204188763=1=2
>> 
>> 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-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

2018-03-08 Thread Richard Guy Briggs 
On 2018-02-14 22:46, Richard Guy Briggs wrote:
> On 2018-02-14 11:49, Steve Grubb wrote:
> > On Wednesday, February 14, 2018 11:18:20 AM EST 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:
> > 
> > Have these been tested with ausearch-test?
> 
> Not yet.

I should have reported that a day or two later I ran the ausearch-test
which passed.

> > > symlink:
> > > type=PROCTITLE msg=audit(02/14/2018 04:40:21.635:238) : proctitle=cat
> > > my-passwd type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=1
> > > name=/tmp/my-passwd inode=17618 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=PATH msg=audit(02/14/2018
> > > 04:40:21.635:238) : item=0 name=/tmp inode=13446 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=CWD msg=audit(02/14/2018 04:40:21.635:238) :
> > > cwd=/tmp
> > > type=SYSCALL msg=audit(02/14/2018 04:40:21.635:238) : arch=x86_64
> > > syscall=openat success=no exit=EACCES(Permission denied) a0=0xff9c
> > > a1=0x7ffc6c1acdda a2=O_RDONLY a3=0x0 items=2 ppid=549 pid=606 auid=root
> > > uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root
> > > fsgid=root tty=ttyS0 ses=1 comm= cat exe=/usr/bin/cat
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=ANOM_LINK msg=audit(02/14/2018 04:40:21.635:238) : op=follow_link
> > > ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root
> > > fsuid=root egid=roo t sgid=root fsgid=root tty=ttyS0 ses=1 comm=cat
> > > exe=/usr/bin/cat
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> > 
> > This record duplicates the SYSCALL event except for the op field. I would 
> > suggest that is the only field needed.
> 
> Agreed, but at the moment, removal of fields isn't possible unless there
> is a conflict, and even then the value should simply be corrected if
> possible.
> 
> > > 
> > > hardlink:
> > > type=PROCTITLE msg=audit(02/14/2018 04:40:25.373:239) : proctitle=ln test
> > > test-ln type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=1
> > > name=/tmp inode=13446 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(02/14/2018
> > > 04:40:25.373:239) : item=0 name=test inode=17619 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(02/14/2018 04:40:25.373:239) : cwd=/tmp
> > > type=SYSCALL msg=audit(02/14/2018 04:40:25.373:239) : arch=x86_64
> > > syscall=linkat success=no exit=EPERM(Operation not permitted)
> > > a0=0xff9c a1=0x7fffe6c3f628 a2=0xff9c a3=0x7fffe6c3f62d items=2
> > > ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb
> > > egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=ANOM_LINK msg=audit(02/14/2018 04:40:25.373:239) : op=linkat ppid=578
> > > pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb
> > > sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> > > 
> > > The remaining problem is how to address this when syscall logging is
> > > disabled since it needs a parent path record and/or a CWD record to
> > > complete it.  It could also use a proctitle record too.  In fact, it
> > > looks like we need a way to have multiple auxiliary records to support
> > > an arbitrary record.  Comments please.
> > 
> > Perhaps this can only be emitted correctly with SYSCALL auditing enabled. 
> > Otherwise, the event should stand completely on its own without syscall and 
> > path records. The information from them can be added, but it risks hitting 
> > the record size limit.
> 
> As Paul just pointed out (which rang a bell...) in:
>   
> https://github.com/linux-audit/audit-kernel/issues/51#issuecomment-365759325
> CONFIG_AUDITSYSCALL is now forced on and if you sabbotage your
> audit.rules with -a task,never, your warranty is void.
> 
> So now, the lurking questions in the back of my head about the

Re: [PATCH] audit: add containerid support for IMA-audit

2018-03-08 Thread Richard Guy Briggs 
On 2018-03-05 09:24, Mimi Zohar wrote:
> On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote:
> > On 2018-03-05 08:43, Mimi Zohar wrote:
> > > Hi Richard,
> > > 
> > > This patch has been compiled, but not runtime tested.
> > 
> > Ok, great, thank you.  I assume you are offering this patch to be
> > included in this patchset?
> 
> Yes, thank you.
> 
> > I'll have a look to see where it fits in the
> > IMA record.  It might be better if it were an AUDIT_CONTAINER_INFO
> > auxiliary record, but I'll have a look at the circumstances of the
> > event.  

I had a look at the context of this record to see if adding the contid
field to it made sense.  I think the only records for which the contid
field makes sense are the two newly proposed records, AUDIT_CONTAINER
which introduces the container ID and the and AUDIT_CONTAINER_INFO which
documents the presence of the container ID in a process event (or
process-less network event).  All others should use the auxiliary record
AUDIT_CONTAINER_INFO rather than include the contid field directly
itself.  There are several reasons for this including record length, the
ability to filter unwanted records, the difficulty of changing the order
of or removing fields in the future.

Syscalls get this information automatically if the container ID is set
for a task via the AUDIT_CONTAINER_INFO auxiliary record.  Generally a
syscall event is one that uses the task's audit_context while a
standalone event uses NULL or builds a local audit_context that is
discarded immediately after the local use.

Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it
appears that they should be split into two distinct audit record types.

The record created in ima_audit_measurement() is a syscall record that
could possibly stand on its own since the subject attributes are
present.  If it remains a syscall auxiliary record it will automatically
have the AUDIT_CONTAINER_INFO record accompany it anyways.  If it is
decided to detach it (which would save cpu/netlink/disk bandwidth but is
not recommended due to not wanting to throw away any other syscall
information or other involved records (PATH, CWD, etc...) then a local
audit_context would be created for the AUDIT_INTEGRITY_RULE and
AUDIT_CONTAINERID_INFO records only and immediately discarded.

The record created in ima_parse_rule() is not currently a syscall record
since it is passed an audit_context of NULL and it has a very different
format that does not include any subject attributes (except subj_*=).
At first glance it appears this one should be a syscall accompanied
auxiliary record.  Either way it should have an AUDIT_CONTAINER_INFO
auxiliary record either by being converted to a syscall auxiliary record
by using current->audit_context rather than NULL when calling
audit_log_start(), or creating a local audit_context and calling
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

Can you briefly describe the circumstances under which these two
different identically-numbered records are produced as a first step
towards splitting them into two distict records?

The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records
appear to be already properly covered for AUDIT_CONTAINER_INFO records
by being syscall auxiliary records.  The AUDIT_INTEGRITY_HASH record
appears to be unused.

> > Can you suggest a procedure to test it?
> 
> Like IMA-measurement and IMA-appraisal, IMA-audit is enabled based on
> policy. The example IMA policy, below, includes IMA-audit messages for
> files executed. 'cat' the policy to /sys/kernel/security/ima/policy.
> 
> /etc/ima/ima-policy:
> audit func=BPRM_CHECK
> 
> There's a FireEye blog titled "Extending Linux Executable Logging With
> The Integrity Measurement Architecture"* that explains how to augment
> their existing system security analytics with file hashes.
> 
> * https://www.fireeye.com/blog/threat-research/2016/11/extending_linux
> _exec.html
> 
> 
> Mimi
> 
> > > If the containerid is defined, include it in the IMA-audit record.
> > > 
> > > Signed-off-by: Mimi Zohar 
> > > ---
> > >  security/integrity/ima/ima_api.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/security/integrity/ima/ima_api.c 
> > > b/security/integrity/ima/ima_api.c
> > > index 33b4458cdbef..41d29a06f28f 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -335,6 +335,9 @@ void ima_audit_measurement(struct 
> > > integrity_iint_cache *iint,
> > >   audit_log_untrustedstring(ab, algo_hash);
> > >  
> > >   audit_log_task_info(ab, current);
> > > + if (audit_containerid_set(current))
> > > + audit_log_format(ab, " contid=%llu",
> > > +  audit_get_containerid(current));
> > >   audit_log_end(ab);
> > >  
> > >   iint->flags |= IMA_AUDITED;
> >

Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated

2018-03-08 Thread Richard Guy Briggs 
On 2018-03-07 18:43, Paul Moore wrote:
> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore  wrote:
> > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina  wrote:
> >> On Wed, 7 Mar 2018, Andy Lutomirski wrote:
> >>> Wow, this was a long time ago.
> >>
> >> Oh yeah; but it now resurfaced on our side, as we are of course receiving
> >> a lot of requests with respect to making syscall performance great again
> >> :)
> >
> > Ooof.  I'm not sure I can handle making more things "great again" ;)
> >
> >>> From memory and a bit of email diving, there are two reasons.
> >>>
> >>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl
> >>>-a task,never cause newly spawned tasks to not suck.  Yes, it's a
> >>>very partial solution.  After considerable nagging, I got Fedora to
> >>>default to -a task,never.
> >>
> >> Hm, right; that's a bit inconvenient, because it takes each and every
> >> vendor having to realize this option, and put it in. Making kernel do the
> >> right thing automatically sounds like a better option to me.
> >
> > This predates audit falling into my lap, but speaking generally I
> > think it would be good if the kernel did The Right Thing, so long as
> > it isn't too painful.
> >
> >>> 2. This patch, as is, may be a bit problematic.  In particular, if one
> >>>task changes the audit rules while another task is in the middle of
> >>>the syscall, then it's too late to audit that syscall correctly.
> >>>This could be seen as a bug or it could be seen as being just fine.
> >>
> >> I don't think this should be a problem, given the fact that the whole
> >> timing/ordering is not predictable anyway due to scheduling.
> >>
> >> Paul, what do you think?
> >
> > I'm not overly concerned about the race condition between configuring
> > the audit filters and syscalls that are currently in-flight; after all
> > we have that now and "fixing" it would be pretty much impractical
> > (impossible maybe?).  Most serious audit users configure it during
> > boot and let it run, frequent runtime changes are not common as far as
> > I can tell.

I'd agree the race condition here can't easily be fixed and isn't worth
fixing for the reasons already stated (rules don't change often and may
even be locked once in place relatively early, scheduling uncertainties).

> > I just looked quickly at the patch and decided it isn't something I'm
> > going to be able to carefully review in the time I've got left today,
> > so it's going to have to wait until tomorrow and Friday ... however,
> > speaking on general principle I don't have an objection to the ideas
> > put forth here.

The approach seems a bit draconian since it touches all tasks but only
when adding the first rule or deleting the last.

What we lose is the ability to set or clear individual task auditing and
have it stick to speed up non-audited tasks when there are rules
present, though this isn't currently used, in favour of audit_context
presence.

> > Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to
> > add, that would be good to have.
> 
> ... and I just realized that linux-audit isn't on the To/CC line,
> adding them now.

(and Andy's non-NACK missed too...)  The mailing list *is* in MAINTAINERS.

> Link to the patch is below.
> 
> * https://marc.info/?t=15204188763=1=2
> 
> 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-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit