Re: [PATCH 2/2] audit: restore AUDIT_LOGINUID unset ABI

[Paul Moore <pmoore@redhat.com>]

On Tuesday, December 16, 2014 02:20:04 PM Richard Guy Briggs wrote:
> On 14/12/12, Paul Moore wrote:
> > On Friday, December 12, 2014 11:44:50 AM Richard Guy Briggs wrote:
> > > On 14/12/12, Paul Moore wrote:
> > > > On Friday, December 12, 2014 12:20:16 AM Richard Guy Briggs wrote:
> > ...
> > 
> > > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > > > index fb4d2df..ea62c7b 100644
> > > > > --- a/kernel/auditfilter.c
> > > > > +++ b/kernel/auditfilter.c
> > > > > @@ -441,6 +441,7 @@ static struct audit_entry
> > > > > *audit_data_to_entry(struct
> > > > > audit_rule_data *data, if ((f->type == AUDIT_LOGINUID) && (f->val ==
> > > > > AUDIT_UID_UNSET)) { f->type = AUDIT_LOGINUID_SET;
> > > > > 
> > > > >  			f->val = 0;
> > > > > 
> > > > > +			entry->rule.flags |= AUDIT_LOGINUID_LEGACY;
> > > > > 
> > > > >  		}
> > > > >  		
> > > > >  		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
> > > > > 
> > > > > @@ -592,7 +593,7 @@ static struct audit_rule_data
> > > > > *audit_krule_to_data(struct audit_krule *krule) return NULL;
> > > > > 
> > > > >  	memset(data, 0, sizeof(*data));
> > > > > 
> > > > > -	data->flags = krule->flags | krule->listnr;
> > > > > +	data->flags = (krule->flags & ~AUDIT_LOGINUID_LEGACY) |
> > > > > 
> > > > >                  krule->listnr;
> > > > 
> > > > Argh!  I missed that the audit_krule->flags end up in
> > > > audit_rule_data->flags.
> > > 
> > > Well, it came in that way...
> > 
> > Yes, it does, my mistake.  I was probably just looking at the structure
> > definition, saw it wasn't exported to userspace, and thought the "flags"
> > field seemed promising.
> > 
> > > > Bummer.
> > > > 



> > > > Some thoughts:
> > > > 
> > > > * Your 1/2 patch saved 32-bits in audit_krule, what are your thoughts
> > > > on adding a new 32-bit bitmap, say "private", which could be used
> > > > internally to track things like this?  I'm not a big fan of
> > > > overloading parts of the public API for use by internal mechanisms, it
> > > > almost always gets messy.
> > > 
> > > I thought it was going to be messier, but I like how it turned out
> > > cleaner because of the way it was already used.
> > 
> > Yes, I think using audit_krule->flags is an improvement over the previous
> > patch, but I think we are better served using a field that doesn't
> > interfere with the userspace API.
> 
> But it doesn't interfere.  A field is passed in from userspace that is
> multiplexed and has to be filtered anyways into its components in the
> internal representation.  It is then combined again on output to
> userspace from more than one source.  Arguably, the field passed in from
> userspace it mis-named, since it isn't strictly flags, but a list number
> from zero to five with a flag added just larger than any of the list
> numbers.
> 
> The userspace API is not affected.

Maybe I'm reading the code wrong, but it is taking bits away from a bitfield 
which is part of the audit API is it not?  This is why I don't like this, or 
the previous approaches, and why I would prefer to track the "legacy" state in 
another private field.

> Would you prefer if I filter the internal flags field with "&
> AUDIT_FILTER_PREPEND" rather than "& ~AUDIT_LOGINUID_LEGACY" to make it
> more clear what is being recombined in audit_krule_to_data()?

If the solution involves filtering out anything before returning the data to 
userspace I'm not in favor of the solution (see above).

> > > > * Also, why is there both an audit_krule->flags and
> > > > audit_krule->listnr field? With the exception of the
> > > > AUDIT_FILTER_PREPEND bit are they always going to be the same?  I
> > > > wonder if some more cleanup could be done here ...
> > > 
> > > This is part of the API.  The flags field is used to hand in the list
> > > number and its intended position on the list.  Once it gets transferred
> > > from a user data blob to a kernel entry, it is split into listnr and
> > > flags.
> > 
> > The question I was trying to ask, perhaps rhetorically at this point, is
> > if there is much/any advantage to spliting the public API flags into the
> > private flags/listnr field.  It's probably not worth worrying about in
> > the context of this fix, just something that popped into my head when
> > looking at this fix. In retrospect I probably shouldn't have muddled the
> > discussion with this idea.
>
> Ok, I hadn't understood that you were contemplating leaving that field
> passed from userspace as it was passed in, in the internal
> representation and simply filtering it for necessary information at the
> point of use.  That has some merit, but that listnr value is used at
> least a dozen and a half places and would need filtering in each of
> those, slightly complicating/obfuscating the code.

Personally I see it more as a performance issue, rather than an obfuscation 
issue.  However, as I said earlier, let's not worry about this now; I'd much 
rather us stay focused on resolving the LOGINUID issue.  It was a mistake on 
my part to bring this up in this thread.
 
> > > I thought it made sense to internally add it to the flags field.
> > 
> > I would still like us to use an internal field for tracking things
> > that aren't part of the API.
> 
> It *is* and internal field.

Yes, and no.  Yes it is internal, but it shares its value with the API.  This 
is my problem with these patches (see above).

> Internally, the "flags" field is only used for *one* bit, which seems
> like a waste to add another 32-bit field to accomodate another
> single-bit flag.

Considering we just reclaimed a 32-bit field I consider this a zero-sum 
change.  Perhaps we'll do things differently in the future, this is a private 
state flag after all, but right now I'm more comfortable creating a new 
private flag field.

> Is it worth turning it into a bitfield to avoid the confusion?

Yes.  At least that's my opinion at this point.

> The overhead of doing the filtering on rule creation and deletion seems
> pretty minimal compared to adding a 32-bit field that stays with every
> entry.

The cost of stealing a bit from the public API can be huge.

-- 
paul moore
security and virtualization @ redhat