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;
From:   Steve Grubb <sgrubb@redhat.com>
To:     linux-audit@redhat.com
Cc:     Paul Moore <paul@paul-moore.com>,
        Richard Guy Briggs <rgb@redhat.com>,
        linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
Date:   Wed, 08 Nov 2017 18:29:48 -0500
Message-ID: <5662600.QY0GDuKsRv@x2>
Organization: Red Hat
In-Reply-To: <CAHC9VhTa_v4i_7f60uYJRWwJtXKLY25j2cq2anMD29wmZqTsmA@mail.gmail.com>
References: <fa0938a05c2d783556d67d4dafef208b156a35ad.1503484357.git.rgb@redhat.com> <CAHC9VhTa_v4i_7f60uYJRWwJtXKLY25j2cq2anMD29wmZqTsmA@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > 
> > startup:
> >         -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> > 
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> > 
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name" 
field - which is undesirable. Please put the file system type in a field all 
by itself called "fstype". You can just leave it as the hex magic number 
prepended with 0x and user space can do the lookup from there,

It might be simplest to just apply a corrective patch over top of this one so 
that you don't have to muck about with git branches and commit messages.

-Steve
 
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> > 
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> > 
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE type=PATH
> > msg=audit(1488317694.446:143): item=796
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964
> > dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=PARENT ...
> > type=PATH msg=audit(1488317694.446:143): item=1
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0
> > nametype=CREATE type=PATH msg=audit(1488317694.446:143): item=0
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6
> > pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod"
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > 
> > ---
> > 
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> > 
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > 
> > ---
> > 
> >  include/linux/audit.h |    8 ++++----
> >  kernel/audit.c        |   19 +++++++++++++++++++
> >  kernel/audit.h        |    1 +
> >  kernel/auditsc.c      |    8 +++++++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > 
> >  #include <linux/freezer.h>
> >  #include <linux/pid_namespace.h>
> >  #include <net/netns/generic.h>
> > 
> > +#include <linux/dcache.h>
> > 
> >  #include "audit.h"
> > 
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,> 
> >         name->gid   = inode->i_gid;
> >         name->rdev  = inode->i_rdev;
> >         security_inode_getsecid(inode, &name->osid);
> > 
> > +       if (name->dentry) {
> > +               dput(name->dentry);
> > +               name->dentry = NULL;
> > +       }
> > 
> >         audit_copy_fcaps(name, dentry);
> >  
> >  }
> > 
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context,
> > struct audit_names *n,> 
> >                         audit_log_n_untrustedstring(ab, n->name->name,
> >                         
> >                                                     n->name_len);
> >                 
> >                 }
> > 
> > +       } else if (n->dentry) {
> > +               char *fullpath;
> > +               const char *fullpathp = NULL;
> > +
> > +               fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > +               if (fullpath)
> > +                       fullpathp = dentry_path_raw(n->dentry, fullpath,
> > PATH_MAX); +                       if (IS_ERR(fullpathp)) {
> > +                               fullpathp = NULL;
> > +                               kfree(fullpath);
> > +                       }
> > +               audit_log_format(ab, " name=%s(0x%lx):%s",
> > +                                n->dentry->d_sb->s_type->name ?: "?",
> > +                                n->dentry->d_sb->s_magic, fullpathp ?:
> > "?");> 
> >         } else
> >         
> >                 audit_log_format(ab, " name=(null)");
> 
> While looking this over one more time before merging I realized that
> you are missing the curly braces in the "if (fullpath)" if-statement
> above.  This is an easy fix, and appears to be the Right Thing, so I'm
> just going to fix up the patch while merging; take a look at the
> result in the audit/next tree and if you have any objections let me
> know so I can back it out.
> 
> I'm also only merging this patch right now, patch 2/2 needs to wait
> until the corresponding userspace is ready so we can test/verify it.


From 1581013811445819052@xxx Thu Oct 12 01:37:45 +0000 2017
X-GM-THRID: 1576519731154696149
X-Gmail-Labels: Inbox,Category Forums