Re: [PATCH ghak28 V4] audit: log audit netlink multicast bind and unbind events
On Wed, Jan 22, 2020 at 6:07 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2020-01-22 17:40, Paul Moore wrote:
> On Fri, Jan 17, 2020 at 3:21 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
...
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 17b0d523afb3..478259f3fa53 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1520,20 +1520,60 @@ static void audit_receive(struct sk_buff *skb)
> > audit_ctl_unlock();
> > }
> >
> > +/* 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;
> > +
> > + if (!audit_enabled)
> > + return;
> > +
> > + ab = audit_log_start(audit_context(), GFP_KERNEL,
AUDIT_EVENT_LISTENER);
> > + if (!ab)
> > + return;
> > +
> > + cred = current_cred();
> > + tty = audit_get_tty();
> > + audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > + task_pid_nr(current),
> > + from_kuid(&init_user_ns, cred->uid),
> > + from_kuid(&init_user_ns,
audit_get_loginuid(current)),
> > + tty ? tty_name(tty) : "(none)",
> > + audit_get_sessionid(current));
>
> Don't we already get all of that information as part of the syscall record?
Yes. However, the syscall record isn't always present. One example is
systemd, shown above.
Assuming that the system supports syscall auditing, the absence of a
syscall record is a configuration choice made by the admin. If the
system doesn't support syscall auditing the obvious "fix" is to do the
work to enable syscall auditing on that platform ... but now we're
starting to get off topic.
The other is the disconnect record, shown above,
which may be asynchronous, or an unmonitored syscall (It could only be
setsockopt, close, shutdown.).
An unmonitored syscall still falls under the category of a
configuration choice so I'm not too concerned about that, but the
async disconnect record is legitimate. Can you provide more
information about when this occurs? I'm guessing this is pretty much
just an abrupt/abnormal program exit?
> I'm pretty sure these are the same arguments I made when
Steve posted
> a prior version of this patch.
You did. I would really like to have dropped them, but they aren't
reliably available.
Personally I'm not too worried if we have duplicate information spread
across records in a single event, as long as they are consistent.
However, I remember Steve complaining rather loudly about duplicated
fields across records in a single event some time back; perhaps that
is not a concern of his anymore (perhaps it was a narrow case at the
time), I don't know.
Here is the deal, either duplicated information is something we are
okay with, or it is something to avoid; we need to pick one. As
mentioned above, I don't really care that much either way (I have a
slight preference, but I don't feel strongly enough to fight for it),
so let's hear the arguments both for and against and decide - whatever
we pick I'll enforce so long as we are stuck with this string format.
--
paul moore
www.paul-moore.com