Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG record to cover async unregister
On Sun, May 17, 2020 at 8:40 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2020-05-17 17:50, Paul Moore wrote:
> On Sun, May 17, 2020 at 10:15 AM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2020-04-28 18:25, Paul Moore wrote:
> > > On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <rgb(a)redhat.com>
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?
> > >
> > > 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.
> >
> > Can you explain why auid falls under this criteria but ses does not if
> > both are unset?
>
> "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."
>
> It's a concession to Steve. As I mentioned previously, I think the
> subject info is bogus in this case; either it is valid and we get it
> from the SYSCALL record or it simply isn't present in any meaningful
> way.
Sorry for being so dense. I still don't follow your explanation. You've
repeated the same paragraph that didn't make sense to me the first time.
What definition of "subject info" are you working with?
The subject is generally the task which is causing the event to occur,
"subject info" would be any such attribute which describes the
subject; examples include LSM label, the various UIDs/GIDs, etc..
Think "current->cred" and you on the right track.
I had assumed
it was the set of fields that contain information that came from that
task's struct task_struct. Some of those fields contain information
that isn't helpful. Why not remove them all rather than keep one that
still contains no useful information?
Once again - and I don't know how else to explain this to you - I
think it is pointless to record the subject info in this record as we
either have that info from other records in the event or there is no
valid subject info to record. As you state in the commit description:
"Some table unregister actions seem to be initiated by the
kernel to garbage collect unused tables that are not
initiated by any userspace actions."
Or is it a matter of keeping one
key field that contains no useful information that proves that the rest
is bogus? Steve said that daemons leave no useful information in auid
as well, so I don't see how keeping this field helps us. My
understanding is that the subj field's "...:kernel_t:..." is the key
here and that pid and comm give us a bit more of a clue that it is a
kernel thread. Is that correct? What use does including auid serve
here?
As I've mentioned in the thread above, including the auid was done as
a concession to Steve, I don't think it serves any useful purpose.
--
paul moore
www.paul-moore.com