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@xxxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >   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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
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