[RFC][PATCH] audit: add feature audit_lost reset

[Richard Guy Briggs]

On 2016-12-07 18:45, Paul Moore wrote:
> On Wed, Dec 7, 2016 at 6:30 PM, Steve Grubb <sgrubb at redhat.com> wrote:
> > On Wednesday, December 7, 2016 6:10:49 PM EST Paul Moore wrote:
> >> On Wed, Dec 7, 2016 at 10:58 AM, Richard Guy Briggs <rgb at redhat.com> wrote:
> >> > On 2016-12-07 10:53, Steve Grubb wrote:
> >> >> On Wednesday, December 7, 2016 10:05:30 AM EST Paul Moore wrote:
> >> >> > On Tue, Dec 6, 2016 at 10:32 PM, Richard Guy Briggs <rgb at redhat.com>
> > wrote:
> >> >> > > On 2016-12-06 19:17, Paul Moore wrote:
> >> >> > >> On Tue, Dec 6, 2016 at 12:13 AM, Richard Guy Briggs <rgb at redhat.com>
> >> >> > >> Okay, back up ... this whole mess about atomic_xchg() was always
> >> >> > >> unrelated to my original suggestion, let's focus on my original
> >> >> > >> comment ... don't reset the counter on a AUDIT_GET, reset it on a
> >> >> > >> AUDIT_SET with an AUDIT_STATUS_LOST, does that make sense?
> >> >> > >
> >> >> > > I understood that.  It sounds like a nice simple and straightforward
> >> >> > > method to do it but for the question of accuracy.  Please rewind to
> >> >> > > my
> >> >> > > fundamental point: How do we get an accurate reading of the last
> >> >> > > value
> >> >> > > of audit_lost before resetting it?
> >> >> >
> >> >> > Okay, I thought you were worried about a different race, which is why
> >> >> > this discussion wasn't making much sense to me.  I understand your
> >> >> > point, but I really dislike the API; although that's not your fault,
> >> >> > it's really the only way to do it via AUDIT_GET.
> >> >> >
> >> >> > I'd much prefer we go with the cleaner AUDIT_SET approach and just not
> >> >> > worry about the small race window.  It would only be an issue if you
> >> >> > reset the count under heavy audit load, and why would you reset the
> >> >> > lost value if you were under a heavy audit load?  That just doesn't
> >> >> > make sense.
> >> >> >
> >> >> > I suppose we should hear from Steve on this since he was the one who
> >> >> > has been asking for this feature, although I'm pretty sure I know what
> >> >> > he is going to say.
> >> >>
> >> >> To start with, this request comes from users of the audit system. I just
> >> >> passed along the request. The issue is that when you do auditctl -s, you
> >> >> get the number of records lost. If you do it the next day, you have to
> >> >> do math to see what the one day delta is. So, to make reporting easy,
> >> >> they want it to be reset whenever they do audictl -s.
> >> >>
> >> >> You could also make a AUDIT_GET_RESET that gets the status and resets the
> >> >> number atomically. Then I can add another commandline option to auditctl
> >> >> that allows an admin to say also reset the counters. If that command
> >> >> line option is passed, I call AUDIT_GET_RESET otherwise I call
> >> >> AUDIT_GET. Thought?>
> >> > This would be slightly simpler in kernel implementation than the method
> >> > I proposed and would work fine, off the top of my head.
> >>
> >> I'd prefer not to introduce another command message type for something
> >> small like this.
> >>
> >> Steve, do you have any objection to the AUDIT_SET based approach?
> >
> > Either way, we'd need a feature flag so that I can tell if the kernel supports
> > this or not.
> 
> I think we are okay without a specific feature flag as sending a
> AUDIT_SET/reset on an old kernel will be harmless; it won't do
> anything, but it shouldn't return an error either.

Ok, so userspace is still left wondering if it worked until the next
time it reads that value, and even then it can't be certain if that
value is the same or higher than it was when userspace thought it reset
that value.  At this point, an old kernel will not return an error and
simply ignore any new AUDIT_STATUS_* flag since each flag is treated
independently and extra flags are ignored and not blocked.  This seems
sloppy since we have two ways of fixing this uncertainty pretty easily.

> > Also, it should accept "0" as the only valid value.
> 
> Of course.

No problem.

> > We can do this and I can make auditctl do the two back to back before displaying the
> > results to minimize the window of risk.

Having it depend on userspace implementation to minimize the risk of a 
race strikes me as unsound design.

> >> Based on what you've said above, it would seem like the potential race
> >> condition with AUDIT_SET wouldn't be a significant issue.
> >
> > All a matter of perspective. What I think is a reasonable risk someone else
> > may disagree. Does anyone else on the list object? If not I'd say go with it.
> 
> Judgement calls are always a matter of perspective, since you are the
> only one who has asked for this (even if it is by proxy) I was asking
> for your perspective.  Anyway, it looks like we're probably okay here;
> if we don't hear anything in the next day or two lets go ahead with
> the AUDIT_SET approach.  If it proves to be a problem we can always
> introduce one of the other approaches later.

If it isn't dependable and accurate, what's the point?  Why not do it
right the first time?

> paul moore

- RGB

--
Richard Guy Briggs <rgb at redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635