| Message ID | 1511803118-2552-2-git-send-email-tixxdz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Mostly typos/spellos... On 11/27/2017 09:18 AM, Djalal Harouni wrote: > Cc: Serge Hallyn <serge@hallyn.com> > Cc: Andy Lutomirski <luto@kernel.org> > Suggested-by: Rusty Russell <rusty@rustcorp.com.au> > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com> > --- > include/linux/kmod.h | 65 ++++++++++++++++++++++++++++++++++++++++++----- > include/linux/lsm_hooks.h | 6 ++++- > include/linux/security.h | 7 +++-- > kernel/kmod.c | 29 ++++++++++++++++----- > security/security.c | 6 +++-- > security/selinux/hooks.c | 3 ++- > 6 files changed, 97 insertions(+), 19 deletions(-) > > diff --git a/include/linux/kmod.h b/include/linux/kmod.h > index 40c89ad..ccd6a1c 100644 > --- a/include/linux/kmod.h > +++ b/include/linux/kmod.h > @@ -33,16 +33,67 @@ > +/** > + * request_module Try to load a kernel module > + * > + * Automatically loads the request module. > + * > + * @mod...: The module name > + */ what are the "..." for? what do they do here? > +#define request_module(mod...) __request_module(true, -1, NULL, mod) > + > +#define request_module_nowait(mod...) __request_module(false, -1, NULL, mod) > + > +/** > + * request_module_cap Load kernel module only if the required capability is set > + * > + * Automatically load a module if the required capability is set and it > + * corresponds to the appropriate subsystem that is indicated by prefix. > + * > + * This allows to load aliased modules like 'netdev-%s' with CAP_NET_ADMIN. > + * > + * ex: > + * request_module_cap(CAP_NET_ADMIN, "netdev", "%s", mod); > + * > + * @required_cap: Required capability to load the module > + * @prefix: The module prefix if any, otherwise NULL > + * @fmt: printf style format string for the name of the module with its > + * arguments if any > + * > + * If '@required_cap' is positive, the security subsystem will check if > + * '@prefix' is set and if caller has the required capability then the > + * operation is allowed. > + * The security subsystem can not make assumption about the boundaries > + * of other subsystems, it is their responsability to make a call with responsibility > + * the right capability and module alias. > + * > + * If '@required_cap' is positive and '@prefix' is NULL then we assume > + * that the '@required_cap' is CAP_SYS_MODULE. > + * > + * If '@required_cap' is negative then there are no permission checks, this > + * is the equivalent to request_module() function. > + * > + * This function trust callers to pass the right capability with the trusts > + * appropriate prefix. > + * > + * Note: the permission checks may still fail, even if the required > + * capability is negative, this is due to module loading restrictions negative; this > + * that are controlled by the enduser. > + */ > +#define request_module_cap(required_cap, prefix, fmt...) \ > + __request_module(true, required_cap, prefix, fmt) > + > #endif /* __LINUX_KMOD_H__ */ > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 7161d8e..d898bd0 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -571,6 +571,9 @@ > * Ability to trigger the kernel to automatically upcall to userspace for > * userspace to load a kernel module with the given name. > * @kmod_name name of the module requested by the kernel > + * @required_cap If positive, the required capability to automatically load > + * the correspondig kernel module. corresponding > + * @prefix The prefix of the module if any. Can be NULL. > * Return 0 if successful. > * @kernel_read_file: > * Read a file specified by userspace. > diff --git a/kernel/kmod.c b/kernel/kmod.c > index bc6addd..679d401 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -109,6 +109,8 @@ static int call_modprobe(char *module_name, int wait) > /** > * __request_module - try to load a kernel module > * @wait: wait (or not) for the operation to complete > + * @required_cap: required capability to load the module > + * @prefix: module prefix if any otherwise NULL > * @fmt: printf style format string for the name of the module > * @...: arguments as specified in the format string > * > @@ -119,14 +121,20 @@ static int call_modprobe(char *module_name, int wait) > * must check that the service they requested is now available not blindly > * invoke it. > * > - * If module auto-loading support is disabled then this function > - * becomes a no-operation. > + * If "required_cap" is positive, The security subsystem will trust the caller the > + * that if it has "required_cap" then it may allow to load some modules that > + * have a specific alias. > + * > + * If module auto-loading support is disabled by clearing the modprobe path > + * then this function becomes a no-operation. > */ > -int __request_module(bool wait, const char *fmt, ...) > +int __request_module(bool wait, int required_cap, > + const char *prefix, const char *fmt, ...) > { > va_list args; > char module_name[MODULE_NAME_LEN]; > int ret; > + int len = 0; > > /* > * We don't allow synchronous module loading from async. Module > @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) > if (!modprobe_path[0]) > return 0; > > + /* > + * Lets attach the prefix to the module name Let's but better: * Attach the prefix to the module name > + */ > + if (prefix != NULL && *prefix != '\0') { > + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); > + if (len >= MODULE_NAME_LEN) > + return -ENAMETOOLONG; > + } > + > va_start(args, fmt); > - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); > + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); > va_end(args); > - if (ret >= MODULE_NAME_LEN) > + if (ret >= MODULE_NAME_LEN - len) > return -ENAMETOOLONG; > > - ret = security_kernel_module_request(module_name); > + ret = security_kernel_module_request(module_name, required_cap, prefix); > if (ret) > return ret; >
Hi Randy, On Mon, Nov 27, 2017 at 7:48 PM, Randy Dunlap <rdunlap@infradead.org> wrote: > Hi, > > Mostly typos/spellos... > > > On 11/27/2017 09:18 AM, Djalal Harouni wrote: >> Cc: Serge Hallyn <serge@hallyn.com> >> Cc: Andy Lutomirski <luto@kernel.org> >> Suggested-by: Rusty Russell <rusty@rustcorp.com.au> >> Suggested-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Djalal Harouni <tixxdz@gmail.com> >> --- >> include/linux/kmod.h | 65 ++++++++++++++++++++++++++++++++++++++++++----- >> include/linux/lsm_hooks.h | 6 ++++- >> include/linux/security.h | 7 +++-- >> kernel/kmod.c | 29 ++++++++++++++++----- >> security/security.c | 6 +++-- >> security/selinux/hooks.c | 3 ++- >> 6 files changed, 97 insertions(+), 19 deletions(-) >> >> diff --git a/include/linux/kmod.h b/include/linux/kmod.h >> index 40c89ad..ccd6a1c 100644 >> --- a/include/linux/kmod.h >> +++ b/include/linux/kmod.h >> @@ -33,16 +33,67 @@ > >> +/** >> + * request_module Try to load a kernel module >> + * >> + * Automatically loads the request module. >> + * >> + * @mod...: The module name >> + */ > > what are the "..." for? what do they do here? Ok, will fix it. > >> +#define request_module(mod...) __request_module(true, -1, NULL, mod) >> + >> +#define request_module_nowait(mod...) __request_module(false, -1, NULL, mod) >> + >> +/** >> + * request_module_cap Load kernel module only if the required capability is set >> + * [...] > > > -- > ~Randy Thank you very much for the review, will fix all.
On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote: ... > After a discussion with Rusty Russell [1], the suggestion was to pass > the capability from request_module() to security_kernel_module_request() > for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from > Kees Cook [2] and experimenting with the code, the patch now does the > following: > > * Adds the request_module_cap() function. > * Updates the __request_module() to take the "required_cap" argument > with the "prefix". ... > Signed-off-by: Djalal Harouni <tixxdz@gmail.com> > --- > diff --git a/kernel/kmod.c b/kernel/kmod.c > index bc6addd..679d401 100644 > --- a/kernel/kmod.c > +++ b/kernel/kmod.c > @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) > if (!modprobe_path[0]) > return 0; > > + /* > + * Lets attach the prefix to the module name > + */ > + if (prefix != NULL && *prefix != '\0') { > + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); > + if (len >= MODULE_NAME_LEN) > + return -ENAMETOOLONG; > + } > + > va_start(args, fmt); > - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); > + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); > va_end(args); > - if (ret >= MODULE_NAME_LEN) > + if (ret >= MODULE_NAME_LEN - len) > return -ENAMETOOLONG; > > - ret = security_kernel_module_request(module_name); > + ret = security_kernel_module_request(module_name, required_cap, prefix); > if (ret) > return ret; > kmod is just a helper to poke userpsace to load a module, that's it. The old init_module() and newer finit_module() do the real handy work or module loading, and both currently only use may_init_module(): static int may_init_module(void) { if (!capable(CAP_SYS_MODULE) || modules_disabled) return -EPERM; return 0; } This begs the question: o If userspace just tries to just use raw finit_module() do we want similar checks? Otherwise, correct me if I'm wrong this all seems pointless. If we want something similar I think we might need to be processing aliases and check for the aliases for their desired restrictions on finit_module(), otherwise userspace can skip through the checks if the module name does not match the alias prefix. To be clear, aliases are completely ignored today on load_module(), so loading 'xfs' with finit_module() will just have the kernel know about 'xfs' not 'fs-xfs'. So we currently do not process aliases in kernel. I have debugging patches to enable us to process them, but they are just for debugging and I've been meaning to send them in for review. I designed them only for debugging given last time someone suggested for aliases processing to be added, the only use case we found was a pre-optimizations we decided to avoid pursuing. Debugging is a good reason to have alias processing in-kernel though. The pre-optimization we decided to stay away from was to check if the requested module via request_module() was already loaded *and* also check if the name passed matches any of the existing module aliases for currently loaded modules. Today request_module() does not even check if a requested module is already loaded, its a stupid loader, it just goes to userspace, and lets userspace figure it out. Userspace in turn could check for aliases, but it could lie, or not be up to date to do that. The pre-optmization is a theoretical gain only then, and if userspace had proper alias checking it is arguable that it may perform just as equal. To help valuate these sorts of things we now have: tools/testing/selftests/kmod/kmod.sh So further patches can use and test impact with it. Anyway -- so aliasing is currently only a debugging consideration, but without processing aliases, all this work seems pointless to me as the real loader is in finit_module(). kmod is just a stupid loader and uses the kernel usermode helper to do work. Luis
On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > kmod is just a helper to poke userpsace to load a module, that's it. > > The old init_module() and newer finit_module() do the real handy work or > module loading, and both currently only use may_init_module(): > > static int may_init_module(void) > { > if (!capable(CAP_SYS_MODULE) || modules_disabled) > return -EPERM; > > return 0; > } > > This begs the question: > > o If userspace just tries to just use raw finit_module() do we want similar > checks? > > Otherwise, correct me if I'm wrong this all seems pointless. Hm? That's direct-loading, not auto-loading. This series is only about auto-loading. We already have a global sysctl for blocking direct-loading (modules_disabled). > If we want something similar I think we might need to be processing aliases and > check for the aliases for their desired restrictions on finit_module(), We don't need to handle aliases. -Kees
Hi Luis, On Tue, Nov 28, 2017 at 8:14 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote: > ... > >> After a discussion with Rusty Russell [1], the suggestion was to pass >> the capability from request_module() to security_kernel_module_request() >> for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from >> Kees Cook [2] and experimenting with the code, the patch now does the >> following: >> >> * Adds the request_module_cap() function. >> * Updates the __request_module() to take the "required_cap" argument >> with the "prefix". > > ... > >> Signed-off-by: Djalal Harouni <tixxdz@gmail.com> >> --- >> diff --git a/kernel/kmod.c b/kernel/kmod.c >> index bc6addd..679d401 100644 >> --- a/kernel/kmod.c >> +++ b/kernel/kmod.c >> @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) >> if (!modprobe_path[0]) >> return 0; >> >> + /* >> + * Lets attach the prefix to the module name >> + */ >> + if (prefix != NULL && *prefix != '\0') { >> + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); >> + if (len >= MODULE_NAME_LEN) >> + return -ENAMETOOLONG; >> + } >> + >> va_start(args, fmt); >> - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); >> + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); >> va_end(args); >> - if (ret >= MODULE_NAME_LEN) >> + if (ret >= MODULE_NAME_LEN - len) >> return -ENAMETOOLONG; >> >> - ret = security_kernel_module_request(module_name); >> + ret = security_kernel_module_request(module_name, required_cap, prefix); >> if (ret) >> return ret; >> > > kmod is just a helper to poke userpsace to load a module, that's it. > > The old init_module() and newer finit_module() do the real handy work or > module loading, and both currently only use may_init_module(): > > static int may_init_module(void) > { > if (!capable(CAP_SYS_MODULE) || modules_disabled) > return -EPERM; > > return 0; > } > > This begs the question: > > o If userspace just tries to just use raw finit_module() do we want similar > checks? > > Otherwise, correct me if I'm wrong this all seems pointless. > > If we want something similar I think we might need to be processing aliases and > check for the aliases for their desired restrictions on finit_module(), > otherwise userspace can skip through the checks if the module name does not > match the alias prefix. > > To be clear, aliases are completely ignored today on load_module(), so loading > 'xfs' with finit_module() will just have the kernel know about 'xfs' not > 'fs-xfs'. > > So we currently do not process aliases in kernel. > > I have debugging patches to enable us to process them, but they are just for > debugging and I've been meaning to send them in for review. I designed them > only for debugging given last time someone suggested for aliases processing to > be added, the only use case we found was a pre-optimizations we decided to avoid > pursuing. Debugging is a good reason to have alias processing in-kernel though. > > The pre-optimization we decided to stay away from was to check if the requested > module via request_module() was already loaded *and* also check if the name passed > matches any of the existing module aliases for currently loaded modules. Today > request_module() does not even check if a requested module is already loaded, > its a stupid loader, it just goes to userspace, and lets userspace figure it > out. Userspace in turn could check for aliases, but it could lie, or not be up > to date to do that. > > The pre-optmization is a theoretical gain only then, and if userspace had > proper alias checking it is arguable that it may perform just as equal. > To help valuate these sorts of things we now have: > > tools/testing/selftests/kmod/kmod.sh > > So further patches can use and test impact with it. > > Anyway -- so aliasing is currently only a debugging consideration, but without > processing aliases, all this work seems pointless to me as the real loader is > in finit_module(). These patchset are about module auto-loading which is triggered from multiple paths in the kernel, the cover letter notes all the differences between the two operations and why the explicit one and "modules_disabled=1" is already a pain. The finit_module() is covered directly by CAP_SYS_MODULE, and for aliasing I am not sure how it will be related or how userspace will maintain it, we do not have a use case for it, we want a simple flag. Thank you!
On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote: > On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > kmod is just a helper to poke userpsace to load a module, that's it. > > > > The old init_module() and newer finit_module() do the real handy work or > > module loading, and both currently only use may_init_module(): > > > > static int may_init_module(void) > > { > > if (!capable(CAP_SYS_MODULE) || modules_disabled) > > return -EPERM; > > > > return 0; > > } > > > > This begs the question: > > > > o If userspace just tries to just use raw finit_module() do we want similar > > checks? > > > > Otherwise, correct me if I'm wrong this all seems pointless. > > Hm? That's direct-loading, not auto-loading. This series is only about > auto-loading. And *all* auto-loading uses aliases? What's the difference between auto-loading and direct-loading? > We already have a global sysctl for blocking direct-loading (modules_disabled). My point was that even if you have a CAP_NET_ADMIN check on request_module(), finit_module() will not check for it, so a crafty userspace could still try to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN check. So unless I'm missing something, I see no point in adding extra checks for request_module() but nothing for the respective load_module(). Luis
On Tue, Nov 28, 2017 at 10:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote: >> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > kmod is just a helper to poke userpsace to load a module, that's it. >> > >> > The old init_module() and newer finit_module() do the real handy work or >> > module loading, and both currently only use may_init_module(): >> > >> > static int may_init_module(void) >> > { >> > if (!capable(CAP_SYS_MODULE) || modules_disabled) >> > return -EPERM; >> > >> > return 0; >> > } >> > >> > This begs the question: >> > >> > o If userspace just tries to just use raw finit_module() do we want similar >> > checks? >> > >> > Otherwise, correct me if I'm wrong this all seems pointless. >> >> Hm? That's direct-loading, not auto-loading. This series is only about >> auto-loading. > > And *all* auto-loading uses aliases? What's the difference between auto-loading > and direct-loading? Not all auto-loading uses aliases, auto-loading is when kernel code calls request_module() to loads the feature that was not present, and direct-loading in this thread is the direct syscalls like finit_module(). >> We already have a global sysctl for blocking direct-loading (modules_disabled). > > My point was that even if you have a CAP_NET_ADMIN check on request_module(), > finit_module() will not check for it, so a crafty userspace could still try > to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN > check. The finit_module() uses CAP_SYS_MODULE which should allow all modules and in this context it should be more privileged than CAP_NET_ADMIN which is only for "netdev-%s" (to not load arbitrary modules with it). finit_module() coming from request_module() always has the CAP_NET_ADMIN, hence the check is done before. > So unless I'm missing something, I see no point in adding extra checks for > request_module() but nothing for the respective load_module(). I see, request_module() is called from kernel context which runs in init namespace will full capabilities, the spawned userspace modprobe will get CAP_SYS_MODULE and all other caps, then after comes modprobe and load_module(). Btw as suggested by Linus I will update with request_module_cap() and I can offer my help maintaining these bits too. > > Luis
On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > And *all* auto-loading uses aliases? What's the difference between auto-loading > and direct-loading? The difference is the process privileges. Unprivilged autoloading (e.g. int n_hdlc = N_HDLC; ioctl(fd, TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() under CAP_SYS_MODULE. >> We already have a global sysctl for blocking direct-loading (modules_disabled). > > My point was that even if you have a CAP_NET_ADMIN check on request_module(), > finit_module() will not check for it, so a crafty userspace could still try > to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN > check. You need CAP_SYS_MODULE to run finit_module(). -Kees
On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote: > On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > And *all* auto-loading uses aliases? What's the difference between auto-loading > > and direct-loading? > > The difference is the process privileges. Unprivilged autoloading > (e.g. int n_hdlc = N_HDLC; ioctl(fd, > TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() > under CAP_SYS_MODULE. Ah, so system call implicated request_module() calls. > >> We already have a global sysctl for blocking direct-loading (modules_disabled). > > > > My point was that even if you have a CAP_NET_ADMIN check on request_module(), > > finit_module() will not check for it, so a crafty userspace could still try > > to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN > > check. > > You need CAP_SYS_MODULE to run finit_module(). OK and since CAP_SYS_MODULE is much more restrictive one could argue, what's the point here? Luis
On Tue, Nov 28, 2017 at 2:12 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote: >> On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > And *all* auto-loading uses aliases? What's the difference between auto-loading >> > and direct-loading? >> >> The difference is the process privileges. Unprivilged autoloading >> (e.g. int n_hdlc = N_HDLC; ioctl(fd, >> TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() >> under CAP_SYS_MODULE. > > Ah, so system call implicated request_module() calls. Yup. Unprivileged user does something that ultimately hits a request_module() in the kernel. Then the kernel calls out with the usermode helper (which has CAP_SYS_MODULE) and calls finit_module(). > OK and since CAP_SYS_MODULE is much more restrictive one could argue, what's the > point here? The goal is to block an unprivileged user from being able to trigger a module load without blocking root from loading modules directly. -Kees
On Tue, Nov 28, 2017 at 10:33:27PM +0100, Djalal Harouni wrote: > On Tue, Nov 28, 2017 at 10:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote: > >> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > >> > kmod is just a helper to poke userpsace to load a module, that's it. > >> > > >> > The old init_module() and newer finit_module() do the real handy work or > >> > module loading, and both currently only use may_init_module(): > >> > > >> > static int may_init_module(void) > >> > { > >> > if (!capable(CAP_SYS_MODULE) || modules_disabled) > >> > return -EPERM; > >> > > >> > return 0; > >> > } > >> > > >> > This begs the question: > >> > > >> > o If userspace just tries to just use raw finit_module() do we want similar > >> > checks? > >> > > >> > Otherwise, correct me if I'm wrong this all seems pointless. > >> > >> Hm? That's direct-loading, not auto-loading. This series is only about > >> auto-loading. > > > > And *all* auto-loading uses aliases? What's the difference between auto-loading > > and direct-loading? > > Not all auto-loading uses aliases, auto-loading is when kernel code > calls request_module() to loads the feature that was not present, It seems the actual interest here is system call implicated request_module() calls? Because there are uses of request_module() which may be module hacks, and not implicated via system calls. > and direct-loading in this thread is the direct syscalls like > finit_module(). OK. > >> We already have a global sysctl for blocking direct-loading (modules_disabled). > > > > My point was that even if you have a CAP_NET_ADMIN check on request_module(), > > finit_module() will not check for it, so a crafty userspace could still try > > to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN > > check. > > The finit_module() uses CAP_SYS_MODULE which should allow all modules > and in this context it should be more privileged than CAP_NET_ADMIN > which is only for "netdev-%s" (to not load arbitrary modules with it). > > finit_module() coming from request_module() always has the > CAP_NET_ADMIN, hence the check is done before. But since CAP_SYS_MODULE is more restrictive, what's the point in checking for CAP_NET_ADMIN? > > So unless I'm missing something, I see no point in adding extra checks for > > request_module() but nothing for the respective load_module(). > > I see, request_module() is called from kernel context which runs in > init namespace will full capabilities, the spawned userspace modprobe > will get CAP_SYS_MODULE and all other caps, then after comes modprobe > and load_module(). Right, so defining the gains of adding this extra check is not very clear yet. It would seem a benefit exists, what is it? > Btw as suggested by Linus I will update with request_module_cap() and > I can > offer my help maintaining these bits too. Can you start by extending lib/test_module.c and tools/testing/selftests/kmod/kmod.sh with a proof of concept of the gains here, as well as ensuring things work as expected ? Luis
On Tue, Nov 28, 2017 at 02:18:18PM -0800, Kees Cook wrote: > On Tue, Nov 28, 2017 at 2:12 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote: > >> On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > >> > And *all* auto-loading uses aliases? What's the difference between auto-loading > >> > and direct-loading? > >> > >> The difference is the process privileges. Unprivilged autoloading > >> (e.g. int n_hdlc = N_HDLC; ioctl(fd, > >> TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() > >> under CAP_SYS_MODULE. > > > > Ah, so system call implicated request_module() calls. > > Yup. Unprivileged user does something that ultimately hits a > request_module() in the kernel. Then the kernel calls out with the > usermode helper (which has CAP_SYS_MODULE) and calls finit_module(). Thanks, using this terminology is much better to understand than auto-loading, given it does make it clear an unprivileged call was one that initiated the request_module() call, there are many uses of request_module() which *are* privileged. > > OK and since CAP_SYS_MODULE is much more restrictive one could argue, what's the > > point here? > > The goal is to block an unprivileged user from being able to trigger a > module load without blocking root from loading modules directly. I see now. Do we have an audit of all system calls which implicate a request_module() call? Networking is a good example for sure to start off with but I was curious if we have a grasp of how wide spread this could be. I'll go review the patches again now with all this in mind. Luis
On Tue, Nov 28, 2017 at 11:18 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, Nov 28, 2017 at 10:33:27PM +0100, Djalal Harouni wrote: >> On Tue, Nov 28, 2017 at 10:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote: >> >> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> >> > kmod is just a helper to poke userpsace to load a module, that's it. >> >> > >> >> > The old init_module() and newer finit_module() do the real handy work or >> >> > module loading, and both currently only use may_init_module(): >> >> > >> >> > static int may_init_module(void) >> >> > { >> >> > if (!capable(CAP_SYS_MODULE) || modules_disabled) >> >> > return -EPERM; >> >> > >> >> > return 0; >> >> > } >> >> > >> >> > This begs the question: >> >> > >> >> > o If userspace just tries to just use raw finit_module() do we want similar >> >> > checks? >> >> > >> >> > Otherwise, correct me if I'm wrong this all seems pointless. >> >> >> >> Hm? That's direct-loading, not auto-loading. This series is only about >> >> auto-loading. >> > >> > And *all* auto-loading uses aliases? What's the difference between auto-loading >> > and direct-loading? >> >> Not all auto-loading uses aliases, auto-loading is when kernel code >> calls request_module() to loads the feature that was not present, > > It seems the actual interest here is system call implicated request_module() > calls? Because there are uses of request_module() which may be module hacks, > and not implicated via system calls. Indeed. >> and direct-loading in this thread is the direct syscalls like >> finit_module(). > > OK. > >> >> We already have a global sysctl for blocking direct-loading (modules_disabled). >> > >> > My point was that even if you have a CAP_NET_ADMIN check on request_module(), >> > finit_module() will not check for it, so a crafty userspace could still try >> > to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN >> > check. >> >> The finit_module() uses CAP_SYS_MODULE which should allow all modules >> and in this context it should be more privileged than CAP_NET_ADMIN >> which is only for "netdev-%s" (to not load arbitrary modules with it). >> >> finit_module() coming from request_module() always has the >> CAP_NET_ADMIN, hence the check is done before. > > But since CAP_SYS_MODULE is more restrictive, what's the point in checking > for CAP_NET_ADMIN? For backward compatibility with 'netdev' modules since it is for those. >> > So unless I'm missing something, I see no point in adding extra checks for >> > request_module() but nothing for the respective load_module(). >> >> I see, request_module() is called from kernel context which runs in >> init namespace will full capabilities, the spawned userspace modprobe >> will get CAP_SYS_MODULE and all other caps, then after comes modprobe >> and load_module(). > > Right, so defining the gains of adding this extra check is not very clear > yet. It would seem a benefit exists, what is it? it will able to filter if the request_module() should continue loading the module or deny it which prevents spawning the *privileged* usermode helper. This is all based on are we allowed to load new features or not, or IOW I don't want to allow new features or modules autoloading from now and on, as stated in the cover letter for various benefit including security, reduce the amount of kernel code running, but also do not allow new features for anyone like tunneling, etc. >> Btw as suggested by Linus I will update with request_module_cap() and > I can >> offer my help maintaining these bits too. > > Can you start by extending lib/test_module.c and > tools/testing/selftests/kmod/kmod.sh with a proof of concept of the gains here, > as well as ensuring things work as expected ? Alright Luis, thanks for the hint, yes I will make sure to cover these. For gains, kees already answered in the other email, and please check the DCCP exploit and others linked in the cover letter. Thank you! > Luis
On Tue, Nov 28, 2017 at 11:48:49PM +0100, Luis R. Rodriguez wrote: > On Tue, Nov 28, 2017 at 02:18:18PM -0800, Kees Cook wrote: > > On Tue, Nov 28, 2017 at 2:12 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote: > > >> On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > >> > And *all* auto-loading uses aliases? What's the difference > > >> > between auto-loading and direct-loading? > > >> > > >> The difference is the process privileges. Unprivilged autoloading > > >> (e.g. int n_hdlc = N_HDLC; ioctl(fd, > > >> TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() > > >> under CAP_SYS_MODULE. > > > > > > Ah, so system call implicated request_module() calls. > > > > Yup. Unprivileged user does something that ultimately hits a > > request_module() in the kernel. Then the kernel calls out with the > > usermode helper (which has CAP_SYS_MODULE) and calls finit_module(). > > Thanks, using this terminology is much better to understand than > auto-loading, given it does make it clear an unprivileged call was one > that initiated the request_module() call, there are many uses of > request_module() which *are* privileged. > > > > OK and since CAP_SYS_MODULE is much more restrictive one could > > > argue, what's the point here? > > > > The goal is to block an unprivileged user from being able to trigger a > > module load without blocking root from loading modules directly. > > I see now. Do we have an audit of all system calls which implicate a > request_module() call? Networking is a good example for sure to start > off with but I was curious if we have a grasp of how wide spread this > could be. I'm not sure it makes sense to classify this by syscalls. In networking, request_module() can be triggered e.g. by a netlink message (genetlink family lookup is an example not needing any privileges) so that one of the syscalls would be sendmsg(). Michal Kubecek
On Tue, 28 Nov 2017 13:39:58 -0800 Kees Cook <keescook@chromium.org> wrote: > On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > And *all* auto-loading uses aliases? What's the difference between auto-loading > > and direct-loading? > > The difference is the process privileges. Unprivilged autoloading > (e.g. int n_hdlc = N_HDLC; ioctl(fd, > TIOCSETD, &n_hdlc)), triggers a privileged call to finit_module() > under CAP_SYS_MODULE. If you have CAP_SYS_DAC you can rename any module to ppp.ko and ask the network manager (which has the right permissions) to init a ppp connection. Capabilities alone are simply not enough to do any kind of useful protection on a current system and the Linux capability model is broken architecturally and not fixable because fixing it would break lots of real systems. I really don't care what the module loading rules end up with and whether we add CAP_SYS_YET_ANOTHER_MEANINGLESS_FLAG but what is actually needed is to properly incorporate it into securiy ruiles for whatever LSM you are using. Alan
From: Alan Cox <gnomes@lxorguk.ukuu.org.uk> Date: Wed, 29 Nov 2017 13:46:12 +0000 > I really don't care what the module loading rules end up with and > whether we add CAP_SYS_YET_ANOTHER_MEANINGLESS_FLAG but what is > actually needed is to properly incorporate it into securiy ruiles > for whatever LSM you are using. I'm surprised we're not using the SHA1 hashes or whatever we compute for the modules to make sure we are loading the foo.ko that we expect to be. Then even if someone can rename every file on the system they cannot force a rogue module to load unless they build one with a proper hash collision. Ie. transform module load strings at build time: ppp.ko --> ppp.ko:SHA1 Or something like that. And the kernel refuses to load a ppp.ko with a mismatching SHA1. All of this capability stuff seems to dance a circle around the problem rather than fix it.
On Wed, Nov 29, 2017 at 09:50:14AM -0500, David Miller wrote: > From: Alan Cox <gnomes@lxorguk.ukuu.org.uk> > Date: Wed, 29 Nov 2017 13:46:12 +0000 > > > I really don't care what the module loading rules end up with and > > whether we add CAP_SYS_YET_ANOTHER_MEANINGLESS_FLAG but what is > > actually needed is to properly incorporate it into securiy ruiles > > for whatever LSM you are using. > > I'm surprised we're not using the SHA1 hashes or whatever we compute > for the modules to make sure we are loading the foo.ko that we expect > to be. We do have signed modules. But this won't help us if the user is using a distro kernel which has compiled some module which is known to be unmaintained which everyone in the know *expects* to have 0-day bugs, such as DCCP. That's because the DCCP module is signed. We could fix this by adding to the signature used for module signing to include the module name, so that the bad guy can't rename dccp.ko to be ppp.ko, I suppose.... > All of this capability stuff seems to dance a circle around the > problem rather than fix it. Half the problem here is that with containers, people are changing the security model, because they want to let untrusted users have "root", without really having "root". Part of the fundamental problem is that there are some well-meaning, but fundamentally misguided people, who have been asserting: "Containers are just as secure as VM's". Well, they are not. And the sooner people get past this, the better off they'll be.... - Ted
From: Theodore Ts'o <tytso@mit.edu> Date: Wed, 29 Nov 2017 10:54:06 -0500 > On Wed, Nov 29, 2017 at 09:50:14AM -0500, David Miller wrote: >> From: Alan Cox <gnomes@lxorguk.ukuu.org.uk> >> Date: Wed, 29 Nov 2017 13:46:12 +0000 >> >> > I really don't care what the module loading rules end up with and >> > whether we add CAP_SYS_YET_ANOTHER_MEANINGLESS_FLAG but what is >> > actually needed is to properly incorporate it into securiy ruiles >> > for whatever LSM you are using. >> >> I'm surprised we're not using the SHA1 hashes or whatever we compute >> for the modules to make sure we are loading the foo.ko that we expect >> to be. > > We do have signed modules. But this won't help us if the user is > using a distro kernel which has compiled some module which is known to > be unmaintained which everyone in the know *expects* to have 0-day > bugs, such as DCCP. That's because the DCCP module is signed. That's not what we're talking about. We're talking about making sure that loading "ppp.ko" really gets ppp.ko rather than some_other_module.ko renamed to ppp.ko via some other mechanism. Both modules have legitimate signatures so the kernel will happily load both.
On Wed, Nov 29, 2017 at 10:58:16AM -0500, David Miller wrote: > That's not what we're talking about. > > We're talking about making sure that loading "ppp.ko" really gets > ppp.ko rather than some_other_module.ko renamed to ppp.ko via some > other mechanism. Right, and the best solution to this problem is to include in the signature the name of the module. (One way of doing this is adding the module name into the cryptographic checksum which is then digitally signed by the kernel module key; we would have to bump the version number of the signature format, obviously.) This binds the name of the module into the digital signature so that it can't be renamed. - Ted
Quoting Theodore Ts'o (tytso@mit.edu): > Half the problem here is that with containers, people are changing the > security model, because they want to let untrusted users have "root", > without really having "root". Part of the fundamental problem is that > there are some well-meaning, but fundamentally misguided people, who > have been asserting: "Containers are just as secure as VM's". > > Well, they are not. And the sooner people get past this, the better > off they'll be.... Just to be clear, module loading requires - and must always continue to require - CAP_SYS_MODULE against the initial user namespace. Containers in user namespaces do not have that. I don't believe anyone has ever claimed that containers which are not in a user namespace are in any way secure. (And as for the other claim, I'd prefer to stick to "VMs are in most cases as insecure as properly configured containers" :) -serge
On Wed, Nov 29, 2017 at 7:58 AM, David Miller <davem@davemloft.net> wrote: > > We're talking about making sure that loading "ppp.ko" really gets > ppp.ko rather than some_other_module.ko renamed to ppp.ko via some > other mechanism. > > Both modules have legitimate signatures so the kernel will happily > load both. Yes. We could make the module name be part of the signing process, but one problem with that is that at module loading time we don't actually have the filename any more. User space opens the file and then just feeds the data to the kernel. So if you fooled modprobe into feeding the wrong module, that's it. And yes, we can obviously embed the module name into the ELF headers (that is all part of the signed payload), but the module name doesn't actually necessarily match what we originally asked for. Why? Module aliases and module dependencies - which is why we have that user mode side at all. When we do "request_module(XYZ)" we don't necessarily know what the dependencies are, so we expect modprobe to just load the right modules. So if modprobe then loads some other module (dccp or whatever), the kernel has no real way to know "oh, that wasn't part of the dependency chain for the module we aked for". Now, if modprobe is taught to check that the filename of the module that it opens actually matches the metadata in the ELF sections, that would solve it, but it's out of the kernels hands.. Linus
On Wed, Nov 29, 2017 at 2:45 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Nov 29, 2017 at 7:58 AM, David Miller <davem@davemloft.net> wrote: >> >> We're talking about making sure that loading "ppp.ko" really gets >> ppp.ko rather than some_other_module.ko renamed to ppp.ko via some >> other mechanism. >> >> Both modules have legitimate signatures so the kernel will happily >> load both. > > Yes. We could make the module name be part of the signing process, but > one problem with that is that at module loading time we don't actually > have the filename any more. FWIW, I added this (well, KBUILD_MODNAME) to the module info just recently: 3e2e857f9c3a ("module: Add module name to modinfo") > User space opens the file and then just feeds the data to the kernel. > So if you fooled modprobe into feeding the wrong module, that's it. > > And yes, we can obviously embed the module name into the ELF headers > (that is all part of the signed payload), but the module name doesn't > actually necessarily match what we originally asked for. > > Why? Module aliases and module dependencies - which is why we have > that user mode side at all. When we do "request_module(XYZ)" we don't > necessarily know what the dependencies are, so we expect modprobe to > just load the right modules. > > So if modprobe then loads some other module (dccp or whatever), the > kernel has no real way to know "oh, that wasn't part of the dependency > chain for the module we aked for". > > Now, if modprobe is taught to check that the filename of the module > that it opens actually matches the metadata in the ELF sections, that > would solve it, but it's out of the kernels hands.. Right, the aliases are why these kinds of renaming shenanigans don't mean anything: it's not distinguishable from whatever modprobe.conf ultimately tells modprobe to do. If you can't trust your filesystem to hold your kernel modules correctly, you have much bigger problems. (And yes, capabilities are a problem here, since there are many paths to full root from individual capabilities, but that's a known issue that is much larger than tricking modprobe.) -Kees
On Wed, Nov 29, 2017 at 11:28:52AM -0600, Serge E. Hallyn wrote: > > Just to be clear, module loading requires - and must always continue to > require - CAP_SYS_MODULE against the initial user namespace. Containers > in user namespaces do not have that. > > I don't believe anyone has ever claimed that containers which are not in > a user namespace are in any way secure. Unless the container performs some action which causes the kernel to call request_module(), which then loads some kernel module, potentially containing cr*p unmaintained code which was included when the distro compiled the world, into the host kernel. This is an attack vector that doesn't exist if you are using VM's. And in general, the attack surface of the entire Linux kernel<->userspace API is far larger than that which is exposed by the guest<->host interface. For that reason, containers are *far* more insecure than VM's, since once the attacker gets root on the guest VM, they then have to attack the hypervisor interface. And if you compare the attack surface of the two, it's pretty clear which is larger, and it's not the hypervisor interface. - Ted
On Wed, Nov 29, 2017 at 07:35:31PM -0500, Theodore Ts'o wrote: > On Wed, Nov 29, 2017 at 11:28:52AM -0600, Serge E. Hallyn wrote: > > > > Just to be clear, module loading requires - and must always continue to > > require - CAP_SYS_MODULE against the initial user namespace. Containers > > in user namespaces do not have that. > > > > I don't believe anyone has ever claimed that containers which are not in > > a user namespace are in any way secure. > > Unless the container performs some action which causes the kernel to > call request_module(), which then loads some kernel module, A local unprivileged user can do the same thing. I reject the popular notion that linux is a single user operating system. More interesting are the (very real) cases where root in a container can do something which a local unprivileged user could not do. Since a local unprivileged user can always create a new namespace, *those* constitute a real and interesting problem. > potentially containing cr*p unmaintained code which was included when > the distro compiled the world, into the host kernel. > This is an attack vector that doesn't exist if you are using VM's. Until the vm tenant uses a trivial vm escape. > And in general, the attack surface of the entire Linux > kernel<->userspace API is far larger than that which is exposed by the > guest<->host interface. > > For that reason, containers are *far* more insecure than VM's, since > once the attacker gets root on the guest VM, they then have to attack > the hypervisor interface. And if you compare the attack surface of > the two, it's pretty clear which is larger, and it's not the > hypervisor interface. Any time anyone spends a day looking at either the black hole that is the hardware emulators or the xen and kvm code itself they walk away with a set of cve's. It *should* be more secure, it's not. You're telling me your house is safe because you put up a no tresspassing sign. -serge
diff --git a/include/linux/kmod.h b/include/linux/kmod.h index 40c89ad..ccd6a1c 100644 --- a/include/linux/kmod.h +++ b/include/linux/kmod.h @@ -33,16 +33,67 @@ extern char modprobe_path[]; /* for sysctl */ /* modprobe exit status on success, -ve on error. Return value * usually useless though. */ -extern __printf(2, 3) -int __request_module(bool wait, const char *name, ...); -#define request_module(mod...) __request_module(true, mod) -#define request_module_nowait(mod...) __request_module(false, mod) +extern __printf(4, 5) +int __request_module(bool wait, int required_cap, + const char *prefix, const char *name, ...); #define try_then_request_module(x, mod...) \ - ((x) ?: (__request_module(true, mod), (x))) + ((x) ?: (__request_module(true, -1, NULL, mod), (x))) #else -static inline int request_module(const char *name, ...) { return -ENOSYS; } -static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; } +static inline __printf(4, 5) +int __request_module(bool wait, int required_cap, + const char *prefix, const char *name, ...) +{ return -ENOSYS; } #define try_then_request_module(x, mod...) (x) #endif +/** + * request_module Try to load a kernel module + * + * Automatically loads the request module. + * + * @mod...: The module name + */ +#define request_module(mod...) __request_module(true, -1, NULL, mod) + +#define request_module_nowait(mod...) __request_module(false, -1, NULL, mod) + +/** + * request_module_cap Load kernel module only if the required capability is set + * + * Automatically load a module if the required capability is set and it + * corresponds to the appropriate subsystem that is indicated by prefix. + * + * This allows to load aliased modules like 'netdev-%s' with CAP_NET_ADMIN. + * + * ex: + * request_module_cap(CAP_NET_ADMIN, "netdev", "%s", mod); + * + * @required_cap: Required capability to load the module + * @prefix: The module prefix if any, otherwise NULL + * @fmt: printf style format string for the name of the module with its + * arguments if any + * + * If '@required_cap' is positive, the security subsystem will check if + * '@prefix' is set and if caller has the required capability then the + * operation is allowed. + * The security subsystem can not make assumption about the boundaries + * of other subsystems, it is their responsability to make a call with + * the right capability and module alias. + * + * If '@required_cap' is positive and '@prefix' is NULL then we assume + * that the '@required_cap' is CAP_SYS_MODULE. + * + * If '@required_cap' is negative then there are no permission checks, this + * is the equivalent to request_module() function. + * + * This function trust callers to pass the right capability with the + * appropriate prefix. + * + * Note: the permission checks may still fail, even if the required + * capability is negative, this is due to module loading restrictions + * that are controlled by the enduser. + */ +#define request_module_cap(required_cap, prefix, fmt...) \ + __request_module(true, required_cap, prefix, fmt) + #endif /* __LINUX_KMOD_H__ */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 7161d8e..d898bd0 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -571,6 +571,9 @@ * Ability to trigger the kernel to automatically upcall to userspace for * userspace to load a kernel module with the given name. * @kmod_name name of the module requested by the kernel + * @required_cap If positive, the required capability to automatically load + * the correspondig kernel module. + * @prefix The prefix of the module if any. Can be NULL. * Return 0 if successful. * @kernel_read_file: * Read a file specified by userspace. @@ -1543,7 +1546,8 @@ union security_list_options { void (*cred_transfer)(struct cred *new, const struct cred *old); int (*kernel_act_as)(struct cred *new, u32 secid); int (*kernel_create_files_as)(struct cred *new, struct inode *inode); - int (*kernel_module_request)(char *kmod_name); + int (*kernel_module_request)(char *kmod_name, int required_cap, + const char *prefix); int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id); int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size, enum kernel_read_file_id id); diff --git a/include/linux/security.h b/include/linux/security.h index 73f1ef6..41e700a 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -326,7 +326,8 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp); void security_transfer_creds(struct cred *new, const struct cred *old); int security_kernel_act_as(struct cred *new, u32 secid); int security_kernel_create_files_as(struct cred *new, struct inode *inode); -int security_kernel_module_request(char *kmod_name); +int security_kernel_module_request(char *kmod_name, int required_cap, + const char *prefix); int security_kernel_read_file(struct file *file, enum kernel_read_file_id id); int security_kernel_post_read_file(struct file *file, char *buf, loff_t size, enum kernel_read_file_id id); @@ -919,7 +920,9 @@ static inline int security_kernel_create_files_as(struct cred *cred, return 0; } -static inline int security_kernel_module_request(char *kmod_name) +static inline int security_kernel_module_request(char *kmod_name, + int required_cap, + const char *prefix) { return 0; } diff --git a/kernel/kmod.c b/kernel/kmod.c index bc6addd..679d401 100644 --- a/kernel/kmod.c +++ b/kernel/kmod.c @@ -109,6 +109,8 @@ static int call_modprobe(char *module_name, int wait) /** * __request_module - try to load a kernel module * @wait: wait (or not) for the operation to complete + * @required_cap: required capability to load the module + * @prefix: module prefix if any otherwise NULL * @fmt: printf style format string for the name of the module * @...: arguments as specified in the format string * @@ -119,14 +121,20 @@ static int call_modprobe(char *module_name, int wait) * must check that the service they requested is now available not blindly * invoke it. * - * If module auto-loading support is disabled then this function - * becomes a no-operation. + * If "required_cap" is positive, The security subsystem will trust the caller + * that if it has "required_cap" then it may allow to load some modules that + * have a specific alias. + * + * If module auto-loading support is disabled by clearing the modprobe path + * then this function becomes a no-operation. */ -int __request_module(bool wait, const char *fmt, ...) +int __request_module(bool wait, int required_cap, + const char *prefix, const char *fmt, ...) { va_list args; char module_name[MODULE_NAME_LEN]; int ret; + int len = 0; /* * We don't allow synchronous module loading from async. Module @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...) if (!modprobe_path[0]) return 0; + /* + * Lets attach the prefix to the module name + */ + if (prefix != NULL && *prefix != '\0') { + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix); + if (len >= MODULE_NAME_LEN) + return -ENAMETOOLONG; + } + va_start(args, fmt); - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args); + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args); va_end(args); - if (ret >= MODULE_NAME_LEN) + if (ret >= MODULE_NAME_LEN - len) return -ENAMETOOLONG; - ret = security_kernel_module_request(module_name); + ret = security_kernel_module_request(module_name, required_cap, prefix); if (ret) return ret; diff --git a/security/security.c b/security/security.c index 1cd8526..91ecebd 100644 --- a/security/security.c +++ b/security/security.c @@ -1015,9 +1015,11 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode) return call_int_hook(kernel_create_files_as, 0, new, inode); } -int security_kernel_module_request(char *kmod_name) +int security_kernel_module_request(char *kmod_name, int required_cap, + const char *prefix) { - return call_int_hook(kernel_module_request, 0, kmod_name); + return call_int_hook(kernel_module_request, 0, kmod_name, + required_cap, prefix); } int security_kernel_read_file(struct file *file, enum kernel_read_file_id id) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d07299d..69f25da 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3889,7 +3889,8 @@ static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode) return ret; } -static int selinux_kernel_module_request(char *kmod_name) +static int selinux_kernel_module_request(char *kmod_name, int required_cap, + const char *prefix) { struct common_audit_data ad;
This is a preparation patch to improve the module auto-load infrastructure. We need this patch to have more control on module auto-load operations. The operation by default is allowed unless enduser or the calling code requests that we need to perform futher permission checks. With this change subsystems will be able to decide if module auto-load feature first will have to do a capability check and load the module if the permission check succeeds or deny the operation. As an example "netdev-%s" modules, they are allowed to be loaded if CAP_NET_ADMIN is set. Therefore, in order to not break this assumption, and allow userspace to load "netdev-%s" modules with CAP_NET_ADMIN, we have added: request_module_cap(required_cap, prefix, fmt...) This new function will take: '@required_cap': Required capability to load the module '@prefix': The module prefix if any, otherwise NULL '@fmt': printf style format string for the name of the module with its arguments if any ex: request_module_cap(CAP_NET_ADMIN, "netdev", "%s", mod); After a discussion with Rusty Russell [1], the suggestion was to pass the capability from request_module() to security_kernel_module_request() for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from Kees Cook [2] and experimenting with the code, the patch now does the following: * Adds the request_module_cap() function. * Updates the __request_module() to take the "required_cap" argument with the "prefix". This patch also updates SELinux which is currently the only user of security_kernel_module_request(), the security hook now accepts 'required_cap' and 'prefix' as arguments. Based on patch by Rusty Russell and discussion with Kees Cook: [1] https://lkml.org/lkml/2017/4/26/735 [2] https://lkml.org/lkml/2017/5/23/775 Cc: Serge Hallyn <serge@hallyn.com> Cc: Andy Lutomirski <luto@kernel.org> Suggested-by: Rusty Russell <rusty@rustcorp.com.au> Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Djalal Harouni <tixxdz@gmail.com> --- include/linux/kmod.h | 65 ++++++++++++++++++++++++++++++++++++++++++----- include/linux/lsm_hooks.h | 6 ++++- include/linux/security.h | 7 +++-- kernel/kmod.c | 29 ++++++++++++++++----- security/security.c | 6 +++-- security/selinux/hooks.c | 3 ++- 6 files changed, 97 insertions(+), 19 deletions(-)