| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170131200226.GF27141@madcap2.tricolour.ca>
Date: Tue, 31 Jan 2017 15:02:26 -0500
From: Richard Guy Briggs <rgb@...hat.com>
To: Paul Moore <paul@...l-moore.com>, Steve Grubb <sgrubb@...hat.com>
Cc: Rusty Russell <rusty@...tcorp.com.au>, linux-audit@...hat.com,
linux-kernel@...r.kernel.org, Jessica Yu <jeyu@...hat.com>
Subject: Re: [RFC] [PATCH] audit: log module name on init_module
On 2017-01-31 11:07, Paul Moore wrote:
> On Tue, Jan 31, 2017 at 7:36 AM, Richard Guy Briggs <rgb@...hat.com> wrote:
> > On 2017-01-31 06:59, Paul Moore wrote:
> >> On Thu, Jan 26, 2017 at 4:21 PM, Richard Guy Briggs <rgb@...hat.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@...hat.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).
Do you see a need to include module initialization arguments? It sounds
potentially useful to me, but also potentially bandwidth-consuming. I
have a prototype patch to add the args as one encoded field. Along with
the addition of this field is the concern about message lengths and
buffer allocations since it is an encoded field that would need twice
the length of the argment text to store in the message.
> 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.
Right, so more similar to SERVICE/DAEMON START/STOP, or using an op=
field...
> >> > 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@...hat.com>
> > Kernel Security Engineering, Base Operating Systems, Red Hat
> > Remote, Ottawa, Canada
> > Voice: +1.647.777.2635, Internal: (81) 32635
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@...hat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
Powered by blists - more mailing lists