Message-Id: <7fdca0e0-19d5-1f08-8aa2-f295ad3a86de@linux.vnet.ibm.com>
Date:   Fri, 18 May 2018 12:49:53 -0400
From:   Stefan Berger <stefanb@...ux.vnet.ibm.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        containers@...ts.linux-foundation.org,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        linux-integrity <linux-integrity@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, paul@...l-moore.com,
        sgrubb@...hat.com
Subject: Re: [PATCH] audit: add containerid support for IMA-audit

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@...ux.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@...hat.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@...hat.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@...hat.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
>

Powered by blists - more mailing lists