Re: [PATCH ghak8 ALT4 V4 1/3] audit: show partial pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2018-02-15 18:19, Richard Guy Briggs wrote:
> On 2018-02-15 18:07, Steve Grubb wrote:
> > On Monday, February 12, 2018 12:02:21 AM EST Richard Guy Briggs 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 for a device that may
> > > no longer be visible upon log inspeciton, and an inode number.
> > > 
> > > Fill in the partial known pathname from the filesystem mount point to
> > > the leaf node 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 able to address them.
> > > 
> > > Sample output:
> > > type=PROCTITLE msg=audit(1488317694.446:143):
> > > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > > msg=audit(1488317694.446:143): item=797
> > > name=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 cap_fp= cap_fi= cap_fe=0
> > > cap_fver=0 type=PATH msg=audit(1488317694.446:143): item=796
> > > name=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
> > > cap_fp= cap_fi= cap_fe=0 cap_fver=0 ...
> > > type=PATH msg=audit(1488317694.446:143): item=1 name=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 cap_fp=
> > > cap_fi= cap_fe=0 cap_fver=0 type=PATH
> > > msg=audit(1488317694.446:143): item=0 name=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 cap_fp= cap_fi= cap_fe=0
> > > cap_fver=0 type=KERN_MODULE msg=audit(1488317694.446:143): name="nfsv4"
> > > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e 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"

So, updated sample output is:
type=SYSCALL msg=audit(1518738520.800:264): arch=c03e syscall=313 
success=yes exit=0 a0=8 a1=55c51f395fc6 a2=0 a3=8 items=834 ppid=579 pid=608 
auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 ses=1 
comm="modprobe" exe="/usr/bin/kmod" 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="mod-load"
type=KERN_MODULE msg=audit(1518738520.800:264): name="nfsv4"
type=PATH msg=audit(1518738520.800:264): item=0 name="events" inode=127 
dev=00:0b mode=040755 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=PARENT_ANON cap_fp= 
cap_fi= cap_fe=0 cap_fver=0 fstype=0x74726163
type=PATH msg=audit(1518738520.800:264): item=1 name="events/nfs4" inode=17795 
dev=00:0b mode=040755 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=CREATE_ANON cap_fp= 
cap_fi= cap_fe=0 cap_fver=0 fstype=0x74726163
...
type=PATH msg=audit(1518738520.800:264): item=832 
name="events/nfs4/nfs4_setclientid" inode=18206 dev=00:0b mode=040755 ouid=0 
ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT_ANON 
cap_fp= cap_fi= cap_fe=0 cap_fver=0 
fstype=0x74726163
type=PATH msg=audit(1518738520.800:264): item=833 
name="events/nfs4/nfs4_setclientid/format" inode=18211 dev=00:0b mode=0100444 
ouid=0 ogid=0 rdev=00:00 

Re: [PATCH ghak8 ALT4 V4 1/3] audit: show partial pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2018-02-15 18:07, Steve Grubb wrote:
> On Monday, February 12, 2018 12:02:21 AM EST Richard Guy Briggs 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 for a device that may
> > no longer be visible upon log inspeciton, and an inode number.
> > 
> > Fill in the partial known pathname from the filesystem mount point to
> > the leaf node 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 able to address them.
> > 
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=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 cap_fp= cap_fi= cap_fe=0
> > cap_fver=0 type=PATH msg=audit(1488317694.446:143): item=796
> > name=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
> > cap_fp= cap_fi= cap_fe=0 cap_fver=0 ...
> > type=PATH msg=audit(1488317694.446:143): item=1 name=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 cap_fp=
> > cap_fi= cap_fe=0 cap_fver=0 type=PATH
> > msg=audit(1488317694.446:143): item=0 name=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 cap_fp= cap_fi= cap_fe=0
> > cap_fver=0 type=KERN_MODULE msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e 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"
> 
> Thanks for the samples, but the event above fails the ausearch-test test 
> suite. The "name" field in the PATH record is not properly escaped.

Is the ausearch-test on github yet?  Last I see is v0.6 on your rh
people page.

