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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
