Re: [RFC PATCH ghak9 2/3] audit: Add a function to log the path of an fd

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

On 2018-07-16 10:29, Ondrej Mosnacek wrote:
> On Fri, Jul 13, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> > > The function logs an FD_PATH record that is associated with the current
> > > syscall. The record associates the given file descriptor with the
> > > current path of the file under it (if it is possible to retrieve such
> > > path). The reader of the log can then logically connect this information
> > > to the syscall arguments from the SYSCALL record (based on the syscall
> > > type).
> > >
> > > Record format:
> > > type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  include/linux/audit.h | 10 ++++++++++
> > >  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 46 insertions(+)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 9334fbef7bae..95d338bb603a 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
> > >  extern void __audit_mmap_fd(int fd, int flags);
> > >  extern void __audit_log_kern_module(char *name);
> > >  extern void __audit_fanotify(unsigned int response);
> > > +extern void __audit_fd_path(int fd);
> > >
> > >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> > >  {
> > > @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int response)
> > >               __audit_fanotify(response);
> > >  }
> > >
> > > +static inline void audit_fd_path(int fd)
> > > +{
> > > +     if (fd >= 0 && !audit_dummy_context())
> >
> > Isn't an fd of 0 valid?
> 
> It is treated as valid by the above condition (it only rejects
> negative values), so I'm not sure if you mean "valid" or "invalid"...
> I suppose an fd of 0 is unlikely to be used as dirfd in openat(2) et
> al., but in general it is a valid fd and I don't think we should
> explicitly exclude it here. The corresponding syscalls' input checks
> will already filter out values that are invalid for them.

Sorry, that must have been a brain fart on my part.  I must have seen
">" when I first reviewed it, then later saw ">="...  so that should be
fine.

> > > +             __audit_fd_path(fd);
> > > +}
> > > +
> > >  extern int audit_n_rules;
> > >  extern int audit_signals;
> > >  #else /* CONFIG_AUDITSYSCALL */
> > > @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
> > >  static inline void audit_fanotify(unsigned int response)
> > >  { }
> > >
> > > +static inline void audit_fd_path(int fd)
> > > +{ }
> > > +
> > >  static inline void audit_ptrace(struct task_struct *t)
> > >  { }
> > >  #define audit_n_rules 0
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d762e0b8160e..82dad69213a2 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -74,6 +74,8 @@
> > >  #include <linux/string.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/fsnotify_backend.h>
> > > +#include <linux/file.h>
> > > +#include <linux/dcache.h>
> > >  #include <uapi/linux/limits.h>
> > >
> > >  #include "audit.h"
> > > @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
> > >               AUDIT_FANOTIFY, "resp=%u", response);
> > >  }
> > >
> > > +void __audit_fd_path(int fd)
> > > +{
> > > +     struct audit_buffer *ab;
> > > +     struct file *file;
> > > +     char *buf, *path;
> > > +
> > > +     if (!audit_enabled)
> > > +             return;
> > > +
> > > +     file = fget_raw(fd);
> > > +     if (!file)
> > > +             return;
> > > +
> > > +     buf = kmalloc(PATH_MAX, GFP_KERNEL);
> > > +     if (!buf)
> >
> > I think we need an fput(file) here.
> 
> Indeed we do, will fix in next revision.
> 
> >
> > > +             return;
> > > +
> > > +     path_get(&file->f_path);
> > > +     path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> > > +     path_put(&file->f_path);
> > > +     fput(file);
> > > +     if (!path || IS_ERR(path))
> > > +             goto free_buf;
> > > +
> > > +     ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> > > +     if (unlikely(!ab))
> > > +             goto free_buf;
> > > +     audit_log_format(ab, "fd=%i path=", fd);
> > > +     audit_log_untrustedstring(ab, path);
> > > +     audit_log_end(ab);
> > > +free_buf:
> > > +     kfree(buf);
> > > +}
> > > +
> > >  static void audit_log_task(struct audit_buffer *ab)
> > >  {
> > >       kuid_t auid, uid;
> >
> > - 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
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- 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