date:20180518

Re: [PATCH] audit: use existing session info function

2018-05-18 Thread Paul Moore

On Thu, May 17, 2018 at 10:01 PM, Richard Guy Briggs  wrote:
> Use the existing audit_log_session_info() function rather than
> hardcoding its functionality.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/auditfilter.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Merged into audit/next, thanks.

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index d7a807e..9e87377 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1089,8 +1089,6 @@ static void audit_list_rules(int seq, struct 
> sk_buff_head *q)
>  static void audit_log_rule_change(char *action, struct audit_krule *rule, 
> int res)
>  {
> struct audit_buffer *ab;
> -   uid_t loginuid = from_kuid(_user_ns, 
> audit_get_loginuid(current));
> -   unsigned int sessionid = audit_get_sessionid(current);
>
> if (!audit_enabled)
> return;
> @@ -1098,7 +1096,7 @@ static void audit_log_rule_change(char *action, struct 
> audit_krule *rule, int re
> ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (!ab)
> return;
> -   audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> +   audit_log_session_info(ab);
> audit_log_task_context(ab);
> audit_log_format(ab, " op=%s", action);
> audit_log_key(ab, rule->filterkey);
> --
> 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: [PATCH ghak82] audit: Fix wrong task in comparison of session ID

2018-05-18 Thread Paul Moore

On Thu, May 17, 2018 at 11:31 AM, Ondrej Mosnacek  wrote:
> The audit_filter_rules() function in auditsc.c compared the session ID
> with the credentials of the current task, while it should use the
> credentials of the task given to audit_filter_rules() as a parameter
> (tsk).
>
> GitHub issue:
> https://github.com/linux-audit/audit-kernel/issues/82
>
> Fixes: 8fae47705685 ("audit: add support for session ID user filter")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Ondrej Mosnacek 
> ---
>  kernel/auditsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Good catch.  However, I'm not completely convinced this is stable material.

This bug really only affects "task" filters which I believe to be
fairly limited in the wild, due to the limited number of fields that
one can filter on.  Every other filter, including the "exit" filter,
work as expected (which is why the audit-testsuite didn't catch this
bug).  Further, even in the case of the task filter, the *only* time
where the current session ID would be different from the tsk session
ID is in the case of a login event, but even in this case the two
values should be equal during the "task" filtering as you can't change
the login-ID/session-ID until after you have successfully
fork()'d/clone()'d the new task.

I'll hold off on merging this in case I'm missing some even more
subtle point that you've found.  From what I can tell this is a good
fix, but not something an actual user would ever really encounter and
therefore not something that warrants inclusion in stable.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ec38e4d97c23..6d577a34b16b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -513,7 +513,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = audit_gid_comparator(cred->fsgid, f->op, 
> f->gid);
> break;
> case AUDIT_SESSIONID:
> -   sessionid = audit_get_sessionid(current);
> +   sessionid = audit_get_sessionid(tsk);
> result = audit_comparator(sessionid, f->op, f->val);
> break;
> case AUDIT_PERS:
> --
> 2.17.0

-- 
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-05-18 Thread Richard Guy Briggs

On 2018-05-18 12:49, Stefan Berger wrote:
> On 05/18/2018 11:45 AM, Richard Guy Briggs wrote:
> > On 2018-05-18 07:49, Stefan Berger wrote:
> > > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:
> > > > On 2018-05-17 10:18, Stefan Berger wrote:
> > > > > On 03/08/2018 06:21 AM, 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.
> > > > > What does 'detach it' mean? Does it mean we're not using
> > > > > current->audit_context?
> > > > Exactly.
> > > > 
> > > > > > 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
> > > > > Do you have an example (pointer) to the format for a 'syscall 
> > > > > accompanied
> > > > > auxiliary record'?
> > > > Any that uses current->audit_context (or recently proposed
> > > > audit_context() function) will be a syscall auxiliary record.  Well
> > > > formed record formats are = and named as listed:
> > > > 
> > > > 
> > > > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events
> > > > 
> > > > https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv
> > > > 
> > > > > > 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
> > > > > ima_parse_rule() is invoked when setting a policy by writing it into
> > > > > /sys/kernel/security/ima/policy. We unfortunately don't have the
> > > > > current->audit_context in this case.
> > > > Sure you

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

2018-05-18 Thread Richard Guy Briggs

On 2018-05-18 12:34, Mimi Zohar wrote:
> On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote:
> > On 2018-05-18 10:39, Mimi Zohar wrote:
> > > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:
> > > > On 05/18/2018 08:53 AM, Mimi Zohar wrote:
> > > 
> > > [..]
> > > 
> > > >  If so, which ones? 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?
> > > > >>> Changing the order of existing fields or inserting fields could 
> > > > >>> break
> > > > >>> stuff and is strongly discouraged without a good reason, but 
> > > > >>> appending
> > > > >>> fields is usually the right way to add information.
> > > > >>>
> > > > >>> There are exceptions, and in this case, I'd pick the "more 
> > > > >>> standard" of
> > > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and 
> > > > >>> stick
> > > > >>> with that, abandoning the other format, renaming the less standard
> > > > >>> version of the record (ima_parse_rule?) and perhpas adopting that
> > > > >>> abandonned format for the new record type while using
> > > > >>> current->audit_context.
> > > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> > > > > ima_audit_measurement().  Could we rename type=1805 to be
> > > > 
> > > > So do we want to change both? I thought that what 
> > > > ima_audit_measurement() produces looks ok but may not have a good name 
> > > > for the 'type'. Now in this case I would not want to 'break user space'.
> > > > The only change I was going to make was to what ima_parse_rule() 
> > > > produces.
> > > 
> > > The only change for now is separating the IMA policy rules from the
> > > IMA-audit messages.
> > > 
> > > Richard, when the containerid is appended to the IMA-audit messages,
> > > would we make the audit type name change then?
> > 
> > No, go ahead and make the change now.  I'm expecting that the
> > containerid record will just be another auxiliary record and should not
> > affect you folks.
> 
> To summarize, we need to disambiguate the 1805, as both
> ima_parse_rule() and ima_audit_measurement() are using the same number
> with different formats.  The main usage of 1805 that we are aware of
> is ima_audit_measurement().  Yet the "type=" name for
> ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
> INTEGRITY_RULE.
> 
> option 1: breaks both uses
> 1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
> 
> option 2: breaks the most common usage
> 1805 - INTEGRITY_RULE - ima_parse_rule()
> 1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
> 
> option 3: leaves the most common usage with the wrong name, and breaks
> the other less common usage
> 1805 - INTEGRITY_RULE - ima_audit_measurement()
> 1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()
> 
> So option 3 is the best option?

Yes, I think so, but option 2 I would be willing to consider.  I'd like
to get Paul and Steve's opinions on this.

> Mimi

- 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: add containerid support for IMA-audit

2018-05-18 Thread Stefan Berger

On 05/18/2018 11:45 AM, Richard Guy Briggs wrote:

On 2018-05-18 07:49, Stefan Berger wrote:

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

On 2018-05-17 10:18, Stefan Berger wrote:

On 03/08/2018 06:21 AM, 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.

What does 'detach it' mean? Does it mean we're not using
current->audit_context?

Exactly.

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

Do you have an example (pointer) to the format for a 'syscall accompanied
auxiliary record'?

Any that uses current->audit_context (or recently proposed
audit_context() function) will be a syscall auxiliary record. Well
formed record formats are = and named as listed:

https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv

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

ima_parse_rule() is invoked when setting a policy by writing it into
/sys/kernel/security/ima/policy. We unfortunately don't have the
current->audit_context in this case.

Sure you do. What process writes to that file? That's the one we care
about, unless it is somehow handed off to a queue and processed later in
a different context.

I just printk'd it again. current->audit_context is NULL in this case.

Oops, that sounds like some of the netfilter empty table
initializations, whereas usually rules have a user actor.

So it's a bug elsewhere not a 'feature?'

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'?

Arguably userspace is already broken by this wildly diverging pair of
formats for the same record.

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

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

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

2018-05-18 Thread Mimi Zohar

On Fri, 2018-05-18 at 11:56 -0400, Richard Guy Briggs wrote:
> On 2018-05-18 10:39, Mimi Zohar wrote:
> > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:
> > > On 05/18/2018 08:53 AM, Mimi Zohar wrote:
> > 
> > [..]
> > 
> > >  If so, which ones? 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?
> > > >>> Changing the order of existing fields or inserting fields could break
> > > >>> stuff and is strongly discouraged without a good reason, but appending
> > > >>> fields is usually the right way to add information.
> > > >>>
> > > >>> There are exceptions, and in this case, I'd pick the "more standard" 
> > > >>> of
> > > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and 
> > > >>> stick
> > > >>> with that, abandoning the other format, renaming the less standard
> > > >>> version of the record (ima_parse_rule?) and perhpas adopting that
> > > >>> abandonned format for the new record type while using
> > > >>> current->audit_context.
> > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> > > > ima_audit_measurement().  Could we rename type=1805 to be
> > > 
> > > So do we want to change both? I thought that what 
> > > ima_audit_measurement() produces looks ok but may not have a good name 
> > > for the 'type'. Now in this case I would not want to 'break user space'.
> > > The only change I was going to make was to what ima_parse_rule() produces.
> > 
> > The only change for now is separating the IMA policy rules from the
> > IMA-audit messages.
> > 
> > Richard, when the containerid is appended to the IMA-audit messages,
> > would we make the audit type name change then?
> 
> No, go ahead and make the change now.  I'm expecting that the
> containerid record will just be another auxiliary record and should not
> affect you folks.

To summarize, we need to disambiguate the 1805, as both
ima_parse_rule() and ima_audit_measurement() are using the same number
with different formats.  The main usage of 1805 that we are aware of
is ima_audit_measurement().  Yet the "type=" name for
ima_audit_measurement() should be INTEGRITY_IMA_AUDIT, not
INTEGRITY_RULE.

option 1: breaks both uses
1805 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()

option 2: breaks the most common usage
1805 - INTEGRITY_RULE - ima_parse_rule()
1806 - INTEGRITY_IMA_AUDIT - ima_audit_measurement()

option 3: leaves the most common usage with the wrong name, and breaks
the other less common usage
1805 - INTEGRITY_RULE - ima_audit_measurement()
1806 - INTEGRITY_POLICY_RULE - ima_parse_rule()

So option 3 is the best option?

Mimi

--
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-05-18 Thread Richard Guy Briggs

On 2018-05-18 10:52, Stefan Berger wrote:
> On 05/18/2018 10:39 AM, Mimi Zohar wrote:
> > On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:
> > > On 05/18/2018 08:53 AM, Mimi Zohar wrote:
> > [..]
> > 
> > > > > > > If so, which ones? 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?
> > > > > > Changing the order of existing fields or inserting fields could 
> > > > > > break
> > > > > > stuff and is strongly discouraged without a good reason, but 
> > > > > > appending
> > > > > > fields is usually the right way to add information.
> > > > > > 
> > > > > > There are exceptions, and in this case, I'd pick the "more 
> > > > > > standard" of
> > > > > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and 
> > > > > > stick
> > > > > > with that, abandoning the other format, renaming the less standard
> > > > > > version of the record (ima_parse_rule?) and perhpas adopting that
> > > > > > abandonned format for the new record type while using
> > > > > > current->audit_context.
> > > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> > > > ima_audit_measurement().  Could we rename type=1805 to be
> > > So do we want to change both? I thought that what
> > > ima_audit_measurement() produces looks ok but may not have a good name
> > > for the 'type'. Now in this case I would not want to 'break user space'.
> > > The only change I was going to make was to what ima_parse_rule() produces.
> > The only change for now is separating the IMA policy rules from the
> > IMA-audit messages.
> > 
> > Richard, when the containerid is appended to the IMA-audit messages,
> > would we make the audit type name change then?
> > 
> > > > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
> > > > message could be named INTEGRITY_RULE or, if that would be confusing,
> > > > INTEGRITY_POLICY_RULE.
> > > For 1806, as we would use it in ima_parse_rule(), we could change that
> > > in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better
> > > for IMA to produce but that's inconsistent then.
> > Ok
> 
> One other question is whether IMA's audit calls should all adhere to
> CONFIG_INTEGRITY_AUDIT.

If I understand your question correctly, then no, since each one is a
different type of record, hence the half dozen IMA record types:
#define AUDIT_INTEGRITY_DATA1800 /* Data integrity verification */
#define AUDIT_INTEGRITY_METADATA1801 /* Metadata integrity verification */
#define AUDIT_INTEGRITY_STATUS  1802 /* Integrity enable status */
#define AUDIT_INTEGRITY_HASH1803 /* Integrity HASH type */
#define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */
#define AUDIT_INTEGRITY_RULE1805 /* policy rule */

> Most do but those two that currently use
> AUDIT_INTEGRITY_RULE do not. Should that be changed as well?

As far as I can tell, all the other IMA audit record types are fine.

>     Stefan

- 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: add containerid support for IMA-audit

2018-05-18 Thread Richard Guy Briggs

On 2018-05-18 10:39, Mimi Zohar wrote:
> On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:
> > On 05/18/2018 08:53 AM, Mimi Zohar wrote:
> 
> [..]
> 
> >  If so, which ones? 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?
> > >>> Changing the order of existing fields or inserting fields could break
> > >>> stuff and is strongly discouraged without a good reason, but appending
> > >>> fields is usually the right way to add information.
> > >>>
> > >>> There are exceptions, and in this case, I'd pick the "more standard" of
> > >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
> > >>> with that, abandoning the other format, renaming the less standard
> > >>> version of the record (ima_parse_rule?) and perhpas adopting that
> > >>> abandonned format for the new record type while using
> > >>> current->audit_context.
> > > This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> > > ima_audit_measurement().  Could we rename type=1805 to be
> > 
> > So do we want to change both? I thought that what 
> > ima_audit_measurement() produces looks ok but may not have a good name 
> > for the 'type'. Now in this case I would not want to 'break user space'.
> > The only change I was going to make was to what ima_parse_rule() produces.
> 
> The only change for now is separating the IMA policy rules from the
> IMA-audit messages.
> 
> Richard, when the containerid is appended to the IMA-audit messages,
> would we make the audit type name change then?

No, go ahead and make the change now.  I'm expecting that the
containerid record will just be another auxiliary record and should not
affect you folks.

> > > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
> > > message could be named INTEGRITY_RULE or, if that would be confusing,
> > > INTEGRITY_POLICY_RULE.
> > 
> > For 1806, as we would use it in ima_parse_rule(), we could change that 
> > in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better 
> > for IMA to produce but that's inconsistent then.
> 
> Ok
> 
> > 
> > >
> > >> 1806 would be in sync with INTEGRITY_RULE now for process related info.
> > >> If this looks good, I'll remove the dependency on your local context
> > >> creation and post the series.
> > >>
> > >> The justification for the change is that the INTEGRITY_RULE, as produced
> > >> by ima_parse_rule(), is broken.
> > > Post which series?  The IMA namespacing patch set?  This change should
> > > be upstreamed independently of IMA namespacing.
> > 
> > Without Richard's local context patch it may just be one or two patches.
> 
> Richard, if we separate the ima_parse_rules() audit messages, changing
> the audit rule number now, without the call to audit_log_task_info(),
> would adding the call later be breaking userspace?

Userspace is arguably already broken due to two formats and one usage
that isn't an auxiliary record.  All that should be necessary for now is
to use a different record number and pass it current->audit_context
instead of NULL.

> Mimi

- 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: add containerid support for IMA-audit

2018-05-18 Thread Richard Guy Briggs

On 2018-05-18 08:53, Mimi Zohar wrote:
> On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote:
> > On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:
> 
> [...]
> 
> > >>> 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
> > >> ima_parse_rule() is invoked when setting a policy by writing it into
> > >> /sys/kernel/security/ima/policy. We unfortunately don't have the
> > >> current->audit_context in this case.
> > > Sure you do.  What process writes to that file?  That's the one we care
> > > about, unless it is somehow handed off to a queue and processed later in
> > > a different context.
> > 
> > I just printk'd it again. current->audit_context is NULL in this case.
> 
> The builtin policy rules are loaded at __init.  Subsequently a custom
> policy can replace the builtin policy rules by writing to the
> securityfs file.  Is the audit_context NULL in both cases?

I wondered the same.

> > >> If so, which ones? 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?
> > > Changing the order of existing fields or inserting fields could break
> > > stuff and is strongly discouraged without a good reason, but appending
> > > fields is usually the right way to add information.
> > >
> > > There are exceptions, and in this case, I'd pick the "more standard" of
> > > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
> > > with that, abandoning the other format, renaming the less standard
> > > version of the record (ima_parse_rule?) and perhpas adopting that
> > > abandonned format for the new record type while using
> > > current->audit_context.
> 
> This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> ima_audit_measurement().  Could we rename type=1805 to be
> INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
> message could be named INTEGRITY_RULE or, if that would be confusing,
> INTEGRITY_POLICY_RULE.

I'm reluctant to change the macro/value.  Just use the existing
AUDIT_INTEGRITY_RULE1805 as appropriate and create a new
AUDIT_INTEGRITY_AUDIT   1806.

> > 1806 would be in sync with INTEGRITY_RULE now for process related info. 
> > If this looks good, I'll remove the dependency on your local context 
> > creation and post the series.
> > 
> > The justification for the change is that the INTEGRITY_RULE, as produced 
> > by ima_parse_rule(), is broken.
> 
> Post which series?  The IMA namespacing patch set?  This change should
> be upstreamed independently of IMA namespacing.
> 
> Mimi
> 

- 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: add containerid support for IMA-audit

2018-05-18 Thread Richard Guy Briggs

On 2018-05-18 07:49, Stefan Berger wrote:
> On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:
> > On 2018-05-17 10:18, Stefan Berger wrote:
> > > On 03/08/2018 06:21 AM, 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.
> > > What does 'detach it' mean? Does it mean we're not using
> > > current->audit_context?
> > Exactly.
> > 
> > > > 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
> > > Do you have an example (pointer) to the format for a 'syscall accompanied
> > > auxiliary record'?
> > Any that uses current->audit_context (or recently proposed
> > audit_context() function) will be a syscall auxiliary record.  Well
> > formed record formats are = and named as listed:
> > 
> > 
> > https://github.com/linux-audit/audit-documentation/wiki/SPEC-Writing-Good-Events
> > 
> > https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv
> > 
> > > > 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
> > > ima_parse_rule() is invoked when setting a policy by writing it into
> > > /sys/kernel/security/ima/policy. We unfortunately don't have the
> > > current->audit_context in this case.
> > Sure you do.  What process writes to that file?  That's the one we care
> > about, unless it is somehow handed off to a queue and processed later in
> > a different context.
> 
> I just printk'd it again. current->audit_context is NULL in this case.

Oops, that sounds like some of the netfilter empty table
initializations, whereas usually rules have a user actor.

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

2018-05-18 Thread Steve Grubb

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

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

2018-05-18 Thread Richard Guy Briggs

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?

> -Steve

- 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: add containerid support for IMA-audit

2018-05-18 Thread Stefan Berger


On 05/18/2018 10:39 AM, Mimi Zohar wrote:

On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:

On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]


If so, which ones? 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?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.

There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

This sounds right, other than "type=INTEGRITY_RULE" (1805) for
ima_audit_measurement().  Could we rename type=1805 to be

So do we want to change both? I thought that what
ima_audit_measurement() produces looks ok but may not have a good name
for the 'type'. Now in this case I would not want to 'break user space'.
The only change I was going to make was to what ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.

Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?


INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.

For 1806, as we would use it in ima_parse_rule(), we could change that
in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better
for IMA to produce but that's inconsistent then.

Ok


One other question is whether IMA's audit calls should all adhere to 
CONFIG_INTEGRITY_AUDIT. Most do but those two that currently use 
AUDIT_INTEGRITY_RULE do not. Should that be changed as well?


    Stefan

--
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-05-18 Thread Mimi Zohar

On Fri, 2018-05-18 at 09:54 -0400, Stefan Berger wrote:
> On 05/18/2018 08:53 AM, Mimi Zohar wrote:

[..]

>  If so, which ones? 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?
> >>> Changing the order of existing fields or inserting fields could break
> >>> stuff and is strongly discouraged without a good reason, but appending
> >>> fields is usually the right way to add information.
> >>>
> >>> There are exceptions, and in this case, I'd pick the "more standard" of
> >>> the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
> >>> with that, abandoning the other format, renaming the less standard
> >>> version of the record (ima_parse_rule?) and perhpas adopting that
> >>> abandonned format for the new record type while using
> >>> current->audit_context.
> > This sounds right, other than "type=INTEGRITY_RULE" (1805) for
> > ima_audit_measurement().  Could we rename type=1805 to be
> 
> So do we want to change both? I thought that what 
> ima_audit_measurement() produces looks ok but may not have a good name 
> for the 'type'. Now in this case I would not want to 'break user space'.
> The only change I was going to make was to what ima_parse_rule() produces.

The only change for now is separating the IMA policy rules from the
IMA-audit messages.

Richard, when the containerid is appended to the IMA-audit messages,
would we make the audit type name change then?

> 
> > INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
> > message could be named INTEGRITY_RULE or, if that would be confusing,
> > INTEGRITY_POLICY_RULE.
> 
> For 1806, as we would use it in ima_parse_rule(), we could change that 
> in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better 
> for IMA to produce but that's inconsistent then.

Ok

> 
> >
> >> 1806 would be in sync with INTEGRITY_RULE now for process related info.
> >> If this looks good, I'll remove the dependency on your local context
> >> creation and post the series.
> >>
> >> The justification for the change is that the INTEGRITY_RULE, as produced
> >> by ima_parse_rule(), is broken.
> > Post which series?  The IMA namespacing patch set?  This change should
> > be upstreamed independently of IMA namespacing.
> 
> Without Richard's local context patch it may just be one or two patches.

Richard, if we separate the ima_parse_rules() audit messages, changing
the audit rule number now, without the call to audit_log_task_info(),
would adding the call later be breaking userspace?

Mimi

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

Re: [PATCH ghak82] audit: Fix extended comparison of GID/EGID

2018-05-18 Thread Richard Guy Briggs

On 2018-05-18 09:44, Ondrej Mosnacek wrote:
> 2018-05-17 19:07 GMT+02:00 Richard Guy Briggs :
> > On 2018-05-17 17:31, Ondrej Mosnacek wrote:
> >> The audit_filter_rules() function in auditsc.c used the in_[e]group_p()
> >> functions to check GID/EGID match, but these functions use the current
> >> task's credentials, while the comparison should use the credentials of
> >> the task given to audit_filter_rules() as a parameter (tsk).
> >>
> >> Note that we can use group_search(cred->group_info, ...) as a
> >> replacement for both in_group_p and in_egroup_p as these functions only
> >> compare the parameter to cred->fsgid/egid and then call group_search.
> >>
> >> In fact, the usage of in_group_p was incorrect also because it compared
> >> to cred->fsgid and not cred->gid.
> >>
> >> GitHub issue:
> >> https://github.com/linux-audit/audit-kernel/issues/82
> >>
> >> Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")
> >> Cc: sta...@vger.kernel.org
> >> Signed-off-by: Ondrej Mosnacek 
> >
> > Nice.  You found a function that let you not have to roll a new global
> > one.  Is the comparision with cred->fsgid important?
> 
> To be honest, I don't really understand the exact purpose/meaning of
> all the different *GIDs, but since we have a separate field for
> comparing FSGID, I don't think it should be checked under GID.
> 
> What concerns me a bit though is that the check for 'extra' groups is
> now the same for GID and EGID... depending on the intended semantics
> of these credential fields (or of the corresponding audit fields),
> this may or may not be what we want. Maybe we should drop this
> extended checking altogether, or maybe do the same check for FSGID, or
> for a different subset of *GIDs... I will try to investigate this and
> figure out what is the right thing to do here.

fsgid may already be covered by another comparision function, but should
it be included in this one to avoid changing the potentially intended
coverage, or was it too broad to start with?  I don't know the answer.

I'd just add a check for fsgid too to be safe (or lazy) to avoid
changing the behaviour, if not doing the background research to find out
the intent.  There could be users depending on existing behaviour.

> > If you run ./scripts/checkpatch.pl on the patch file it will complain on
> > those lines longer than 80 chars.  I don't have a problem with it.
> 
> Yes, unfortunately it doesn't seem that splitting the lines would help
> much here... I'll see if I can rewrite the conditions in a simpler
> way.
> 
> >
> >
> >> ---
> >>  kernel/auditsc.c | 8 
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index cbab0da86d15..ec38e4d97c23 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -490,20 +490,20 @@ static int audit_filter_rules(struct task_struct 
> >> *tsk,
> >>   result = audit_gid_comparator(cred->gid, f->op, 
> >> f->gid);
> >>   if (f->op == Audit_equal) {
> >>   if (!result)
> >> - result = in_group_p(f->gid);
> >> + result = 
> >> groups_search(cred->group_info, f->gid);
> >>   } else if (f->op == Audit_not_equal) {
> >>   if (result)
> >> - result = !in_group_p(f->gid);
> >> + result = 
> >> !groups_search(cred->group_info, f->gid);
> >>   }
> >>   break;
> >>   case AUDIT_EGID:
> >>   result = audit_gid_comparator(cred->egid, f->op, 
> >> f->gid);
> >>   if (f->op == Audit_equal) {
> >>   if (!result)
> >> - result = in_egroup_p(f->gid);
> >> + result = 
> >> groups_search(cred->group_info, f->gid);
> >>   } else if (f->op == Audit_not_equal) {
> >>   if (result)
> >> - result = !in_egroup_p(f->gid);
> >> + result = 
> >> !groups_search(cred->group_info, f->gid);
> >>   }
> >>   break;
> >>   case AUDIT_SGID:
> >> --
> >> 2.17.0
> >>
> >
> > - 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
> 
> -- 
> Ondrej Mosnacek 
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

2018-05-18 Thread Steve Grubb

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

--
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-05-18 Thread Stefan Berger


On 05/18/2018 08:53 AM, Mimi Zohar wrote:

On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote:

On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]


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

ima_parse_rule() is invoked when setting a policy by writing it into
/sys/kernel/security/ima/policy. We unfortunately don't have the
current->audit_context in this case.

Sure you do.  What process writes to that file?  That's the one we care
about, unless it is somehow handed off to a queue and processed later in
a different context.

I just printk'd it again. current->audit_context is NULL in this case.

The builtin policy rules are loaded at __init.  Subsequently a custom
policy can replace the builtin policy rules by writing to the
securityfs file.  Is the audit_context NULL in both cases?


The builtin policy rules are not parsed from what I can see. They seem 
to be encoded in the format the parser would produce.


I get current->audit_context == NULL in the case the user cat's the 
policy into IMA's policy securityfs file.







If so, which ones? 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?

Changing the order of existing fields or inserting fields could break
stuff and is strongly discouraged without a good reason, but appending
fields is usually the right way to add information.

There are exceptions, and in this case, I'd pick the "more standard" of
the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
with that, abandoning the other format, renaming the less standard
version of the record (ima_parse_rule?) and perhpas adopting that
abandonned format for the new record type while using
current->audit_context.

This sounds right, other than "type=INTEGRITY_RULE" (1805) for
ima_audit_measurement().  Could we rename type=1805 to be


So do we want to change both? I thought that what 
ima_audit_measurement() produces looks ok but may not have a good name 
for the 'type'. Now in this case I would not want to 'break user space'. 
The only change I was going to make was to what ima_parse_rule() produces.



INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.


For 1806, as we would use it in ima_parse_rule(), we could change that 
in your patch to INTEGRITY_POLICY_RULE. IMA_POLICY_RULE may be better 
for IMA to produce but that's inconsistent then.





1806 would be in sync with INTEGRITY_RULE now for process related info.
If this looks good, I'll remove the dependency on your local context
creation and post the series.

The justification for the change is that the INTEGRITY_RULE, as produced
by ima_parse_rule(), is broken.

Post which series?  The IMA namespacing patch set?  This change should
be upstreamed independently of IMA namespacing.


Without Richard's local context patch it may just be one or two patches.

   Stefan



Mimi



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

Re: [PATCH ghak81 V3a] fixup! audit: collect audit task parameters

2018-05-18 Thread Ingo Molnar


* Richard Guy Briggs  wrote:

> Enable fork.c compilation with audit disabled.
> 
> Signed-off-by: Richard Guy Briggs 
> ---
> Hi Paul, this one got caught by the 0-day kbuildbot.  Can you squash it
> down if you haven't merged it yet?
> ---
>  kernel/fork.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 92ab849..ff82928 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1713,7 +1713,9 @@ static __latent_entropy struct task_struct 
> *copy_process(
>   p->start_time = ktime_get_ns();
>   p->real_start_time = ktime_get_boot_ns();
>   p->io_context = NULL;
> +#ifdef CONFIG_AUDITSYSCALL
>   p->audit = NULL;
> +#endif /* CONFIG_AUDITSYSCALL */

Please, simply use:

  #endif

the comment is used for (much) larger blocks, to make it clear which block ends 
there if the top is not visible.

Thanks,

Ingo

--
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-05-18 Thread Mimi Zohar

On Fri, 2018-05-18 at 07:49 -0400, Stefan Berger wrote:
> On 05/17/2018 05:30 PM, Richard Guy Briggs wrote:

[...]

> >>> 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
> >> ima_parse_rule() is invoked when setting a policy by writing it into
> >> /sys/kernel/security/ima/policy. We unfortunately don't have the
> >> current->audit_context in this case.
> > Sure you do.  What process writes to that file?  That's the one we care
> > about, unless it is somehow handed off to a queue and processed later in
> > a different context.
> 
> I just printk'd it again. current->audit_context is NULL in this case.

The builtin policy rules are loaded at __init.  Subsequently a custom
policy can replace the builtin policy rules by writing to the
securityfs file.  Is the audit_context NULL in both cases?



> >> If so, which ones? 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?
> > Changing the order of existing fields or inserting fields could break
> > stuff and is strongly discouraged without a good reason, but appending
> > fields is usually the right way to add information.
> >
> > There are exceptions, and in this case, I'd pick the "more standard" of
> > the formats for AUDIT_INTEGRITY_RULE (ima_audit_measurement?) and stick
> > with that, abandoning the other format, renaming the less standard
> > version of the record (ima_parse_rule?) and perhpas adopting that
> > abandonned format for the new record type while using
> > current->audit_context.

This sounds right, other than "type=INTEGRITY_RULE" (1805) for
ima_audit_measurement().  Could we rename type=1805 to be
INTEGRITY_AUDIT or INTEGRITY_IMA_AUDIT?  The new type=1806 audit
message could be named INTEGRITY_RULE or, if that would be confusing,
INTEGRITY_POLICY_RULE.


> 1806 would be in sync with INTEGRITY_RULE now for process related info. 
> If this looks good, I'll remove the dependency on your local context 
> creation and post the series.
> 
> The justification for the change is that the INTEGRITY_RULE, as produced 
> by ima_parse_rule(), is broken.

Post which series?  The IMA namespacing patch set?  This change should
be upstreamed independently of IMA namespacing.

Mimi

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