Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister
From: Richard Guy Briggs
Date: Wed Apr 29 2020 - 10:32:37 EST
On 2020-04-28 18:25, Paul Moore wrote:
> On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > Some table unregister actions seem to be initiated by the kernel to
> > garbage collect unused tables that are not initiated by any userspace
> > actions. It was found to be necessary to add the subject credentials to
> > cover this case to reveal the source of these actions. A sample record:
> >
> > type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null)
>
> [I'm going to comment up here instead of in the code because it is a
> bit easier for everyone to see what the actual impact might be on the
> records.]
>
> Steve wants subject info in this case, okay, but let's try to trim out
> some of the fields which simply don't make sense in this record; I'm
> thinking of fields that are unset/empty in the kernel case and are
> duplicates of other records in the userspace/syscall case. I think
> that means we can drop "tty", "ses", "comm", and "exe" ... yes?
>From the ghak28 discussion, this list and order was selected due to
Steve's preference for the "kernel" record convention, so deviating from
this will create yet a new field list. I'll defer to Steve on this. It
also has to do with the searchability of fields if they are missing.
I do agree that some fields will be superfluous in the kernel case.
The most important field would be "subj", but then "pid" and "comm", I
would think. Based on this contents of the "subj" field, I'd think that
"uid", "auid", "tty", "ses" and "exe" are not needed.
> While "auid" is a potential target for removal based on the
> dup-or-unset criteria, I think it falls under Steve's request for
> subject info here, even if it is garbage in this case.
If we keep auid, I'd say keep ses, since they usually go together,
though they are separated by another field in this "kernel" record field
ordering.
I expect this orphan record to occur so infrequently that I don't think
bandwidth or space are a serious concern.
> > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > ---
> > kernel/auditsc.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d281c18d1771..d7a45b181be0 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2557,12 +2557,30 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> > enum audit_nfcfgop op)
> > {
> > struct audit_buffer *ab;
> > + const struct cred *cred;
> > + struct tty_struct *tty;
> > + char comm[sizeof(current->comm)];
> >
> > ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_NETFILTER_CFG);
> > if (!ab)
> > return;
> > audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
> > name, af, nentries, audit_nfcfgs[op].s);
> > +
> > + 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));
> > + audit_put_tty(tty);
> > + audit_log_task_context(ab); /* subj= */
> > + audit_log_format(ab, " comm=");
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > + audit_log_d_path_exe(ab, current->mm); /* exe= */
> > +
> > audit_log_end(ab);
> > }
> > EXPORT_SYMBOL_GPL(__audit_log_nfcfg);
>
> --
> paul moore
> www.paul-moore.com
>
- RGB
--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635