Re: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records

[Paul Moore]

On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb <sgrubb@xxxxxxxxxx> wrote:
> On Thu, 17 Jan 2019 08:21:40 -0500
> Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb@xxxxxxxxxx> wrote:
> > > On Mon, 14 Jan 2019 17:58:58 -0500
> > > Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > >
> > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> > > > <rgb@xxxxxxxxxx> wrote:
> > > > >
> > > > > Tie syscall information to all CONFIG_CHANGE calls since they
> > > > > are all a result of user actions.
> > >
> > > Please don't tie syscall information to this. The syscall will be
> > > sendto. We don't need that information, its implicit. Also, doing
> > > this will possibly wreck things in libauparse. Please test the
> > > events with ausearch --format csv and --format text. IFF the event
> > > looks better or the same should we do this. If stuff disappears,
> > > the patch is breaking things
> >
> > We've discussed this quite a bit already;
>
> Yes, and you still don't seem be listening. You have to cooperate on
> modifying events. We as a community need to respect each other's needs
> and work together to solve problems. What this is saying sounds a lot
> like I don't care if it breaks things, I'm gonna do it my way. Tough
> luck.

I see you added LKML to the To/CC line.  Fun.  Unless they've been
following the audit list for the past several years I'm not sure they
have all the context to fully understand some of the things in this
thread, but sure, why not.

I understand you are frustrated Steve, I get it.  I can also
understand how you feel that I'm not listening to you because I'm not
agreeing with you on this, I get this too.  However, we've talked
about this quite a bit and keeping individual audit records split
across multiple events when they are all part of the same event just
doesn't make sense to me.  The users who have commented on this also
agree that associated records should be combined into one event.  I
definitely don't want to put words in Richard's mouth, but I believe
he has also said that he believes that associating all related records
into a single event makes sense.

I'm listening to you Steve - look at all the times I've asked for your
perspective when it comes to certification requirements? - I just
don't always agree with you.

> You do not have to make sense of any of these events. So, what does it
> really matter to you how the event is formed? What I'm asking for is
> have these changes been vetted to ensure that they are not breaking
> things?

We are not changing the record formats, all we are doing is changing
the timestamp/event counter so that related records are grouped
together into the same event.  For example, in this particular case
(describing this for the LKML crowd that may be reading this) we are
combining the syscall audit record that triggers the audit
configuration change with the audit configuration change record into a
single event.  The code currently treats the syscall record and the
audit config change record as separate events - which is just silly as
they belong to the same event, triggered by the same user action -
this patch should fix this by associating the two records so they are
part of the same audit event.  From my perspective this is a bug fix.

> > connecting associated records into a single event is something that
> > should happen, needs to happen, and will happen.  Conceptually it
> > makes no sense to record the syscall (and any other associated
> > records) which triggers the audit configuration change, and the
> > configuration change record itself as two distinct events - they are
> > the same event.
>
> Except that they are not. The design of the audit system is to save
> disk space as much as possible by emitting single record events on
> certain event types that are simple. To add a syscall to it adds useless
> information (such as a socket address record), slows down processing,
> and wastes disk space. If you get a SYSCALL record, that indicates that
> you have triggered an event that the system admin has placed explicit
> rules on. That is different than the common criteria required
> configuration change event.

I've said this many times in the past, I believe Richard has said the
same too (maybe it was in a GH issue?), but I'll say it again ... if
the system's audit config is such that syscall records are not being
generated, then this patch will not cause them to be generated; all
this patch does is ensure that *if* a syscall record is being
generated *and* an audit config change record is being generated, then
the two records will share the same event counter and treated as a
single event.  This change does not cause any change to the amount of
information emitted by the kernel, all it does is group the related
records together.

> >  We've also heard from a prominent user that
> > associating records in this way is desirable.
> >
> > If the ausearch csv and text audit log transformations can't handle
> > this particular change, I would consider that a shortcoming of that
> > code.  We have multi-record events now, and this is only going to
> > increase in the future.
>
> Isn't there some kind a guideline about not breaking user space?

Yes, when that bit of userspace code predates the change.  You knew
this was coming when you wrote that tool, writing userspace
applications that make poor assumptions and using that as a way to
block kernel development is not something I look upon favorably.

-- 
paul moore
www.paul-moore.com