Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1587500467.git.rgb@redhat.com> <b8ba40255978a73ea15e3859d5c945ecd5fede8e.1587500467.git.rgb@redhat.com>
 <CAHC9VhR9sNB58A8uQ4FNgAXOgVJ3RaWF4y5MAo=3mcTojaym0Q@mail.gmail.com>
 <20200517141515.qqx3jx5ulb2546tx@madcap2.tricolour.ca> <CAHC9VhQVRyJ7GRHrujW5Ri-pvBRBgFM2Y8+OYQxca1bUyv2eyg@mail.gmail.com>
 <20200518003920.e6vyzhvadyi5wdjd@madcap2.tricolour.ca>
In-Reply-To: <20200518003920.e6vyzhvadyi5wdjd@madcap2.tricolour.ca>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 18 May 2020 10:40:51 -0400
Message-ID: <CAHC9VhTRTm3OywsmZfcjQjfU3OytCbFZ_Qa1uO4LCOEhC=3tnQ@mail.gmail.com>
Subject: Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG
 record to cover async unregister
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     fw@strlen.de, LKML <linux-kernel@vger.kernel.org>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        netfilter-devel@vger.kernel.org, ebiederm@xmission.com,
        twoerner@redhat.com, Eric Paris <eparis@parisplace.org>,
        tgraf@infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, May 17, 2020 at 8:40 PM Richard Guy Briggs <rgb@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@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@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