Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <ca70ee17d85860aa599e0001a75d639d819de7ae.1579292286.git.rgb@redhat.com>
 <CAHC9VhR9p+aOTzv7g-ujuMsMtLvOZKkoKJWsthZnj38rzJe1TA@mail.gmail.com>
 <20200122230742.7vwtvmhhjerray5f@madcap2.tricolour.ca> <CAHC9VhTcv9E8DUDJ2Y-PzXmU0_+ufVydbPB3Q_Fhb8-7TUZMmg@mail.gmail.com>
 <20200123161349.z55l2dd7qsyhoxbn@madcap2.tricolour.ca>
In-Reply-To: <20200123161349.z55l2dd7qsyhoxbn@madcap2.tricolour.ca>
From:   Paul Moore <paul@paul-moore.com>
Date:   Thu, 23 Jan 2020 11:57:38 -0500
Message-ID: <CAHC9VhTEfZXCV6TwJ4KOoDCea3x5i85_gBmMi=cygGG9OQCGOQ@mail.gmail.com>
Subject: Re: [PATCH ghak28 V4] audit: log audit netlink multicast bind and
 unbind events
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     nhorman@redhat.com, LKML <linux-kernel@vger.kernel.org>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        Eric Paris <eparis@parisplace.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 23, 2020 at 11:14 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-01-23 09:32, Paul Moore wrote:
> > On Wed, Jan 22, 2020 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-01-22 17:40, Paul Moore wrote:
> > > > On Fri, Jan 17, 2020 at 3:21 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > ...
> >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 17b0d523afb3..478259f3fa53 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -1520,20 +1520,60 @@ static void audit_receive(struct sk_buff  *skb)
> > > > >         audit_ctl_unlock();
> > > > >  }
> > > > >
> > > > > +/* Log information about who is connecting to the audit multicast socket */
> > > > > +static void audit_log_multicast_bind(int group, const char *op, int err)
> > > > > +{
> > > > > +       const struct cred *cred;
> > > > > +       struct tty_struct *tty;
> > > > > +       char comm[sizeof(current->comm)];
> > > > > +       struct audit_buffer *ab;
> > > > > +
> > > > > +       if (!audit_enabled)
> > > > > +               return;
> > > > > +
> > > > > +       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_EVENT_LISTENER);
> > > > > +       if (!ab)
> > > > > +               return;
> > > > > +
> > > > > +       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));
> > > >
> > > > Don't we already get all of that information as part of the syscall record?
> > >
> > > Yes.  However, the syscall record isn't always present.  One example is
> > > systemd, shown above.
> >
> > Assuming that the system supports syscall auditing, the absence of a
> > syscall record is a configuration choice made by the admin.  If the
> > system doesn't support syscall auditing the obvious "fix" is to do the
> > work to enable syscall auditing on that platform ... but now we're
> > starting to get off topic.
>
> Well, the system did spit out a syscall record with the example above,
> so it has support for syscall auditing.
>
> I'm testing on f30 with an upstream kernel, the standard 30-stig ruleset and
> with kernel command line audit=1.  What else is needed to support a syscall
> record on systemd before any audit rules have been put in place?  We may still
> have a bug here that affects early process auditing.  What am I missing?
>
> If we can get that sorted out, we don't need subject attributes in this record.

It looks like some debugging is in order.  There must be some sort of
action initiated by userspace which is causing the multicast
"op=connect", right?  Find out what that is and why it isn't
generating a syscall record (maybe it's not a syscall? I don't know
what systemd is doing here).

> > > The other is the disconnect record, shown above,
> > > which may be asynchronous, or an unmonitored syscall (It could only be
> > > setsockopt, close, shutdown.).
> >
> > An unmonitored syscall still falls under the category of a
> > configuration choice so I'm not too concerned about that, but the
> > async disconnect record is legitimate.  Can you provide more
> > information about when this occurs?  I'm guessing this is pretty much
> > just an abrupt/abnormal program exit?
>
> Again, what configuration choice are you talking about?
> "-a task,never"?  That isn't active on this system.
>
> The output was produced by the test case quoted in the patch description.
>
> I should not have had to put a rule in place to do syscall auditing on connect,
> bind, setsockopt, close, shutdown.
>
> The disconnect would have been due to a perl close() call.  I would not have
> expected that to be async, but I don't know the details of what the perl
> implementation does.

You mentioned two cases: unmonitored syscalls and async records (I
assumed these were just "disconnect").  Monitoring a syscall is a
configuration choice, regardless of what the defaults may be, and
since the folks likely to care about these multicast events are the
same sort of folks that care deeply about audit, asking them to do
some additional configuration tweaks seems like a reasonable thing to
get this new record with the proper information.  The async records
are potentially more interesting, but less clear, which is why I asked
for more info.

Regardless, all of this is pretty moot if we decide we don't care
about duplicate information.  Let's make a decision on duplicate
fields across multiple records before we worry too much about the rest
of what we are discussing.

> > > > I'm pretty sure these are the same arguments I made when Steve posted
> > > > a prior version of this patch.
> > >
> > > You did.  I would really like to have dropped them, but they aren't
> > > reliably available.
> >
> > Personally I'm not too worried if we have duplicate information spread
> > across records in a single event, as long as they are consistent.
> > However, I remember Steve complaining rather loudly about duplicated
> > fields across records in a single event some time back; perhaps that
> > is not a concern of his anymore (perhaps it was a narrow case at the
> > time), I don't know.
> >
> > Here is the deal, either duplicated information is something we are
> > okay with, or it is something to avoid; we need to pick one.  As
> > mentioned above, I don't really care that much either way (I have a
> > slight preference, but I don't feel strongly enough to fight for it),
> > so let's hear the arguments both for and against and decide - whatever
> > we pick I'll enforce so long as we are stuck with this string format.
>
> Steve, can you say why this order should be the standard?  From:
>         http://people.redhat.com/sgrubb/audit/record-fields.html
>
> I get:
>         SYSCALL/ANOM_LINK/FEATURE_CHANGE
>                 ppid    pid     auid    uid     gid     euid    suid    fsuid   egid    sgid    fsgid   tty     ses     comm    exe     subj

Oh man, let's please not have *another* debate about field ordering
before we answer the duplicate field question.  If history has shown
us anything, it is that debates around audit record field ordering
tend to kill progress.  Let's try to stay focused.

-- 
paul moore
www.paul-moore.com