Date:   Fri, 17 Apr 2020 18:19:07 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Paris <eparis@...isplace.org>
Subject: Re: [PATCH ghak28 V7] audit: log audit netlink multicast bind and
 unbind events

On Fri, Apr 17, 2020 at 5:34 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> On 2020-04-17 17:18, Paul Moore wrote:
> > On Tue, Mar 17, 2020 at 12:04 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> > >
> > > Log information about programs connecting to and disconnecting from the
> > > audit netlink multicast socket. This is needed so that during
> > > investigations a security officer can tell who or what had access to the
> > > audit trail.  This helps to meet the FAU_SAR.2 requirement for Common
> > > Criteria.  Here is the systemd startup event:
> > >
> > > type=PROCTITLE msg=audit(2020-02-18 15:26:50.775:10) : proctitle=/init
> > > type=SYSCALL msg=audit(2020-02-18 15:26:50.775:10) : arch=x86_64 syscall=bind success=yes exit=0 a0=0x19 a1=0x55645c369b70 a2=0xc a3=0x7fff9fedec24 items=0 ppid=0 pid=1 auid=unset uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=unset comm=systemd exe=/usr/lib/systemd/systemd subj=kernel key=(null)
> > > type=UNKNOWN[1335] msg=audit(2020-02-18 15:26:50.775:10) : pid=1 uid=root auid=unset tty=(none) ses=unset subj=kernel comm=systemd exe=/usr/lib/systemd/systemd nl-mcgrp=1 op=connect res=yes
> > >
> > > And the events from the test suite:
> > >
> > > type=PROCTITLE msg=audit(2020-02-18 15:28:01.594:307) : proctitle=/usr/bin/perl -w amcast_joinpart/test
> > > type=SYSCALL msg=audit(2020-02-18 15:28:01.594:307) : arch=x86_64 syscall=bind success=yes exit=0 a0=0x7 a1=0x558ebc428be0 a2=0xc a3=0x0 items=0 ppid=642 pid=645 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=UNKNOWN[1335] msg=audit(2020-02-18 15:28:01.594:307) : pid=645 uid=root auid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=perl exe=/usr/bin/perl nl-mcgrp=1 op=connect res=yes
> > >
> > > type=PROCTITLE msg=audit(2020-03-17 11:35:31.474:344) : proctitle=/usr/bin/perl -w amcast_joinpart/test
> > > type=SYSCALL msg=audit(2020-03-17 11:35:31.474:344) : arch=x86_64 syscall=setsockopt success=yes exit=0 a0=0x7 a1=SOL_NETLINK a2=0x2 a3=0x7ffee21ca5f0 items=0 ppid=686 pid=689 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=3 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=UNKNOWN[1335] msg=audit(2020-03-17 11:35:31.474:344) : pid=689 uid=root auid=root tty=ttyS0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=perl exe=/usr/bin/perl nl-mcgrp=1 op=disconnect res=yes
> > >
> > > type=UNKNOWN[1335] msg=audit(2020-01-17 10:36:24.051:295) : pid=674 uid=root auid=root tty=ttyS0 ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=perl exe=/usr/bin/perl nl-mcgrp=1 op=disconnect res=yes
> >
> > This patch looks fine to me, but this line is curious ... I'm assuming
> > this is just a stray/cut-n-paste-error from the last time you updated
> > the commit description?  If so, just let me know and I can drop it
> > while merging, otherwise there is something odd going on ....
>
> That last line is the result of close() from an earlier version of the
> testsuite, rather than setsockopt(..., NETLINK_DROP_MEMBERSHIP, ...).
>
> This is why we need the subject attributes, as noted in the first note
> above the changelog below.

Argh.  I wasn't looking at the subject info, I was just noting the
timestamp was obviously wrong and the connects/disconnects didn't
match.

I was going to be nice and just drop that message during the merge,
but you need to regenerate those messages with the audit records from
just this patch since it is 1/3.  As it stands right now someone is
going to be very confused in a few years when they try to reconcile
the code changes with your commit description.

Regardless of the subject info, we should make sure the
connects/disconnects are matched.  The example you provide from the
test shows one connect, but two disconnects.  That is going to confuse
people looking through the audit logs.  I'm hoping you just did a
copy-n-paste error, but if not we need to figure that out and find a
way to make them match if there is somewhat to do so.

While you are respinning this patch, please fix the checkpatch.pl
errors; there were multiple line length, space/tab, and code indent
problems.  I was going to fix them during the merge, but you might as
well fix them now.  Feel free to insert my usual plea to run your
patches through checkpatch.pl before submitting.

> > > Please see the upstream issue tracker at
> > >   https://github.com/linux-audit/audit-kernel/issues/28
> > > With the feature description at
> > >   https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Multicast-Socket-Join-Part
> > > The testsuite support is at
> > >   https://github.com/rgbriggs/audit-testsuite/compare/ghak28-mcast-part-join
> > >   https://github.com/linux-audit/audit-testsuite/pull/93
> > > And the userspace support patch is at
> > >   https://github.com/linux-audit/audit-userspace/pull/114
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > >
> > > ---
> > > Note: subj attrs included due to missing syscall record for disconnect on close
> > > Note: tried refactor of subj attrs, but this is yet another new order.
> > >
> > > Changelog:
> > > v7:
> > > - rename audit_log_multicast_bind to audit_log_multicast
> > >
> > > v6:
> > > - rebased on 5.6-rc1 audit/next and audit log BPF
> > > - updated patch description sample records
> > >
> > > v5:
> > > - rebased on 5.5-rc1 audit/next
> > > - group bind/unbind ops
> > > - add audit context
> > > - justify message number skip
> > > - check audit_enabled
> > > - change field name from nlnk-grp to nl-mcgrp
> > > - fix whitespace issues
> > >
> > > v4:
> > > - 2017-10-13 sgrubb
> > > - squash to 1 patch
> > > - rebase on KERN_MODULE event
> > > - open code subj attrs
> > >
> > > v3:
> > > - 2016-11-30 sgrubb
> > > - rebase on REPLACE event
> > > - minimize audit_log_format calls
> > > - rename audit_log_bind to audit_log_multicast_bind
> > >
> > > v2:
> > > - 2015-07-23 sgrubb
> > > - spin off audit_log_task_simple in seperate patch
> > >
> > > v1:
> > > - 2014-10-07 rgb
> > > ---
> > >  include/uapi/linux/audit.h |  1 +
> > >  kernel/audit.c             | 48 ++++++++++++++++++++++++++++++++++++++++++----
> > >  2 files changed, 45 insertions(+), 4 deletions(-)

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists