Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast
On Tue, Oct 17, 2017 at 12:01 PM, Steve Grubb <sgrubb(a)redhat.com> wrote:
On Tuesday, October 17, 2017 11:11:31 AM EDT Paul Moore wrote:
> On Mon, Oct 16, 2017 at 6:28 PM, Steve Grubb <sgrubb(a)redhat.com> wrote:
> > On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
> >> > > +/* Log information about who is connecting to the audit
multicast
> >> > > socket
> >> > > */ +static void audit_log_multicast_bind(int group, const char
*op,
> >> > > int
> >> > > err) +{
> >> > > + const struct cred *cred;
> >> > > + struct tty_struct *tty;
> >> > > + char comm[sizeof(current->comm)];
> >> > > + struct audit_buffer *ab;
> >> > > +
> >> > > + ab = audit_log_start(NULL, GFP_KERNEL,
AUDIT_EVENT_LISTENER);
> >> >
> >> > It really seems like this should be associated with the current task,
> >> > e.g. "audit_log_start(current->audit_context, ...)".
After all, the
> >> > whole point of this record is to capture information about the subject
> >> > who is binding to the multicast socket.
> >>
> >> OK, easy enough.
> >
> > But wouldn't that make it an auxiliary record (if there happens to be a
> > syscall record) instead of a standalone event?
>
> I've always found the significance placed on auxiliary vs standalone
> events amusing. The only significant difference between the two is
> that "auxiliary" records share a timestamp with the other records that
> have been triggered by a given syscall instance whereas standlone
> records always have a new timestamp generated. I'm personally of the
> opinion that if we are emitting records we should always try to group
> related records into a single event to help reduce the confusion for
> people looking through their audit logs; in other words, whenever
> possible we should pass the audit_context to audit_log_start() and
> friends.
No, actually that introduces confusion. An event should have one specific
meaning. This subject did something to that. Adding a syscall record takes a
simple event and makes it incomprehensible to anyone that doesn't have
familiarity with audit events. All necessary information is in the event that
I created.
What? That makes zero sense. The SYSCALL record and the
AUDIT_EVENT_LISTENER record are part of the same real world event: a
process executed a syscall and established themselves as an audit
event listener.
> If syscall auditing is disabled, these records (e.g.
> AUDIT_EVENT_LISTENER) will still be emitted by the kernel as
> "standalone" events. If syscall auditing is enabled, these records
> will be grouped with the triggering syscall, which just makes sense.
> Look at the recent problem with init/systemd and the MAC_STATUS
> records, that is a great example.
Right. That was a mess because of piggy backing on top of a SYSCALL.
Again ... what? The SYSCALL record was the solution to that
particular problem, and demonstrated that an event could have multiple
records depending on the audit configuration (in this particular case
syscall auditing disabled/enabled).
I do not
like events that are sometime simple and sometimes compound.
That's dependent on your system's audit configuration.
I think that also
messes up anyone doing json or xml formatting because they need a firm layout
in their schema.
I would think anyone consuming audit data for analysis at a higher
level would *really* want to see all related records grouped together
(i.e. same timestamp). Any higher order analysis should be able to
handle event metadata that is optional/missing (we have that now).
The event is good as is. It has all required fields ...
... until you decide that you need to add something; we've seen how
well that is going.
--
paul moore
www.paul-moore.com