> -Steve
> 
> > 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 
> > 
> > ---
> > v4:
> >   fix fullpath memleak
> >   switch from log_format() to audit_log_untrustedstring()
> >   remove leading / from pathname relative to unknown mount point
> > 
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> > 
> > v2:
> >   deconstify struct dentry*
> >   add hex prefix to fstype
> > ---
> >  include/linux/audit.h |  8 
> >  kernel/audit.c| 28 +++-
> >  kernel/audit.h|  1 +
> >  kernel/auditsc.c  |  8 +++-
> >  4 files changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..2020f1d 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -232,7 +232,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,
> > 

Re: [RFC PATCH ghak21 4/4] audit: add parent of refused symlink to audit_names

[Richard Guy Briggs]

On 2018-02-15 18:34, Paul Moore wrote:
> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs  wrote:
> > Audit link denied events for symlinks were missing the parent PATH
> > record.  Add it.  Since the full pathname may not be available,
> > reconstruct it from the path in the nameidata supplied.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/namei.c | 9 +
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 0edf133..bf1c046b 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -923,6 +923,7 @@ static inline int may_follow_link(struct nameidata *nd)
> > const struct inode *inode;
> > const struct inode *parent;
> > kuid_t puid;
> > +   char *pathname;
> >
> > if (!sysctl_protected_symlinks)
> > return 0;
> > @@ -945,6 +946,14 @@ static inline int may_follow_link(struct nameidata *nd)
> > if (nd->flags & LOOKUP_RCU)
> > return -ECHILD;
> >
> > +   pathname = kmalloc(PATH_MAX + 1, GFP_KERNEL);
> > +   if (!pathname)
> > +   return -ENOMEM;
> > +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> > pathname,
> > +  PATH_MAX + 1)),
> > +   nd->stack[0].link.dentry, 0);
> 
> Hmm, it's been a while since I've looked at the audit vfs/inode code,
> but isn't the audit_inode() call directly above effectively a
> duplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)
> call you added in patch 3/4?

It gets the full pathname string of the link, then converts it to a
struct filename*, and then below, we feed it that struct filename* with
the parent's dentry and the LOOKUP_PARENT flag to drop the last part of
the pathname and set the filetype to PARENT.

I didn't try, it, but I expect if I feed it parent's dentry above, I'd
get only the parent pathname and when I feed it to audit_inode() below
it would again drop the last part of the pathname when the LOOKUP_PARENT
flag is included, or not label it as a filetype PARENT if we don't
include the flag to get the full parent pathname.

> > +   audit_inode(nd->name, nd->stack[0].link.dentry->d_parent, 
> > LOOKUP_PARENT);
> > +
> > audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> > audit_log_link_denied("follow_link", >stack[0].link);
> > return -EACCES;
> > --
> > 1.8.3.1
> 
> -- 
> paul moore
> www.paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak21 4/4] audit: add parent of refused symlink to audit_names

[Paul Moore]

On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs  wrote:
> Audit link denied events for symlinks were missing the parent PATH
> record.  Add it.  Since the full pathname may not be available,
> reconstruct it from the path in the nameidata supplied.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/namei.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 0edf133..bf1c046b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -923,6 +923,7 @@ static inline int may_follow_link(struct nameidata *nd)
> const struct inode *inode;
> const struct inode *parent;
> kuid_t puid;
> +   char *pathname;
>
> if (!sysctl_protected_symlinks)
> return 0;
> @@ -945,6 +946,14 @@ static inline int may_follow_link(struct nameidata *nd)
> if (nd->flags & LOOKUP_RCU)
> return -ECHILD;
>
> +   pathname = kmalloc(PATH_MAX + 1, GFP_KERNEL);
> +   if (!pathname)
> +   return -ENOMEM;
> +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> pathname,
> +  PATH_MAX + 1)),
> +   nd->stack[0].link.dentry, 0);

Hmm, it's been a while since I've looked at the audit vfs/inode code,
but isn't the audit_inode() call directly above effectively a
duplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)
call you added in patch 3/4?

