Message-ID: <16251997.Lr3lOVDqLJ@x2>
Date:   Mon, 21 May 2018 14:40:40 -0400
From:   Steve Grubb <sgrubb@...hat.com>
To:     Stefan Berger <stefanb@...ux.vnet.ibm.com>
Cc:     Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Richard Guy Briggs <rgb@...hat.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
Subject: Re: [PATCH] audit: add containerid support for IMA-audit

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

Powered by blists - more mailing lists