Re: [PATCH ALT4 V2 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs <rgb@redhat.com>]

On 2017-04-04 17:19, Paul Moore wrote:
> On Tue, Apr 4, 2017 at 5:21 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().
> >
> > 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>
> > ---
> >  include/linux/audit.h |    8 ++++----
> >  kernel/audit.c        |   16 ++++++++++++++++
> >  kernel/audit.h        |    1 +
> >  kernel/auditsc.c      |    8 +++++++-
> >  4 files changed, 28 insertions(+), 5 deletions(-)
> 
> I see the "ALT4" in the subject line which gives me pause; does this
> mean you plan further development for the other potential fixes, or
> did you just include it on this patch for better tracking with it's
> predecessor?

The "ALT4" was included for tracking.  This is the planned path forward,
abandonning the rest of the "ALT*"s.

> I'm basically asking what to expect.  Should I review this for
> inclusion, or wait for other alternatives?

Please review.

> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index aba3a26..367a03a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -241,7 +241,7 @@ extern void __audit_inode(struct filename *name, const struct dentry *dentry,
> >                                 unsigned int flags);
> >  extern void __audit_file(const struct file *);
> >  extern void __audit_inode_child(struct inode *parent,
> > -                               const struct dentry *dentry,
> > +                               struct dentry *dentry,
> >                                 const unsigned char type);
> >  extern void __audit_seccomp(unsigned long syscall, long signr, int code);
> >  extern void __audit_ptrace(struct task_struct *t);
> > @@ -306,7 +306,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
> >                                 AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
> >  }
> >  static inline void audit_inode_child(struct inode *parent,
> > -                                    const struct dentry *dentry,
> > +                                    struct dentry *dentry,
> >                                      const unsigned char type) {
> >         if (unlikely(!audit_dummy_context()))
> >                 __audit_inode_child(parent, dentry, type);
> > @@ -487,7 +487,7 @@ static inline void __audit_inode(struct filename *name,
> >                                         unsigned int flags)
> >  { }
> >  static inline void __audit_inode_child(struct inode *parent,
> > -                                       const struct dentry *dentry,
> > +                                       struct dentry *dentry,
> >                                         const unsigned char type)
> >  { }
> >  static inline void audit_inode(struct filename *name,
> > @@ -501,7 +501,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
> >                                 const struct dentry *dentry)
> >  { }
> >  static inline void audit_inode_child(struct inode *parent,
> > -                                    const struct dentry *dentry,
> > +                                    struct dentry *dentry,
> >                                      const unsigned char type)
> >  { }
> >  static inline void audit_core_dumps(long signr)
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 25dd70a..7d83c5a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -66,6 +66,7 @@
> >  #include <linux/freezer.h>
> >  #include <linux/pid_namespace.h>
> >  #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> >  #include "audit.h"
> >
> > @@ -1884,6 +1885,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);
> >  }
> >
> > @@ -1925,6 +1930,17 @@ 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;
> > +
> > +               fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > +               if (!fullpath)
> > +                       return;
> > +               fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> > +               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)");
> >
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 144b7eb..2a11583 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -84,6 +84,7 @@ struct audit_names {
> >
> >         unsigned long           ino;
> >         dev_t                   dev;
> > +       struct dentry           *dentry;
> >         umode_t                 mode;
> >         kuid_t                  uid;
> >         kgid_t                  gid;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 4db32e8..b3797c7 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -74,6 +74,7 @@
> >  #include <linux/string.h>
> >  #include <linux/uaccess.h>
> >  #include <uapi/linux/limits.h>
> > +#include <linux/dcache.h>
> >
> >  #include "audit.h"
> >
> > @@ -881,6 +882,8 @@ static inline void audit_free_names(struct audit_context *context)
> >                 list_del(&n->list);
> >                 if (n->name)
> >                         putname(n->name);
> > +               if (n->dentry)
> > +                       dput(n->dentry);
> >                 if (n->should_free)
> >                         kfree(n);
> >         }
> > @@ -1858,7 +1861,7 @@ void __audit_file(const struct file *file)
> >   * unsuccessful attempts.
> >   */
> >  void __audit_inode_child(struct inode *parent,
> > -                        const struct dentry *dentry,
> > +                        struct dentry *dentry,
> >                          const unsigned char type)
> >  {
> >         struct audit_context *context = current->audit_context;
> > @@ -1914,6 +1917,7 @@ void __audit_inode_child(struct inode *parent,
> >                 if (!n)
> >                         return;
> >                 audit_copy_inode(n, NULL, parent);
> > +               n->dentry = dget_parent(dentry);
> >         }
> >
> >         if (!found_child) {
> > @@ -1935,6 +1939,8 @@ void __audit_inode_child(struct inode *parent,
> >                 audit_copy_inode(found_child, dentry, inode);
> >         else
> >                 found_child->ino = AUDIT_INO_UNSET;
> > +       if (!found_parent)
> > +               found_child->dentry = dget(dentry);
> >  }
> >  EXPORT_SYMBOL_GPL(__audit_inode_child);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- 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