> +   audit_inode(nd->name, nd->stack[0].link.dentry->d_parent, 
> LOOKUP_PARENT);
> +
> audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> audit_log_link_denied("follow_link", >stack[0].link);
> return -EACCES;
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak8 ALT4 V4 1/3] audit: show partial pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2018-02-15 18:07, Steve Grubb wrote:
> On Monday, February 12, 2018 12:02:21 AM EST Richard Guy Briggs 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 for a device that may
> > no longer be visible upon log inspeciton, and an inode number.
> > 
> > Fill in the partial known pathname from the filesystem mount point to
> > the leaf node 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 able to address them.
> > 
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=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 cap_fp= cap_fi= cap_fe=0
> > cap_fver=0 type=PATH msg=audit(1488317694.446:143): item=796
> > name=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
> > cap_fp= cap_fi= cap_fe=0 cap_fver=0 ...
> > type=PATH msg=audit(1488317694.446:143): item=1 name=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 cap_fp=
> > cap_fi= cap_fe=0 cap_fver=0 type=PATH
> > msg=audit(1488317694.446:143): item=0 name=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 cap_fp= cap_fi= cap_fe=0
> > cap_fver=0 type=KERN_MODULE msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e 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"
> 
> Thanks for the samples, but the event above fails the ausearch-test test 
> suite. The "name" field in the PATH record is not properly escaped.

Let me re-run the ausearch-test on the log file from the machine in
question...  I don't remember if I copied the above from a recent run,
or just hand-edited the output to update it.  It should be fine since it
was updated to now run through audit_log_untrustedstring().

> -Steve
> 
> > 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 
> > 
> > ---
> > v4:
> >   fix fullpath memleak
> >   switch from log_format() to audit_log_untrustedstring()
> >   remove leading / from pathname relative to unknown mount point
> > 
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> > 
> > v2:
> >   deconstify struct dentry*
> >   add hex prefix to fstype
> > ---
> >  include/linux/audit.h |  8 
> >  kernel/audit.c| 28 +++-
> >  kernel/audit.h|  1 +
> >  kernel/auditsc.c  |  8 +++-
> >  4 files changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..2020f1d 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -232,7 +232,7 @@ extern void __audit_inode(struct filename *name, const
> > struct dentry *dentry, unsigned int flags);
> >  extern void __audit_file(const 

Re: [RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Richard Guy Briggs]

On 2018-02-15 15:43, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs  wrote:
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  kernel/auditfilter.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> I realize this is an RFC patchset, but considering recent patchsets I
> feel some clarification might be helpful to prevent future delays ...
> when submitting patchsets for merging please do not submit "fixup!"
> patches which fix problems in patches that are submitted earlier in
> the patchset, simply merge/squash the "fixup!" patches before
> submitting.

Yeah, the only reason this is a "fixup!" patch is to clearly show what
remedial step was (surprisingly) needed to fix the bug.

Same with the following "debug!" annotation.  Clearly not intended for
upstream to Linus.

> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 3343d1c..48dcb59 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, 
> > u32 *mask)
> >  static int audit_match_signal(struct audit_entry *entry)
> >  {
> > int i;
> > +   u32 archval;
> > struct audit_field *arch;
> >
> > for (i = 0; i < entry->rule.field_count; i++)
> > if (entry->rule.fields[i].type == AUDIT_ARCH) {
> > arch = >rule.fields[i];
> > +   archval = arch->val;
> > break;
> > }
> >
> > @@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
> >entry->rule.mask));
> > }
> >
> > -   switch(audit_classify_arch(arch->val)) {
> > +   switch(audit_classify_arch(archval)) {
> > case 0: /* native */
> > return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
> >entry->rule.mask));
> > --
> > 1.8.3.1
> 
> paul moore

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak8 ALT4 V4 1/3] audit: show partial pathname for entries with anonymous parents

[Steve Grubb]

On Monday, February 12, 2018 12:02:21 AM EST Richard Guy Briggs 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 for a device that may
> no longer be visible upon log inspeciton, and an inode number.
> 
> Fill in the partial known pathname from the filesystem mount point to
> the leaf node 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 able to address them.
> 
> Sample output:
> type=PROCTITLE msg=audit(1488317694.446:143):
> proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> msg=audit(1488317694.446:143): item=797
> name=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 cap_fp= cap_fi= cap_fe=0
> cap_fver=0 type=PATH msg=audit(1488317694.446:143): item=796
> name=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
> cap_fp= cap_fi= cap_fe=0 cap_fver=0 ...
> type=PATH msg=audit(1488317694.446:143): item=1 name=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 cap_fp=
> cap_fi= cap_fe=0 cap_fver=0 type=PATH
> msg=audit(1488317694.446:143): item=0 name=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 cap_fp= cap_fi= cap_fe=0
> cap_fver=0 type=KERN_MODULE msg=audit(1488317694.446:143): name="nfsv4"
> type=SYSCALL msg=audit(1488317694.446:143): arch=c03e 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"

