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

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

[Mimi Zohar]

Hi Richard,

This patch has been compiled, but not runtime tested.

---

If the containerid is defined, include it in the IMA-audit record.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 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;
-- 
2.7.5

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

[Richard Guy Briggs]

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?  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.  Can you suggest a procedure to test it?

> ---
> 
> If the containerid is defined, include it in the IMA-audit record.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  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;
> -- 
> 2.7.5
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Mimi Zohar]

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.  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 <zohar@linux.vnet.ibm.com>
> > ---
> >  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;
> > -- 
> > 2.7.5
> > 
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
> 

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

[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 <zohar@linux.vnet.ibm.com>
> > > ---
> > >  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;
> > > -- 
> > > 2.7.5
> > > 
> > 
> > - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[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

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

[Richard Guy Briggs]

On 2018-03-08 13:02, Mimi Zohar wrote:
> 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.

I would argue this type of message/record should be converted to an
accompanied syscall record, or have the subject attributes added to the
record so that it is clear what user/process initiated this action.  It
now occurs to me that to save audit communications and disk bandwidth,
one syscall record could accompany all the rule records, but if the list
is long enough it might overwhelm userspace audit event parsing code.
Steve?

Regardless, the ima_parse_rule() record format needs to be fixed to
address the non-standard use of "<" and ">" operators instead of the
standard "=" field/value separator.

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

Ok, so this might be one circumstance where the container id field
could make sense to include it in the primary record, but it will
already be automatically getting an AUDIT_CONTAINER_INFO record by
virtue of being a syscall auxiliary record.  You say "These records are
probably being used independently of any other audit records.", but
unless you are certain these are not used by any other tools, it will
have to remain as a syscall auxiliary record.  This means it will
duplicate the container id info if the container id field is added to
the AUDIT_INTEGRITY_RULE record or cause your tools to need to be able
to parse audit *events* rather than just individual audit *records* to
get the associated container id if the container id field is not added
to the AUDIT_INTEGRITY_RULE.

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

This makes sense to me, but will depend on other users of this record
type.  Since there are already two very different formats for this one
existing record type, changing the record type either doesn't matter if
nothing else has noticed or this is what triggered the examination of
this issue in the first place and should be fixed.

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

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Stefan Berger]

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?

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

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

> audit_log_container_info() then releasing the local context.  This
> version of the record has additional concerns covered here:
> https://github.com/linux-audit/audit-kernel/issues/52

Following the discussion there and the concern with breaking user space, 
how can we split up the AUDIT_INTEGRITY_RULE that is used in 
ima_audit_measurement() and ima_parse_rule(), without 'breaking user space'?

A message produced by ima_parse_rule() looks like this here:

type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure" 
fsmagic="0x9fa0" res=1

in contrast to that an INTEGRITY_PCR record type:

type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0 
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 
op="invalid_pcr" cause="open_writers" comm="scp" 
name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1

Should some of the fields from INTEGRITY_PCR also appear in 
INTEGRITY_RULE? 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?

     Stefan


>
> 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 <zohar@linux.vnet.ibm.com>
>>>> ---
>>>>   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;
>>>> -- 
>>>> 2.7.5
>>>>
>>> - RGB
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>

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

[Richard Guy Briggs]

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 <fieldname>=<value> 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.

> > 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 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> op="invalid_pcr" cause="open_writers" comm="scp"
> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> 
> Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?

Not necessarily.  There are a number of records in the PCR record that
would be redundant when connected to a syscall record, but removing them
is discouraged to avoid breaking parsers that expect them.

I don't see any need to touch the PCR record.

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

Does this help?

>     Stefan
> 
> > 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 <zohar@linux.vnet.ibm.com>
> > > > > ---
> > > > >   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;
> > > > > -- 
> > > > > 2.7.5
> > > > > 
> > > > - RGB
> > - RGB
> > 
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> > 
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Stefan Berger]

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 <fieldname>=<value> 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.

>
>>> 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 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> op="invalid_pcr" cause="open_writers" comm="scp"
>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
>>
>> Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?
> Not necessarily.  There are a number of records in the PCR record that
> would be redundant when connected to a syscall record, but removing them
> is discouraged to avoid breaking parsers that expect them.
>
> I don't see any need to touch the PCR record.

I wasn't going to touch the PCR record.


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

Since current->audit_context is NULL I built on your patch, but I am not 
sure whether it is justifyable to use that before your container id 
series is applied.

