diff mbox

[v5,next,1/5] modules:capabilities: add request_module_cap()

Message ID 1511803118-2552-2-git-send-email-tixxdz@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Djalal Harouni Nov. 27, 2017, 5:18 p.m. UTC
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(-)

Comments

Randy Dunlap Nov. 27, 2017, 6:48 p.m. UTC | #1
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;
>
Djalal Harouni Nov. 27, 2017, 9:35 p.m. UTC | #2
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.
Luis Chamberlain Nov. 28, 2017, 7:14 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook Nov. 28, 2017, 8:11 p.m. UTC | #4
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
Djalal Harouni Nov. 28, 2017, 8:18 p.m. UTC | #5
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!
Luis Chamberlain Nov. 28, 2017, 9:16 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Djalal Harouni Nov. 28, 2017, 9:33 p.m. UTC | #7
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
Kees Cook Nov. 28, 2017, 9:39 p.m. UTC | #8
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
Luis Chamberlain Nov. 28, 2017, 10:12 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook Nov. 28, 2017, 10:18 p.m. UTC | #10
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
Luis Chamberlain Nov. 28, 2017, 10:18 p.m. UTC | #11
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain Nov. 28, 2017, 10:48 p.m. UTC | #12
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Djalal Harouni Nov. 28, 2017, 10:52 p.m. UTC | #13
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
Michal Kubecek Nov. 29, 2017, 7:49 a.m. UTC | #14
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox Nov. 29, 2017, 1:46 p.m. UTC | #15
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 29, 2017, 2:50 p.m. UTC | #16
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o Nov. 29, 2017, 3:54 p.m. UTC | #17
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 29, 2017, 3:58 p.m. UTC | #18
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o Nov. 29, 2017, 4:29 p.m. UTC | #19
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Serge E. Hallyn Nov. 29, 2017, 5:28 p.m. UTC | #20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds Nov. 29, 2017, 10:45 p.m. UTC | #21
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook Nov. 30, 2017, 12:06 a.m. UTC | #22
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
Theodore Ts'o Nov. 30, 2017, 12:35 a.m. UTC | #23
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Serge E. Hallyn Nov. 30, 2017, 5:17 p.m. UTC | #24
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;