Thanks for the samples, but the event above fails the ausearch-test test 
suite. The "name" field in the PATH record is not properly escaped.

-Steve

> 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 
> 
> ---
> v4:
>   fix fullpath memleak
>   switch from log_format() to audit_log_untrustedstring()
>   remove leading / from pathname relative to unknown mount point
> 
> v3:
>   fix audit_buffer leak and dname error allocation leak audit_log_name
>   only put audit_name->dentry if it is being replaced
> 
> v2:
>   deconstify struct dentry*
>   add hex prefix to fstype
> ---
>  include/linux/audit.h |  8 
>  kernel/audit.c| 28 +++-
>  kernel/audit.h|  1 +
>  kernel/auditsc.c  |  8 +++-
>  4 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..2020f1d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -232,7 +232,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);
> @@ -297,7 +297,7 @@ static inline void audit_inode_parent_hidden(struct
> filename *name, AUDIT_INODE_PARENT | 

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Paul Moore]

On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook  wrote:
> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs  wrote:
>> On 2018-02-14 09:51, Kees Cook wrote:
>>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs  wrote:
>>> > Audit link denied events emit disjointed records when audit is disabled.
>>> > No records should be emitted when audit is disabled.
>>> >
>>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>>> > Signed-off-by: Richard Guy Briggs 
>>> > ---
>>> >  kernel/audit.c | 3 +++
>>> >  1 file changed, 3 insertions(+)
>>> >
>>> > diff --git a/kernel/audit.c b/kernel/audit.c
>>> > index 227db99..4c3fd24 100644
>>> > --- a/kernel/audit.c
>>> > +++ b/kernel/audit.c
>>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, 
>>> > const struct path *link)
>>> > struct audit_buffer *ab;
>>> > struct audit_names *name;
>>> >
>>> > +   if (!audit_enabled || audit_dummy_context())
>>> > +   return;
>>> > +
>>> > name = kzalloc(sizeof(*name), GFP_NOFS);
>>> > if (!name)
>>> > return;
>>>
>>> Doesn't this means errors here would be silent if audit isn't enabled?
>>> I don't that; sysadmins should see this notification regardless of the
>>> audit state...
>>
>> This is a user error and not a system error, so I would think if system
>> auditing is disabled, they don't care about this kind of error.
>
> It could indicate an attack attempt...

We get beat up by several folks when we emit audit records with audit
disabled, and they have a very valid point.

I'm not arguing that the information isn't useful, I'm arguing that if
you are interested in the sort of information that audit provides you
should enable audit.  :)

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak8 ALT4 V4 0/3] audit: show more information for entries with anonymous parents

[Paul Moore]

On Mon, Feb 12, 2018 at 12:02 AM, Richard Guy Briggs  wrote:
> More than one filesystem was causing hundreds to thousands of null PATH
> records to be associated with the *init_module SYSCALL records on a few
> modules with corresponding audit syscall rules.
>
> This patchset adds extra information to those PATH records to provide
> insight into what is generating them, including a partial pathname,
> fstype field, and two new filetypes that indicate the pathname isn't
> anchored at the root of the task's root filesystem.
>
> Richard Guy Briggs (3):
>   audit: show partial pathname for entries with anonymous parents
>   audit: append new fstype field for anonymous PATH records
>   audit: add new filetypes CREATE_ANON and PARENT_ANON

The more I look at this, the more I prefer your original approach that
prefixed the relative pathname with the fstype.  Yes, I do realize
that you sort of work around that by including the fstype as a new
field in the PATH records, but we're still stuck with those odd
relative/un-rooted name fields.

Further, I don't recall ever hearing a good reason why the original
approach wasn't acceptable to Steve's userspace.  I know he did make
some very last minute hand-wavy comments, but none of those made any
sense to me; I don't understand why Steve's audit record parser is
even looking in the pathname string.

I'm going to park these patches in limbo for the time being.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH 2/3] fixup! audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs  wrote:
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/auditfilter.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