This is what ima_parse_rule() produces now after having it call 
audit_log_task_info() as well and by introducing 1806 for 
ima_parse_rule() only ( https://lkml.org/lkml/2018/5/11/386 ):

type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure" 
fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 
suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" 
exe="/usr/bin/cat" 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023

[before: type=INTEGRITY_RULE msg=audit(1526566213.870:305): 
action="dont_measure" fsmagic="0x9fa0" res=1]

For comparison the INTEGRITY_RULE:

type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" 
hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b" 
ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 
sgid=0 fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp" 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023


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.

     Stefan


>
> Does this help?
>
>>      Stefan
>>
>>> 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 <zohar@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>    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;
>>>>>> -- 
>>>>>> 2.7.5
>>>>>>
>>>>> - RGB
>>> - RGB
>>>
>>> --
>>> Richard Guy Briggs <rgb@redhat.com>
>>> Sr. S/W Engineer, Kernel Security, Base Operating Systems
>>> Remote, Ottawa, Red Hat Canada
>>> IRC: rgb, SunRaycer
>>> Voice: +1.647.777.2635, Internal: (81) 32635
>>>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>

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

[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

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

[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

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

[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

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

[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

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

[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_DATA        1800 /* Data integrity verification */
#define AUDIT_INTEGRITY_METADATA    1801 /* Metadata integrity verification */
#define AUDIT_INTEGRITY_STATUS      1802 /* Integrity enable status */
#define AUDIT_INTEGRITY_HASH        1803 /* Integrity HASH type */
#define AUDIT_INTEGRITY_PCR         1804 /* PCR invalidation msgs */
#define AUDIT_INTEGRITY_RULE        1805 /* 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 <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[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 <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[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

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

[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 <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Steve Grubb]

On Friday, May 18, 2018 12:34:24 PM EDT 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?

>From a user space perspective, I don't care as long the event is well formed 
(No unnecessary untrusted string logging) and we have the required fields for 
searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the 
object is identifiable in the event.

-Steve

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

[Stefan Berger]

On 05/21/2018 01:21 PM, Steve Grubb wrote:
> On Friday, May 18, 2018 12:34:24 PM EDT 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?
>  From a user space perspective, I don't care as long the event is well formed

Are you saying this because of the tools you referred to that require 
proper ordering and all fields need to be available?

> (No unnecessary untrusted string logging) and we have the required fields for
> searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And the
> object is identifiable in the event.

There's at least one documented user that showed the existing format...

https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.html

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

[Steve Grubb]

Hello Stefan,

On Monday, May 21, 2018 2:04:08 PM EDT Stefan Berger wrote:
> On 05/21/2018 01:21 PM, Steve Grubb wrote:
> > On Friday, May 18, 2018 12:34:24 PM EDT 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?
> >> 
> >  From a user space perspective, I don't care as long the event is well
> >  formed
>
> Are you saying this because of the tools you referred to that require
> proper ordering and all fields need to be available?

Its because the order was established a long time ago. User space tools still 
expect that ordering.
 
> > (No unnecessary untrusted string logging) and we have the required fields
> > for searching: pid, uid, auid, tty, session, subj, comm, exe, & res. And
> > the object is identifiable in the event.
> 
> There's at least one documented user that showed the existing format...
> 
> https://www.fireeye.com/blog/threat-research/2016/11/extending_linux_exec.h
> tml

Hmm. I see. If that event was ever sent to linux-audit mail list for 
feedback, we would have had some discussion because it's not a proper event. 
The order of the fields changed, new fields were added, untrusted string 
logging is being used, field names are not documented in the field name 
dictionary, existing field names are not being used, subject information is 
incomplete, results is missing, and its not entirely clear what the object 
is.

-Steve

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

[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_RULE        1805 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 <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[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 <fieldname>=<value> 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 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 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > op="invalid_pcr" cause="open_writers" comm="scp"
> > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> > > 
> > > Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?
> > Not necessarily.  There are a number of records in the PCR record that
> > would be redundant when connected to a syscall record, but removing them
> > is discouraged to avoid breaking parsers that expect them.
> > 
> > I don't see any need to touch the PCR record.
> 
> I wasn't going to touch the PCR record.
> 
> 
> > > 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.
> 
> Since current->audit_context is NULL I built on your patch, but I am not
> sure whether it is justifyable to use that before your container id series
> is applied.
> 
> This is what ima_parse_rule() produces now after having it call
> audit_log_task_info() as well and by introducing 1806 for ima_parse_rule()
> only ( https://lkml.org/lkml/2018/5/11/386 ):
> 
> type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure"
> fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0
> fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> 
> [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305):
> action="dont_measure" fsmagic="0x9fa0" res=1]

Since this appears to be a a user action, use current->audit_context
to make it a syscall auxiliary record rather than adding all these
redundant fields.

> For comparison the INTEGRITY_RULE:
> 
> type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b"
> ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> 
> 
> 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.

What would be the macro name for 1806?

> The justification for the change is that the INTEGRITY_RULE, as produced by
> ima_parse_rule(), is broken.
> 
>     Stefan
> 
> 
> > 
> > Does this help?
> > 
> > >      Stefan
> > > 
> > > > 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 <zohar@linux.vnet.ibm.com>
> > > > > > > ---
> > > > > > >    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;
> > > > > > > -- 
> > > > > > > 2.7.5
> > > > > > > 
> > > > > > - RGB
> > > > - RGB
> > > > 
> > > > --
> > > > Richard Guy Briggs <rgb@redhat.com>
> > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > > Remote, Ottawa, Red Hat Canada
> > > > IRC: rgb, SunRaycer
> > > > Voice: +1.647.777.2635, Internal: (81) 32635
> > > > 
> > - RGB
> > 
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> > 
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[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 <fieldname>=<value> 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 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>>> op="invalid_pcr" cause="open_writers" comm="scp"
>>>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
>>>>
>>>> Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?
>>> Not necessarily.  There are a number of records in the PCR record that
>>> would be redundant when connected to a syscall record, but removing them
>>> is discouraged to avoid breaking parsers that expect them.
>>>
>>> I don't see any need to touch the PCR record.
>> I wasn't going to touch the PCR record.
>>
>>
>>>> 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.
>> Since current->audit_context is NULL I built on your patch, but I am not
>> sure whether it is justifyable to use that before your container id series
>> is applied.
>>
>> This is what ima_parse_rule() produces now after having it call
>> audit_log_task_info() as well and by introducing 1806 for ima_parse_rule()
>> only ( https://lkml.org/lkml/2018/5/11/386 ):
>>
>> type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure"
>> fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0
>> fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat"
>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>
>> [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305):
>> action="dont_measure" fsmagic="0x9fa0" res=1]
> Since this appears to be a a user action, use current->audit_context
> to make it a syscall auxiliary record rather than adding all these
> redundant fields.

Sure, once we have a non-NULL pointer in current->audit_context I'd do that.

>
>> For comparison the INTEGRITY_RULE:
>>
>> type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b"
>> ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
>> fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp"
>> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>
>>
>> 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.
> What would be the macro name for 1806?

I currently work with AUDIT_INTEGRITY_POLICY_RULE.


>
>> The justification for the change is that the INTEGRITY_RULE, as produced by
>> ima_parse_rule(), is broken.
>>
>>      Stefan
>>
>>
>>> Does this help?
>>>
>>>>       Stefan
>>>>
>>>>> 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 <zohar@linux.vnet.ibm.com>
>>>>>>>> ---
>>>>>>>>     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;
>>>>>>>> -- 
>>>>>>>> 2.7.5
>>>>>>>>
>>>>>>> - RGB
>>>>> - RGB
>>>>>
>>>>> --
>>>>> Richard Guy Briggs <rgb@redhat.com>
>>>>> Sr. S/W Engineer, Kernel Security, Base Operating Systems
>>>>> Remote, Ottawa, Red Hat Canada
>>>>> IRC: rgb, SunRaycer
>>>>> Voice: +1.647.777.2635, Internal: (81) 32635
>>>>>
>>> - RGB
>>>
>>> --
>>> Richard Guy Briggs <rgb@redhat.com>
>>> Sr. S/W Engineer, Kernel Security, Base Operating Systems
>>> Remote, Ottawa, Red Hat Canada
>>> IRC: rgb, SunRaycer
>>> Voice: +1.647.777.2635, Internal: (81) 32635
>>>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>

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

[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 <fieldname>=<value> 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?'

Er, it is a challenge we're aware of and have already solved a number of
them.

What happens if you boot with "audit=1" on the kernel command line?

> > > > > > 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 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > > > op="invalid_pcr" cause="open_writers" comm="scp"
> > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> > > > > 
> > > > > Should some of the fields from INTEGRITY_PCR also appear in INTEGRITY_RULE?
> > > > Not necessarily.  There are a number of records in the PCR record that
> > > > would be redundant when connected to a syscall record, but removing them
> > > > is discouraged to avoid breaking parsers that expect them.
> > > > 
> > > > I don't see any need to touch the PCR record.
> > > I wasn't going to touch the PCR record.
> > > 
> > > 
> > > > > 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.
> > > Since current->audit_context is NULL I built on your patch, but I am not
> > > sure whether it is justifyable to use that before your container id series
> > > is applied.
> > > 
> > > This is what ima_parse_rule() produces now after having it call
> > > audit_log_task_info() as well and by introducing 1806 for ima_parse_rule()
> > > only ( https://lkml.org/lkml/2018/5/11/386 ):
> > > 
> > > type=UNKNOWN[1806] msg=audit(1526643702.328:304): action="dont_measure"
> > > fsmagic="0x9fa0" res=1 ppid=1563 pid=1595 auid=0 uid=0 gid=0 euid=0 suid=0
> > > fsuid=0 egid=0 sgid=0 fsgid=0 tty=tty2 ses=2 comm="cat" exe="/usr/bin/cat"
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > 
> > > [before: type=INTEGRITY_RULE msg=audit(1526566213.870:305):
> > > action="dont_measure" fsmagic="0x9fa0" res=1]
> > Since this appears to be a a user action, use current->audit_context
> > to make it a syscall auxiliary record rather than adding all these
> > redundant fields.
> 
> Sure, once we have a non-NULL pointer in current->audit_context I'd do that.
> 
> > 
> > > For comparison the INTEGRITY_RULE:
> > > 
> > > type=INTEGRITY_RULE msg=audit(1526642504.074:331): file="/usr/bin/ssh" hash="sha256:4abc2558424b9ca61c34af43169d9b9e174d7825bf60c9c76be377549081db5b"
> > > ppid=1623 pid=1624 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > > fsgid=0 tty=tty2 ses=2 comm="scp" exe="/usr/bin/scp"
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > 
> > > 
> > > 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.
> > What would be the macro name for 1806?
> 
> I currently work with AUDIT_INTEGRITY_POLICY_RULE.
> 
> 
> > 
> > > The justification for the change is that the INTEGRITY_RULE, as produced by
> > > ima_parse_rule(), is broken.
> > > 
> > >      Stefan
> > > 
> > > 
> > > > Does this help?
> > > > 
> > > > >       Stefan
> > > > > 
> > > > > > 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 <zohar@linux.vnet.ibm.com>
> > > > > > > > > ---
> > > > > > > > >     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;
> > > > > > > > > -- 
> > > > > > > > > 2.7.5
> > > > > > > > > 
> > > > > > > > - RGB
> > > > > > - RGB
> > > > > > 
> > > > > > --
> > > > > > Richard Guy Briggs <rgb@redhat.com>
> > > > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > > > > Remote, Ottawa, Red Hat Canada
> > > > > > IRC: rgb, SunRaycer
> > > > > > Voice: +1.647.777.2635, Internal: (81) 32635
> > > > > > 
> > > > - RGB
> > > > 
> > > > --
> > > > Richard Guy Briggs <rgb@redhat.com>
> > > > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > > > Remote, Ottawa, Red Hat Canada
> > > > IRC: rgb, SunRaycer
> > > > Voice: +1.647.777.2635, Internal: (81) 32635
> > > > 
> > - RGB
> > 
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Sr. S/W Engineer, Kernel Security, Base Operating Systems
> > Remote, Ottawa, Red Hat Canada
> > IRC: rgb, SunRaycer
> > Voice: +1.647.777.2635, Internal: (81) 32635
> > 
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Steve Grubb]

On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
> > audit_log_container_info() then releasing the local context.  This
> > version of the record has additional concerns covered here:
> > https://github.com/linux-audit/audit-kernel/issues/52
> 
> Following the discussion there and the concern with breaking user space,
> how can we split up the AUDIT_INTEGRITY_RULE that is used in
> ima_audit_measurement() and ima_parse_rule(), without 'breaking user
> space'?
> 
> A message produced by ima_parse_rule() looks like this here:
> 
> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
> fsmagic="0x9fa0" res=1

Why is action and fsmagic being logged as untrusted strings? Untrusted 
strings are used when an unprivileged user can affect the contents of the 
field such as creating a file with space or special characters in the name.

Also, subject and object information is missing. Who loaded this rule?

> in contrast to that an INTEGRITY_PCR record type:
> 
> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> op="invalid_pcr" cause="open_writers" comm="scp"
> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1

Why is op & cause being logged as an untrusted string? This also has 
incomplete subject information.

> Should some of the fields from INTEGRITY_PCR also appear in
> INTEGRITY_RULE? If so, which ones? 

pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are required to 
be searchable

> We could probably refactor the current  integrity_audit_message() and have
> ima_parse_rule() call into it to get those fields as well. I suppose adding
> new fields to it wouldn't be considered breaking user space?

The audit user space utilities pretty much expects those fields in that order 
for any IMA originating events. You can add things like op or cause before 
that. The reason why you can do that is those additional fields are not 
required to be searchable by common criteria.

-Steve

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

[Stefan Berger]

On 05/21/2018 12:58 PM, Steve Grubb wrote:
> On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
>>> audit_log_container_info() then releasing the local context.  This
>>> version of the record has additional concerns covered here:
>>> https://github.com/linux-audit/audit-kernel/issues/52
>> Following the discussion there and the concern with breaking user space,
>> how can we split up the AUDIT_INTEGRITY_RULE that is used in
>> ima_audit_measurement() and ima_parse_rule(), without 'breaking user
>> space'?
>>
>> A message produced by ima_parse_rule() looks like this here:
>>
>> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
>> fsmagic="0x9fa0" res=1
> Why is action and fsmagic being logged as untrusted strings? Untrusted
> strings are used when an unprivileged user can affect the contents of the
> field such as creating a file with space or special characters in the name.
>
> Also, subject and object information is missing. Who loaded this rule?
>
>> in contrast to that an INTEGRITY_PCR record type:
>>
>> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
>> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>> op="invalid_pcr" cause="open_writers" comm="scp"
>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> Why is op & cause being logged as an untrusted string? This also has
> incomplete subject information.

It's calling audit_log_string() in both cases:

https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity_audit.c#L48

>
>> Should some of the fields from INTEGRITY_PCR also appear in
>> INTEGRITY_RULE? If so, which ones?
> pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are required to
> be searchable
>
>> We could probably refactor the current  integrity_audit_message() and have
>> ima_parse_rule() call into it to get those fields as well. I suppose adding
>> new fields to it wouldn't be considered breaking user space?
> The audit user space utilities pretty much expects those fields in that order
> for any IMA originating events. You can add things like op or cause before

We will call into audit_log_task, which will put the parameters into 
correct order:

auid uid gid ses subj pid comm exe

https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433




> that. The reason why you can do that is those additional fields are not
> required to be searchable by common criteria.
>
> -Steve
>
>

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

[Steve Grubb]

Hello Stefan,

On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote:
> On 05/21/2018 12:58 PM, Steve Grubb wrote:
> > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
> >>> audit_log_container_info() then releasing the local context.  This
> >>> version of the record has additional concerns covered here:
> >>> https://github.com/linux-audit/audit-kernel/issues/52
> >> 
> >> Following the discussion there and the concern with breaking user space,
> >> how can we split up the AUDIT_INTEGRITY_RULE that is used in
> >> ima_audit_measurement() and ima_parse_rule(), without 'breaking user
> >> space'?
> >> 
> >> A message produced by ima_parse_rule() looks like this here:
> >> 
> >> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
> >> fsmagic="0x9fa0" res=1
> > 
> > Why is action and fsmagic being logged as untrusted strings? Untrusted
> > strings are used when an unprivileged user can affect the contents of the
> > field such as creating a file with space or special characters in the
> > name.
> > 
> > Also, subject and object information is missing. Who loaded this rule?
> > 
> >> in contrast to that an INTEGRITY_PCR record type:
> >> 
> >> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
> >> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> >> op="invalid_pcr" cause="open_writers" comm="scp"
> >> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> > 
> > Why is op & cause being logged as an untrusted string? This also has
> > incomplete subject information.
> 
> It's calling audit_log_string() in both cases:
> 
> https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity
> _audit.c#L48

I see. Looking things over, I see that it seems like it should do the right 
thing. But the original purpose for this function is here:

https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944

This is where it is logging an untrusted string and has to decide to encode 
it or just place it in quotes. If it has quotes, that means it's an untrusted 
string but has no control characters in it. I think you want to use 
audit_log_format() for any string that is trustworthy.


As an aside, I wonder why audit_log_string() is in the API when it is a 
helper to audit_log_untrustedstring() ?  Without understanding the rules of 
untrusted strings, it can't be used correctly without re-inventing 
audit_log_untrustedstring() by hand.


> >> Should some of the fields from INTEGRITY_PCR also appear in
> >> INTEGRITY_RULE? If so, which ones?
> > 
> > pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are
> > required to be searchable
> > 
> >> We could probably refactor the current  integrity_audit_message() and
> >> have ima_parse_rule() call into it to get those fields as well. I
> >> suppose adding new fields to it wouldn't be considered breaking user
> >> space?
> > 
> > The audit user space utilities pretty much expects those fields in that
> > order for any IMA originating events. You can add things like op or
> > cause before
>
> We will call into audit_log_task, which will put the parameters into
> correct order:
> 
> auid uid gid ses subj pid comm exe

I'm telling you what the correct order is.  :-)  A long time ago, the IMA 
system had audit events with the order I'm mentioning. For example, here's 
one from a log I collected back in 2013:

type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295 
ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added" 
comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0

it was missing "tty" and "exe", but the order is as I mentioned. The 
expectation is that INTEGRITY events maintain this established order across 
all events.

Thanks,
-Steve


> https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433
> 
> > that. The reason why you can do that is those additional fields are not
> > required to be searchable by common criteria.
> > 
> > -Steve

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

[Stefan Berger]

On 05/21/2018 02:30 PM, Steve Grubb wrote:
> Hello Stefan,
>
> On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote:
>> On 05/21/2018 12:58 PM, Steve Grubb wrote:
>>> On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
>>>>> audit_log_container_info() then releasing the local context.  This
>>>>> version of the record has additional concerns covered here:
>>>>> https://github.com/linux-audit/audit-kernel/issues/52
>>>> Following the discussion there and the concern with breaking user space,
>>>> how can we split up the AUDIT_INTEGRITY_RULE that is used in
>>>> ima_audit_measurement() and ima_parse_rule(), without 'breaking user
>>>> space'?
>>>>
>>>> A message produced by ima_parse_rule() looks like this here:
>>>>
>>>> type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
>>>> fsmagic="0x9fa0" res=1
>>> Why is action and fsmagic being logged as untrusted strings? Untrusted
>>> strings are used when an unprivileged user can affect the contents of the
>>> field such as creating a file with space or special characters in the
>>> name.
>>>
>>> Also, subject and object information is missing. Who loaded this rule?
>>>
>>>> in contrast to that an INTEGRITY_PCR record type:
>>>>
>>>> type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
>>>> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>>> op="invalid_pcr" cause="open_writers" comm="scp"
>>>> name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
>>> Why is op & cause being logged as an untrusted string? This also has
>>> incomplete subject information.
>> It's calling audit_log_string() in both cases:
>>
>> https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity
>> _audit.c#L48
> I see. Looking things over, I see that it seems like it should do the right
> thing. But the original purpose for this function is here:
>
> https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944
>
> This is where it is logging an untrusted string and has to decide to encode
> it or just place it in quotes. If it has quotes, that means it's an untrusted
> string but has no control characters in it. I think you want to use
> audit_log_format() for any string that is trustworthy.

Replacing all occurrences (in IMA) of audit_log_string() with 
audit_log_format().
>
>
> As an aside, I wonder why audit_log_string() is in the API when it is a
> helper to audit_log_untrustedstring() ?  Without understanding the rules of
> untrusted strings, it can't be used correctly without re-inventing
> audit_log_untrustedstring() by hand.
>
>
>>>> Should some of the fields from INTEGRITY_PCR also appear in
>>>> INTEGRITY_RULE? If so, which ones?
>>> pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are
>>> required to be searchable
>>>
>>>> We could probably refactor the current  integrity_audit_message() and
>>>> have ima_parse_rule() call into it to get those fields as well. I
>>>> suppose adding new fields to it wouldn't be considered breaking user
>>>> space?
>>> The audit user space utilities pretty much expects those fields in that
>>> order for any IMA originating events. You can add things like op or
>>> cause before
>> We will call into audit_log_task, which will put the parameters into
>> correct order:
>>
>> auid uid gid ses subj pid comm exe
> I'm telling you what the correct order is.  :-)  A long time ago, the IMA

:-) Thanks. Was getting confused.

> system had audit events with the order I'm mentioning. For example, here's
> one from a log I collected back in 2013:
>
> type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295
> ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added"
> comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0
>
> it was missing "tty" and "exe", but the order is as I mentioned. The
> expectation is that INTEGRITY events maintain this established order across
> all events.

I am *appending* exe= and tty= now:

type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0 
ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 
op="invalid_pcr" cause="open_writers" comm="ssh" 
name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1 
exe="/usr/bin/ssh" tty=tty2


    Stefan


>
> Thanks,
> -Steve
>
>
>> https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433
>>
>>> that. The reason why you can do that is those additional fields are not
>>> required to be searchable by common criteria.
>>>
>>> -Steve
>
>
>

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

[Richard Guy Briggs]

On 2018-05-21 17:57, Stefan Berger wrote:
> On 05/21/2018 02:30 PM, Steve Grubb wrote:
> > Hello Stefan,
> > 
> > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote:
> > > On 05/21/2018 12:58 PM, Steve Grubb wrote:
> > > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
> > > > > > audit_log_container_info() then releasing the local context.  This
> > > > > > version of the record has additional concerns covered here:
> > > > > > https://github.com/linux-audit/audit-kernel/issues/52
> > > > > Following the discussion there and the concern with breaking user space,
> > > > > how can we split up the AUDIT_INTEGRITY_RULE that is used in
> > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking user
> > > > > space'?
> > > > > 
> > > > > A message produced by ima_parse_rule() looks like this here:
> > > > > 
> > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305): action="dont_measure"
> > > > > fsmagic="0x9fa0" res=1
> > > > Why is action and fsmagic being logged as untrusted strings? Untrusted
> > > > strings are used when an unprivileged user can affect the contents of the
> > > > field such as creating a file with space or special characters in the
> > > > name.
> > > > 
> > > > Also, subject and object information is missing. Who loaded this rule?
> > > > 
> > > > > in contrast to that an INTEGRITY_PCR record type:
> > > > > 
> > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0 auid=0
> > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > > > op="invalid_pcr" cause="open_writers" comm="scp"
> > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> > > > Why is op & cause being logged as an untrusted string? This also has
> > > > incomplete subject information.
> > > It's calling audit_log_string() in both cases:
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/security/integrity/integrity
> > > _audit.c#L48
> > I see. Looking things over, I see that it seems like it should do the right
> > thing. But the original purpose for this function is here:
> > 
> > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944
> > 
> > This is where it is logging an untrusted string and has to decide to encode
> > it or just place it in quotes. If it has quotes, that means it's an untrusted
> > string but has no control characters in it. I think you want to use
> > audit_log_format() for any string that is trustworthy.
> 
> Replacing all occurrences (in IMA) of audit_log_string() with
> audit_log_format().
> > 
> > 
> > As an aside, I wonder why audit_log_string() is in the API when it is a
> > helper to audit_log_untrustedstring() ?  Without understanding the rules of
> > untrusted strings, it can't be used correctly without re-inventing
> > audit_log_untrustedstring() by hand.
> > 
> > 
> > > > > Should some of the fields from INTEGRITY_PCR also appear in
> > > > > INTEGRITY_RULE? If so, which ones?
> > > > pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are
> > > > required to be searchable
> > > > 
> > > > > We could probably refactor the current  integrity_audit_message() and
> > > > > have ima_parse_rule() call into it to get those fields as well. I
> > > > > suppose adding new fields to it wouldn't be considered breaking user
> > > > > space?
> > > > The audit user space utilities pretty much expects those fields in that
> > > > order for any IMA originating events. You can add things like op or
> > > > cause before
> > > We will call into audit_log_task, which will put the parameters into
> > > correct order:
> > > 
> > > auid uid gid ses subj pid comm exe
> > I'm telling you what the correct order is.  :-)  A long time ago, the IMA
> 
> :-) Thanks. Was getting confused.
> 
> > system had audit events with the order I'm mentioning. For example, here's
> > one from a log I collected back in 2013:
> > 
> > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0 auid=4294967295
> > ses=4294967295 subj=kernel op="add_template_measure" cause="hash_added"
> > comm="init" name="01parse-kernel.sh" dev=rootfs ino=5413 res=0
> > 
> > it was missing "tty" and "exe", but the order is as I mentioned. The
> > expectation is that INTEGRITY events maintain this established order across
> > all events.
> 
> I am *appending* exe= and tty= now:
> 
> type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0
> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> op="invalid_pcr" cause="open_writers" comm="ssh"
> name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1
> exe="/usr/bin/ssh" tty=tty2

This isn't necessary since they already covered in the already
connected SYSCALL record which duplicates even more information than is
already.

>    Stefan
> 
> > -Steve
> > 
> > > https://elixir.bootlin.com/linux/latest/source/kernel/auditsc.c#L2433
> > > 
> > > > that. The reason why you can do that is those additional fields are not
> > > > required to be searchable by common criteria.
> > > > 
> > > > -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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

[Steve Grubb]

On Tuesday, May 22, 2018 9:43:46 AM EDT Richard Guy Briggs wrote:
> On 2018-05-21 17:57, Stefan Berger wrote:
> > On 05/21/2018 02:30 PM, Steve Grubb wrote:
> > > Hello Stefan,
> > > 
> > > On Monday, May 21, 2018 1:53:04 PM EDT Stefan Berger wrote:
> > > > On 05/21/2018 12:58 PM, Steve Grubb wrote:
> > > > > On Thursday, May 17, 2018 10:18:13 AM EDT Stefan Berger wrote:
> > > > > > > audit_log_container_info() then releasing the local context. 
> > > > > > > This
> > > > > > > version of the record has additional concerns covered here:
> > > > > > > https://github.com/linux-audit/audit-kernel/issues/52
> > > > > > 
> > > > > > Following the discussion there and the concern with breaking user
> > > > > > space,
> > > > > > how can we split up the AUDIT_INTEGRITY_RULE that is used in
> > > > > > ima_audit_measurement() and ima_parse_rule(), without 'breaking
> > > > > > user
> > > > > > space'?
> > > > > > 
> > > > > > A message produced by ima_parse_rule() looks like this here:
> > > > > > 
> > > > > > type=INTEGRITY_RULE msg=audit(1526566213.870:305):
> > > > > > action="dont_measure"
> > > > > > fsmagic="0x9fa0" res=1
> > > > > 
> > > > > Why is action and fsmagic being logged as untrusted strings?
> > > > > Untrusted
> > > > > strings are used when an unprivileged user can affect the contents
> > > > > of the
> > > > > field such as creating a file with space or special characters in
> > > > > the
> > > > > name.
> > > > > 
> > > > > Also, subject and object information is missing. Who loaded this
> > > > > rule?
> > > > > 
> > > > > > in contrast to that an INTEGRITY_PCR record type:
> > > > > > 
> > > > > > type=INTEGRITY_PCR msg=audit(1526566235.193:334): pid=1615 uid=0
> > > > > > auid=0
> > > > > > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > > > > op="invalid_pcr" cause="open_writers" comm="scp"
> > > > > > name="/var/log/audit/audit.log" dev="dm-0" ino=1962625 res=1
> > > > > 
> > > > > Why is op & cause being logged as an untrusted string? This also
> > > > > has
> > > > > incomplete subject information.
> > > > 
> > > > It's calling audit_log_string() in both cases:
> > > > 
> > > > https://elixir.bootlin.com/linux/latest/source/security/integrity/int
> > > > egrity _audit.c#L48
> > > 
> > > I see. Looking things over, I see that it seems like it should do the
> > > right thing. But the original purpose for this function is here:
> > > 
> > > https://elixir.bootlin.com/linux/latest/source/kernel/audit.c#L1944
> > > 
> > > This is where it is logging an untrusted string and has to decide to
> > > encode it or just place it in quotes. If it has quotes, that means
> > > it's an untrusted string but has no control characters in it. I think
> > > you want to use audit_log_format() for any string that is trustworthy.
> > 
> > Replacing all occurrences (in IMA) of audit_log_string() with
> > audit_log_format().
> > 
> > > As an aside, I wonder why audit_log_string() is in the API when it is a
> > > helper to audit_log_untrustedstring() ?  Without understanding the
> > > rules of untrusted strings, it can't be used correctly without
> > > re-inventing audit_log_untrustedstring() by hand.
> > > 
> > > > > > Should some of the fields from INTEGRITY_PCR also appear in
> > > > > > INTEGRITY_RULE? If so, which ones?
> > > > > 
> > > > > pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are
> > > > > required to be searchable
> > > > > 
> > > > > > We could probably refactor the current  integrity_audit_message()
> > > > > > and
> > > > > > have ima_parse_rule() call into it to get those fields as well. I
> > > > > > suppose adding new fields to it wouldn't be considered breaking
> > > > > > user
> > > > > > space?
> > > > > 
> > > > > The audit user space utilities pretty much expects those fields in
> > > > > that
> > > > > order for any IMA originating events. You can add things like op or
> > > > > cause before
> > > > 
> > > > We will call into audit_log_task, which will put the parameters into
> > > > correct order:
> > > > 
> > > > auid uid gid ses subj pid comm exe
> > > 
> > > I'm telling you what the correct order is.  :-)  A long time ago, the
> > > IMA
> > :
> > :-) Thanks. Was getting confused.
> > :
> > > system had audit events with the order I'm mentioning. For example,
> > > here's
> > > one from a log I collected back in 2013:
> > > 
> > > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0
> > > auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure"
> > > cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs
> > > ino=5413 res=0
> > > 
> > > it was missing "tty" and "exe", but the order is as I mentioned. The
> > > expectation is that INTEGRITY events maintain this established order
> > > across all events.
> > 
> > I am *appending* exe= and tty= now:
> > 
> > type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0
> > ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > op="invalid_pcr" cause="open_writers" comm="ssh"
> > name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1
> > exe="/usr/bin/ssh" tty=tty2
> 
> This isn't necessary since they already covered in the already
> connected SYSCALL record which duplicates even more information than is
> already.

My logs don't show any syscall record being attached. Nor should it. This is 
a simple event that should stand on its own.

-Steve

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

[Steve Grubb]

On Monday, May 21, 2018 5:57:29 PM EDT Stefan Berger wrote:
> >>>> Should some of the fields from INTEGRITY_PCR also appear in
> >>>> INTEGRITY_RULE? If so, which ones?
> >>> 
> >>> pid, uid, auid, tty, session, subj, comm, exe, res.  <- these are
> >>> required to be searchable
> >>> 
> >>>> We could probably refactor the current  integrity_audit_message() and
> >>>> have ima_parse_rule() call into it to get those fields as well. I
> >>>> suppose adding new fields to it wouldn't be considered breaking user
> >>>> space?
> >>> 
> >>> The audit user space utilities pretty much expects those fields in that
> >>> order for any IMA originating events. You can add things like op or
> >>> cause before
> >> 
> >> We will call into audit_log_task, which will put the parameters into
> >> correct order:
> >> 
> >> auid uid gid ses subj pid comm exe
> > 
> > I'm telling you what the correct order is.  :-)  A long time ago, the IMA
> 
>  Thanks. Was getting confused.
> 
> > system had audit events with the order I'm mentioning. For example,
> > here's one from a log I collected back in 2013:
> > 
> > type=INTEGRITY_PCR msg=audit(1327409021.813:21): pid=1 uid=0
> > auid=4294967295 ses=4294967295 subj=kernel op="add_template_measure"
> > cause="hash_added" comm="init" name="01parse-kernel.sh" dev=rootfs
> > ino=5413 res=0
> > 
> > it was missing "tty" and "exe", but the order is as I mentioned. The
> > expectation is that INTEGRITY events maintain this established order
> > across all events.
> 
> I am *appending* exe= and tty= now:
> 
> type=INTEGRITY_PCR msg=audit(1526939047.809:305): pid=1609 uid=0 auid=0
> ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> op="invalid_pcr" cause="open_writers" comm="ssh"
> name="/var/lib/sss/mc/passwd" dev="dm-0" ino=1962679 res=1
> exe="/usr/bin/ssh" tty=tty2

It is safe to put them where they belong. The tools currently skip over tty 
and exe if they would be present. So, nothing would get messed up. Its very 
simple to add an "if" statement to check for these new fields and collect 
them for searching. Also, "res" is traditionally the last field in any event.

Thanks,
-Steve