Re: [RFC] [PATCH] audit: log module name on init_module

[Steve Grubb <sgrubb@redhat.com>]

On Tue, 31 Jan 2017 11:07:24 -0500
Paul Moore <paul@paul-moore.com> wrote:

> On Tue, Jan 31, 2017 at 7:36 AM, Richard Guy Briggs <rgb@redhat.com>
> wrote:
> > On 2017-01-31 06:59, Paul Moore wrote:  
> >> On Thu, Jan 26, 2017 at 4:21 PM, Richard Guy Briggs
> >> <rgb@redhat.com> wrote:  
> >> > This adds a new auxiliary record MODULE_INIT to the SYSCALL
> >> > event.
> >> >
> >> > We get finit_module for free since it made most sense to hook
> >> > this in to load_module().
> >> >
> >> > https://github.com/linux-audit/audit-kernel/issues/7
> >> > https://github.com/linux-audit/audit-kernel/wiki/RFE-Module-load-record-format  
> >>
> >> Consistency nit: capitalize the first letter in the wiki page words
> >> (see the existing RFE pages)
> >>  
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> >  include/linux/audit.h      |   12 ++++++++++++
> >> >  include/uapi/linux/audit.h |    1 +
> >> >  kernel/audit.h             |    3 +++
> >> >  kernel/auditsc.c           |   20 ++++++++++++++++++++
> >> >  kernel/module.c            |    5 ++++-
> >> >  5 files changed, 40 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> >> > index 2be99b2..7bb23d5 100644
> >> > --- a/include/linux/audit.h
> >> > +++ b/include/linux/audit.h
> >> > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct
> >> > linux_binprm *bprm, const struct cred *old);
> >> >  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_module_init(char *name);
> >> >
> >> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >> >  {
> >> > @@ -450,6 +451,12 @@ static inline void audit_mmap_fd(int fd,
> >> > int flags) __audit_mmap_fd(fd, flags);
> >> >  }
> >> >
> >> > +static inline void audit_module_init(char *name)
> >> > +{
> >> > +       if (!audit_dummy_context())
> >> > +               __audit_module_init(name);
> >> > +}  
> >>
> >> More on this below, but I was expecting the function above to named
> >> audit_log_kern_module(), or something similar.  
> >
> > Ok fair enough, I had mis-understood your previous point.  
> 
> I probably could have been more specific too.
> 
> > Any comment on the new record format?  
> 
> Not really, it's just the single field so it's kinda hard to have
> anything meaningful to say.  We obviously need to worry about the
> field name, but I'll let Steve speak to that as that likely means more
> to him than it does to me.  From my perspective, "name" seems
> perfectly reasonable, especially since it is in the context of a
> module specific record (no real worries about it being ambiguous).

I'm fine with "name".

> I guess that does bring up the question of the record name, right now
> it is AUDIT_MODULE_INIT ... do we want it to be specific to init/load
> or do we want it more generic with an "op=" field (you mentioned some
> thoughts both ways in the GH issue).  I'm not sure I care too much
> either way, but once again I imagine Steve may have a preference for
> one over the other.

If this does not hook delete_module, then I see no reason for an op=
field. But even if it were to, the operation is implicit in the syscall
name. So, I don't think anything else is needed.

-Steve


> >> >  extern int audit_n_rules;
> >> >  extern int audit_signals;
> >> >  #else /* CONFIG_AUDITSYSCALL */
> >> > @@ -561,6 +568,11 @@ static inline void audit_log_capset(const
> >> > struct cred *new, { }
> >> >  static inline void audit_mmap_fd(int fd, int flags)
> >> >  { }
> >> > +
> >> > +static inline void audit_module_init(char *name)
> >> > +{
> >> > +}
> >> > +
> >> >  static inline void audit_ptrace(struct task_struct *t)
> >> >  { }
> >> >  #define audit_n_rules 0
> >> > diff --git a/include/uapi/linux/audit.h
> >> > b/include/uapi/linux/audit.h index 3f24110..4a328b4 100644
> >> > --- a/include/uapi/linux/audit.h
> >> > +++ b/include/uapi/linux/audit.h
> >> > @@ -111,6 +111,7 @@
> >> >  #define AUDIT_PROCTITLE                1327    /* Proctitle
> >> > emit event */ #define AUDIT_FEATURE_CHANGE   1328    /* audit
> >> > log listing feature changes */ #define AUDIT_REPLACE
> >> > 1329    /* Replace auditd if this packet unanswerd */ +#define
> >> > AUDIT_MODULE_INIT      1330    /* Module init event */
> >> >
> >> >  #define AUDIT_AVC              1400    /* SE Linux avc denial
> >> > or grant */ #define AUDIT_SELINUX_ERR      1401    /* Internal
> >> > SE Linux Errors */ diff --git a/kernel/audit.h b/kernel/audit.h
> >> > index 431444c..144b7eb 100644
> >> > --- a/kernel/audit.h
> >> > +++ b/kernel/audit.h
> >> > @@ -199,6 +199,9 @@ struct audit_context {
> >> >                 struct {
> >> >                         int                     argc;
> >> >                 } execve;
> >> > +               struct {
> >> > +                       char                    *name;
> >> > +               } module;
> >> >         };
> >> >         int fds[2];
> >> >         struct audit_proctitle proctitle;
> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> > index bb5f504..3e12678 100644
> >> > --- a/kernel/auditsc.c
> >> > +++ b/kernel/auditsc.c
> >> > @@ -1172,6 +1172,14 @@ out:
> >> >         kfree(buf_head);
> >> >  }
> >> >
> >> > +static void audit_log_kern_module(struct audit_context *context,
> >> > +                                 struct audit_buffer **ab)
> >> > +{
> >> > +       audit_log_format(*ab, " name=");
> >> > +       audit_log_untrustedstring(*ab, context->module.name);
> >> > +       kfree(context->module.name);
> >> > +}
> >> > +
> >> >  static void show_special(struct audit_context *context, int
> >> > *call_panic) {
> >> >         struct audit_buffer *ab;
> >> > @@ -1268,6 +1276,9 @@ static void show_special(struct
> >> > audit_context *context, int *call_panic) case AUDIT_EXECVE: {
> >> >                 audit_log_execve_info(context, &ab);
> >> >                 break; }
> >> > +       case AUDIT_MODULE_INIT:
> >> > +               audit_log_kern_module(context, &ab);
> >> > +               break;  
> >>
> >> With the exception of AUDIT_EXECVE everything else in
> >> show_special() is simply open coded inside the show_special()
> >> function; considering that audit_log_kern_module() is trivial it
> >> seems like a separate function isn't really needed here.
> >>  
> >> >         }
> >> >         audit_log_end(ab);
> >> >  }
> >> > @@ -2368,6 +2379,15 @@ void __audit_mmap_fd(int fd, int flags)
> >> >         context->type = AUDIT_MMAP;
> >> >  }
> >> >
> >> > +void __audit_module_init(char *name)
> >> > +{
> >> > +       struct audit_context *context = current->audit_context;
> >> > +
> >> > +       context->module.name = kmalloc(strlen(name) + 1,
> >> > GFP_KERNEL);
> >> > +       strcpy(context->module.name, name);
> >> > +       context->type = AUDIT_MODULE_INIT;
> >> > +}
> >> > +
> >> >  static void audit_log_task(struct audit_buffer *ab)
> >> >  {
> >> >         kuid_t auid, uid;
> >> > diff --git a/kernel/module.c b/kernel/module.c
> >> > index 529efae..678407e 100644
> >> > --- a/kernel/module.c
> >> > +++ b/kernel/module.c
> >> > @@ -61,6 +61,7 @@
> >> >  #include <linux/pfn.h>
> >> >  #include <linux/bsearch.h>
> >> >  #include <linux/dynamic_debug.h>
> >> > +#include <linux/audit.h>
> >> >  #include <uapi/linux/module.h>
> >> >  #include "module-internal.h"
> >> >
> >> > @@ -3593,6 +3594,8 @@ static int load_module(struct load_info
> >> > *info, const char __user *uargs, goto free_copy;
> >> >         }
> >> >
> >> > +       audit_module_init(mod->name);
> >> > +
> >> >         /* Reserve our place in the list. */
> >> >         err = add_unformed_module(mod);
> >> >         if (err)
> >> > @@ -3681,7 +3684,7 @@ static int load_module(struct load_info
> >> > *info, const char __user *uargs, mod->name, after_dashes);
> >> >         }
> >> >
> >> > -       /* Link in to syfs. */
> >> > +       /* Link in to sysfs. */
> >> >         err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
> >> >         if (err < 0)
> >> >                 goto coming_cleanup;  
> >>
> >> paul moore  
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs <rgb@redhat.com>
> > Kernel Security Engineering, Base Operating Systems, Red Hat
> > Remote, Ottawa, Canada
> > Voice: +1.647.777.2635, Internal: (81) 32635  
> 
> 
>