I realize this is an RFC patchset, but considering recent patchsets I
feel some clarification might be helpful to prevent future delays ...
when submitting patchsets for merging please do not submit "fixup!"
patches which fix problems in patches that are submitted earlier in
the patchset, simply merge/squash the "fixup!" patches before
submitting.

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 3343d1c..48dcb59 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -221,11 +221,13 @@ static inline int audit_match_class_bits(int class, u32 
> *mask)
>  static int audit_match_signal(struct audit_entry *entry)
>  {
> int i;
> +   u32 archval;
> struct audit_field *arch;
>
> for (i = 0; i < entry->rule.field_count; i++)
> if (entry->rule.fields[i].type == AUDIT_ARCH) {
> arch = >rule.fields[i];
> +   archval = arch->val;
> break;
> }
>
> @@ -238,7 +240,7 @@ static int audit_match_signal(struct audit_entry *entry)
>entry->rule.mask));
> }
>
> -   switch(audit_classify_arch(arch->val)) {
> +   switch(audit_classify_arch(archval)) {
> case 0: /* native */
> return (audit_match_class_bits(AUDIT_CLASS_SIGNAL,
>entry->rule.mask));
> --
> 1.8.3.1
>



-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

[Paul Moore]

On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs  wrote:
> The arch_f pointer was added to the struct audit_krule in commit:
> e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
>
> This is only used on addition and deletion of rules which isn't time
> critical and the arch field is likely to be one of the first fields,
> easily found iterating over the field type.  This isn't worth the
> additional complexity and storage.  Delete the field.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h |  1 -
>  kernel/auditfilter.c  | 12 
>  2 files changed, 8 insertions(+), 5 deletions(-)

I haven't decided if I like the removal of arch_f or not, but I think
I might know where your oops/panic is coming from, thoughts below ...

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..64a3b0e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -58,7 +58,6 @@ struct audit_krule {
> u32 field_count;
> char*filterkey; /* ties events to rules */
> struct audit_field  *fields;
> -   struct audit_field  *arch_f; /* quick access to arch field */
> struct audit_field  *inode_f; /* quick access to an inode field */
> struct audit_watch  *watch; /* associated watch */
> struct audit_tree   *tree;  /* associated watched tree */
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 739a6d2..3343d1c 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 
> *mask)
>
>  static int audit_match_signal(struct audit_entry *entry)
>  {
> -   struct audit_field *arch = entry->rule.arch_f;
> +   int i;
> +   struct audit_field *arch;
> +
> +   for (i = 0; i < entry->rule.field_count; i++)
> +   if (entry->rule.fields[i].type == AUDIT_ARCH) {
> +   arch = >rule.fields[i];
> +   break;
> +   }

In the original code arch_f was initialized to NULL via the allocator
so the arch local variable was guaranteed to have a valid value or
NULL.  Unfortunately, in your code if there is no AUDIT_ARCH field
arch could remain uninitialized which I believe could lead to the
oops/panic you are seeing.

> if (!arch) {
> /* When arch is unspecified, we must check both masks on 
> biarch
> @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct 
> audit_rule_data *data,
> if (!gid_valid(f->gid))
> goto exit_free;
> break;
> -   case AUDIT_ARCH:
> -   entry->rule.arch_f = f;
> -   break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH V3 0/2] audit: speed up audit syscall entry

[Paul Moore]

On Wed, Feb 14, 2018 at 9:47 PM, Richard Guy Briggs  wrote:
> These fixes should speed up audit syscall entry by doing away with the
> audit entry filter check, moving up the valid connection check before
> filling in the context and not caring if there is a bug when audit is
> disabled.
>
> Passes audit-testsuite.
> See: https://github.com/linux-audit/audit-kernel/issues/6
>
> v3:
>   - squash patch 1 and 2
> v2:
>   - bail earlier to avoid setting up unneeded state
>   - don't bother checking for bug when disabled
>
> Richard Guy Briggs (2):
>   audit: deprecate the AUDIT_FILTER_ENTRY filter
>   audit: bail before bug check if audit disabled
>
>  kernel/auditfilter.c |  4 ++--
>  kernel/auditsc.c | 22 ++
>  2 files changed, 12 insertions(+), 14 deletions(-)

Both patches merged into audit/next, thanks.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit