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;
Date:   Wed, 6 May 2020 18:42:33 -0400
From:   Richard Guy Briggs <rgb@redhat.com>
To:     Steve Grubb <sgrubb@redhat.com>
Cc:     Paul Moore <paul@paul-moore.com>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        netfilter-devel@vger.kernel.org, omosnace@redhat.com, fw@strlen.de,
        twoerner@redhat.com, Eric Paris <eparis@parisplace.org>,
        ebiederm@xmission.com, tgraf@infradead.org
Subject: Re: [PATCH ghak25 v4 3/3] audit: add subj creds to NETFILTER_CFG
 record to cover async unregister
Message-ID: <20200506224233.najv6ltb5gzcicqb@madcap2.tricolour.ca>
References: <cover.1587500467.git.rgb@redhat.com>
 <3348737.k9gCtgYObn@x2>
 <20200429213247.6ewxqf66i2apgyuz@madcap2.tricolour.ca>
 <3250272.v6NOfJhyum@x2>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <3250272.v6NOfJhyum@x2>
User-Agent: NeoMutt/20180716
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2020-05-06 17:26, Steve Grubb wrote:
> On Wednesday, April 29, 2020 5:32:47 PM EDT Richard Guy Briggs wrote:
> > On 2020-04-29 14:47, Steve Grubb wrote:
> > > On Wednesday, April 29, 2020 10:31:46 AM EDT Richard Guy Briggs wro=
te:
> > > > On 2020-04-28 18:25, Paul Moore wrote:
> > > > > On Wed, Apr 22, 2020 at 5:40 PM Richard Guy Briggs <rgb@redhat.=
com>
> > >=20
> > > wrote:
> > > > > > Some table unregister actions seem to be initiated by the ker=
nel 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=3DNETFILTER_CFG msg=3Daudit(2020-03-11 21:25:21.491:269)=
 :
> > > > > > table=3Dnat
> > > > > > family=3Dbridge entries=3D0 op=3Dunregister pid=3D153 uid=3Dr=
oot auid=3Dunset
> > > > > > tty=3D(none) ses=3Dunset subj=3Dsystem_u:system_r:kernel_t:s0
> > > > > > comm=3Dkworker/u4:2 exe=3D(null)>
> > > > >=20
> > > > > [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.]
> > > > >=20
> > > > > Steve wants subject info in this case, okay, but let's try to t=
rim
> > > > > 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 t=
hink
> > > > > that means we can drop "tty", "ses", "comm", and "exe" ... yes?
> > > >=20
> > > > From the ghak28 discussion, this list and order was selected due =
to
> > > > Steve's preference for the "kernel" record convention, so deviati=
ng
> > > > 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.
> > > >=20
> > > > I do agree that some fields will be superfluous in the kernel cas=
e.
> > > > The most important field would be "subj", but then "pid" and "com=
m", I
> > > > would think.  Based on this contents of the "subj" field, I'd thi=
nk
> > > > that "uid", "auid", "tty", "ses" and "exe" are not needed.
> > >=20
> > > We can't be adding deleting fields based on how its triggered. If t=
hey
> > > are unset, that is fine. The main issue is they have to behave the =
same.
> >=20
> > I don't think the intent was to have fields swing in and out dependin=
g
> > on trigger.  The idea is to potentially permanently not include them =
in
> > this record type only.  The justification is that where they aren't
> > needed for the kernel trigger situation it made sense to delete them
> > because if it is a user context event it will be accompanied by a
> > syscall record that already has that information and there would be n=
o
> > sense in duplicating it.
>=20
> We should not be adding syscall records to anything that does not resul=
t from=20
> a syscall rule triggering the event. Its very wasteful. More wasteful t=
han=20
> just adding the necessary fields.

So what you are saying is you want all the fields that are being
proposed to be added to this record?

If the records are all from one event, they all should all have the same
timestamp/serial number so that the records are kept together and not
mistaken for multiple events.  One reason for having information in
seperate records is to be able to filter them either in kernel or in
userspace if you don't need certain records.

> I also wished we had a coding specification that put this in writing so=
 that=20
> every event is not a committee decision. That anyone can look at the do=
cument=20
> and Do The Right Thing =E2=84=A2.
>=20
> If I add a section to Writing-Good-Events outlining the expected orderi=
ng of=20
> fields, would that be enough that we do not have long discussions about=
 event=20
> format? I'm thinking this would also help new people that want to contr=
ibute.

If you add this expected ordering of fields, can we re-factor all the
kernel code to use this order because the userspace parser won't care
what order they are in?

I realize this isn't what you are saying, but having a clear description
in that document that talks about the different classes of events and
what each one needs in terms of minimum to full subject attributes and
object attributes would help a lot.  It would also help for new records
to be able to decide if it should follow the format of an existing
related or similar record, or a new class with the expected standard orde=
r.

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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