diff mbox

[v4,next,1/3] modules:capabilities: allow __request_module() to take a capability argument

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

Commit Message

Djalal Harouni May 22, 2017, 11:57 a.m. UTC
This is a preparation patch for the module auto-load restriction feature.

In order to restrict module auto-load operations we need to check if the
caller has CAP_SYS_MODULE capability. This allows to align security
checks of automatic module loading with the checks of the explicit operations.

However for "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 only load "netdev-%s" modules with CAP_NET_ADMIN
capability which is considered a privileged operation, we have two
choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
the capability form request_module() to security_kernel_module_request()
hook and let the capability subsystem decide.

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.

The patch does not update request_module(), it updates the internal
__request_module() that will take an extra "allow_cap" argument. If
positive, then automatic module load operation can be allowed.

__request_module() will be only called by networking code which is the
exception to this, so we do not break userspace and CAP_NET_ADMIN can
continue to load 'netdev-%s' modules. Other kernel code should continue
to use request_module() which calls security_kernel_module_request() and
will check for CAP_SYS_MODULE capability in next patch. Allowing more
control on who can trigger automatic module loading.

This patch updates security_kernel_module_request() to take the
'allow_cap' argument and SELinux which is currently the only user of
security_kernel_module_request() hook.

Based on patch by Rusty Russell:
https://lkml.org/lkml/2017/4/26/735

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>

[1] https://lkml.org/lkml/2017/4/24/7
---
 include/linux/kmod.h      | 15 ++++++++-------
 include/linux/lsm_hooks.h |  4 +++-
 include/linux/security.h  |  4 ++--
 kernel/kmod.c             | 15 +++++++++++++--
 net/core/dev_ioctl.c      | 10 +++++++++-
 security/security.c       |  4 ++--
 security/selinux/hooks.c  |  2 +-
 7 files changed, 38 insertions(+), 16 deletions(-)

Comments

Kees Cook May 22, 2017, 10:20 p.m. UTC | #1
On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> This is a preparation patch for the module auto-load restriction feature.
>
> In order to restrict module auto-load operations we need to check if the
> caller has CAP_SYS_MODULE capability. This allows to align security
> checks of automatic module loading with the checks of the explicit operations.
>
> However for "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 only load "netdev-%s" modules with CAP_NET_ADMIN
> capability which is considered a privileged operation, we have two
> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
> the capability form request_module() to security_kernel_module_request()
> hook and let the capability subsystem decide.
>
> 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.
>
> The patch does not update request_module(), it updates the internal
> __request_module() that will take an extra "allow_cap" argument. If
> positive, then automatic module load operation can be allowed.

I find this refactor slightly confusing. I would expect to collapse
the existing caps checks in net/core/dev_ioctl.c and
net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
add a new non-__ function instead of requiring callers use
__request_module.

request_module_capable(int cap_required, fmt, args);

adjust __request_module() for the new arg, and when cap_required !=
-1, perform a cap check.

Then make request_module pass -1 to __request_module(), and change
dev_ioctl.c (and tcp_cong.c) from:

        if (no_module && capable(CAP_NET_ADMIN))
                no_module = request_module("netdev-%s", name);
        if (no_module && capable(CAP_SYS_MODULE))
                request_module("%s", name);

to:

        if (no_module)
                no_module = request_module_capable(CAP_NET_ADMIN,
"netdev-%s", name);
        if (no_module)
                no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);

that'll make the code cleaner, too.

> __request_module() will be only called by networking code which is the
> exception to this, so we do not break userspace and CAP_NET_ADMIN can
> continue to load 'netdev-%s' modules. Other kernel code should continue
> to use request_module() which calls security_kernel_module_request() and
> will check for CAP_SYS_MODULE capability in next patch. Allowing more
> control on who can trigger automatic module loading.
>
> This patch updates security_kernel_module_request() to take the
> 'allow_cap' argument and SELinux which is currently the only user of
> security_kernel_module_request() hook.
>
> Based on patch by Rusty Russell:
> https://lkml.org/lkml/2017/4/26/735
>
> 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>
>
> [1] https://lkml.org/lkml/2017/4/24/7
> ---
>  include/linux/kmod.h      | 15 ++++++++-------
>  include/linux/lsm_hooks.h |  4 +++-
>  include/linux/security.h  |  4 ++--
>  kernel/kmod.c             | 15 +++++++++++++--
>  net/core/dev_ioctl.c      | 10 +++++++++-
>  security/security.c       |  4 ++--
>  security/selinux/hooks.c  |  2 +-
>  7 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index c4e441e..a314432 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -32,18 +32,19 @@
>  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(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...);
>  #define try_then_request_module(x, mod...) \
> -       ((x) ?: (__request_module(true, mod), (x)))
> +       ((x) ?: (__request_module(true, -1, 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(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...)
> +{ return -ENOSYS; }
>  #define try_then_request_module(x, mod...) (x)
>  #endif
>
> +#define request_module(mod...) __request_module(true, -1, mod)
> +#define request_module_nowait(mod...) __request_module(false, -1, mod)
>
>  struct cred;
>  struct file;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f7914d9..7688f79 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -578,6 +578,8 @@
>   *     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
> + *     @allow_cap capability that allows to automatically load a kernel
> + *     module.

I would describe this as "required to load".

>   *     Return 0 if successful.
>   * @kernel_read_file:
>   *     Read a file specified by userspace.
> @@ -1516,7 +1518,7 @@ 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 allow_cap);
>         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 549cb82..2f4c9d3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,7 +325,7 @@ 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 allow_cap);
>  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);
> @@ -926,7 +926,7 @@ 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 allow_cap)
>  {
>         return 0;
>  }
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 563f97e..15c96e8 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -110,6 +110,7 @@ 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
> + * @allow_cap: if positive, may allow modprobe if this capability is set.
>   * @fmt: printf style format string for the name of the module
>   * @...: arguments as specified in the format string
>   *
> @@ -120,10 +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 "allow_cap" is positive, The security subsystem will trust the caller
> + * that "allow_cap" may allow to load some modules with a specific alias,
> + * the security subsystem will make some exceptions based on that. This is
> + * primally useful for backward compatibility. A permission check should not
> + * be that strict and userspace should be able to continue to trigger module
> + * auto-loading if needed.
> + *
>   * If module auto-loading support is disabled then this function
>   * becomes a no-operation.
> + *
> + * This function should not be directly used by other subsystems, for that
> + * please call request_module().
>   */
> -int __request_module(bool wait, const char *fmt, ...)
> +int __request_module(bool wait, int allow_cap, const char *fmt, ...)
>  {
>         va_list args;
>         char module_name[MODULE_NAME_LEN];
> @@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
>         if (ret >= MODULE_NAME_LEN)
>                 return -ENAMETOOLONG;
>
> -       ret = security_kernel_module_request(module_name);
> +       ret = security_kernel_module_request(module_name, allow_cap);
>         if (ret)
>                 return ret;
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index b94b1d2..c494351 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
>         rcu_read_unlock();
>
>         no_module = !dev;
> +       /*
> +        * First do the CAP_NET_ADMIN check, then let the security
> +        * subsystem checks know that this can be allowed since this is
> +        * a "netdev-%s" module and CAP_NET_ADMIN is set.
> +        *
> +        * For this exception call __request_module().
> +        */
>         if (no_module && capable(CAP_NET_ADMIN))
> -               no_module = request_module("netdev-%s", name);
> +               no_module = __request_module(true, CAP_NET_ADMIN,
> +                                            "netdev-%s", name);
>         if (no_module && capable(CAP_SYS_MODULE))
>                 request_module("%s", name);
>  }
> diff --git a/security/security.c b/security/security.c
> index 714433e..cedb790 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1021,9 +1021,9 @@ 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 allow_cap)
>  {
> -       return call_int_hook(kernel_module_request, 0, kmod_name);
> +       return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
>  }
>
>  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 158f6a0..85eeff6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3842,7 +3842,7 @@ 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 allow_cap)
>  {
>         struct common_audit_data ad;
>
> --
> 2.10.2
>

Otherwise, looks good!

-Kees
Djalal Harouni May 23, 2017, 10:29 a.m. UTC | #2
On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> This is a preparation patch for the module auto-load restriction feature.
>>
>> In order to restrict module auto-load operations we need to check if the
>> caller has CAP_SYS_MODULE capability. This allows to align security
>> checks of automatic module loading with the checks of the explicit operations.
>>
>> However for "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 only load "netdev-%s" modules with CAP_NET_ADMIN
>> capability which is considered a privileged operation, we have two
>> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
>> the capability form request_module() to security_kernel_module_request()
>> hook and let the capability subsystem decide.
>>
>> 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.
>>
>> The patch does not update request_module(), it updates the internal
>> __request_module() that will take an extra "allow_cap" argument. If
>> positive, then automatic module load operation can be allowed.
>
> I find this refactor slightly confusing. I would expect to collapse
> the existing caps checks in net/core/dev_ioctl.c and
> net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
> add a new non-__ function instead of requiring callers use
> __request_module.
>
> request_module_capable(int cap_required, fmt, args);
>
> adjust __request_module() for the new arg, and when cap_required !=
> -1, perform a cap check.
>
> Then make request_module pass -1 to __request_module(), and change
> dev_ioctl.c (and tcp_cong.c) from:
>
>         if (no_module && capable(CAP_NET_ADMIN))
>                 no_module = request_module("netdev-%s", name);
>         if (no_module && capable(CAP_SYS_MODULE))
>                 request_module("%s", name);
>
> to:
>
>         if (no_module)
>                 no_module = request_module_capable(CAP_NET_ADMIN,
> "netdev-%s", name);
>         if (no_module)
>                 no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);
>
> that'll make the code cleaner, too.

The refactoring in the patch is more for backward compatibility with
CAP_NET_ADMIN,
as discussed here: https://lkml.org/lkml/2017/4/26/147

I think if there is an interface request_module_capable() , then code
will use it. The DCCP code path did not check capabilities at all and
called request_module(), other code does the same.

A new interface can be abused, the result of this: we may break
"modules_autoload_mode" in mode 0 and 1. In the long term code will
want to change may_autoload_module() to also allow mode 1 to load a
module with CAP_NET_ADMIN or other caps in its own userns, resulting
in "modules_autoload_mode == 0 == 1". Without userns in the game we
may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
get the appropriate protocol.... and no one will be able to review all
this code or track new patches with request_module_capable() callers.

Kernel modules are global resources, and this patchset makes sure to
treat them that way. It aligns explicit and implicit module loading
operations with CAP_SYS_MODULE. The description is using PRIVILEGED in
regard for CAP_NET_ADMIN backward compatibility only. Capabilities
just got more useful thanks to the ambient caps, but please lets make
sure that inherited caps do not break "modules_autoload_mode=1" and
make it act like "modules_autoload_mode=0". We end up handing the
permission checks from the module subsystem to the ones that are
requesting it, the module subsystem should not blindly trust
request_module() calls that come from outdated modules.

I don't mind refactoring the code so it is better, but IMO we should
avoid exposing or playing the capability game unless there is a good
reason. If CAP_SYS_MODULE is not set, then my devices should not
autoload *old* modules without me, it is easy to set and to track.

I would also add, that the per-task module auto-load flag is using the
same *may_autoload_module()* checks for one reason: consistency. Some
capability usage in the kernel is not consistent, these patches make
sure to not fall into that trap. If you set the global sysctl for your
secure server, or the per-task for containers and sandboxes for
cluster nodes, desktops or whatever, the modules auto-load feature
will act only in one way and based on CAP_SYS_MODULE.

Does this make sense ?


>
>> __request_module() will be only called by networking code which is the
>> exception to this, so we do not break userspace and CAP_NET_ADMIN can
>> continue to load 'netdev-%s' modules. Other kernel code should continue
>> to use request_module() which calls security_kernel_module_request() and
>> will check for CAP_SYS_MODULE capability in next patch. Allowing more
>> control on who can trigger automatic module loading.
>>
>> This patch updates security_kernel_module_request() to take the
>> 'allow_cap' argument and SELinux which is currently the only user of
>> security_kernel_module_request() hook.
>>
>> Based on patch by Rusty Russell:
>> https://lkml.org/lkml/2017/4/26/735
>>
>> 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>
>>
>> [1] https://lkml.org/lkml/2017/4/24/7
>> ---
>>  include/linux/kmod.h      | 15 ++++++++-------
>>  include/linux/lsm_hooks.h |  4 +++-
>>  include/linux/security.h  |  4 ++--
>>  kernel/kmod.c             | 15 +++++++++++++--
>>  net/core/dev_ioctl.c      | 10 +++++++++-
>>  security/security.c       |  4 ++--
>>  security/selinux/hooks.c  |  2 +-
>>  7 files changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
>> index c4e441e..a314432 100644
>> --- a/include/linux/kmod.h
>> +++ b/include/linux/kmod.h
>> @@ -32,18 +32,19 @@
>>  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(3, 4)
>> +int __request_module(bool wait, int allow_cap, const char *name, ...);
>>  #define try_then_request_module(x, mod...) \
>> -       ((x) ?: (__request_module(true, mod), (x)))
>> +       ((x) ?: (__request_module(true, -1, 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(3, 4)
>> +int __request_module(bool wait, int allow_cap, const char *name, ...)
>> +{ return -ENOSYS; }
>>  #define try_then_request_module(x, mod...) (x)
>>  #endif
>>
>> +#define request_module(mod...) __request_module(true, -1, mod)
>> +#define request_module_nowait(mod...) __request_module(false, -1, mod)
>>
>>  struct cred;
>>  struct file;
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index f7914d9..7688f79 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -578,6 +578,8 @@
>>   *     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
>> + *     @allow_cap capability that allows to automatically load a kernel
>> + *     module.
>
> I would describe this as "required to load".

Ok, will fix it.


>>   *     Return 0 if successful.
>>   * @kernel_read_file:
>>   *     Read a file specified by userspace.
>> @@ -1516,7 +1518,7 @@ 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 allow_cap);
>>         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 549cb82..2f4c9d3 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -325,7 +325,7 @@ 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 allow_cap);
>>  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);
>> @@ -926,7 +926,7 @@ 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 allow_cap)
>>  {
>>         return 0;
>>  }
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 563f97e..15c96e8 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -110,6 +110,7 @@ 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
>> + * @allow_cap: if positive, may allow modprobe if this capability is set.
>>   * @fmt: printf style format string for the name of the module
>>   * @...: arguments as specified in the format string
>>   *
>> @@ -120,10 +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 "allow_cap" is positive, The security subsystem will trust the caller
>> + * that "allow_cap" may allow to load some modules with a specific alias,
>> + * the security subsystem will make some exceptions based on that. This is
>> + * primally useful for backward compatibility. A permission check should not
>> + * be that strict and userspace should be able to continue to trigger module
>> + * auto-loading if needed.
>> + *
>>   * If module auto-loading support is disabled then this function
>>   * becomes a no-operation.
>> + *
>> + * This function should not be directly used by other subsystems, for that
>> + * please call request_module().
>>   */
>> -int __request_module(bool wait, const char *fmt, ...)
>> +int __request_module(bool wait, int allow_cap, const char *fmt, ...)
>>  {
>>         va_list args;
>>         char module_name[MODULE_NAME_LEN];
>> @@ -150,7 +161,7 @@ int __request_module(bool wait, const char *fmt, ...)
>>         if (ret >= MODULE_NAME_LEN)
>>                 return -ENAMETOOLONG;
>>
>> -       ret = security_kernel_module_request(module_name);
>> +       ret = security_kernel_module_request(module_name, allow_cap);
>>         if (ret)
>>                 return ret;
>>
>> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
>> index b94b1d2..c494351 100644
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -366,8 +366,16 @@ void dev_load(struct net *net, const char *name)
>>         rcu_read_unlock();
>>
>>         no_module = !dev;
>> +       /*
>> +        * First do the CAP_NET_ADMIN check, then let the security
>> +        * subsystem checks know that this can be allowed since this is
>> +        * a "netdev-%s" module and CAP_NET_ADMIN is set.
>> +        *
>> +        * For this exception call __request_module().
>> +        */
>>         if (no_module && capable(CAP_NET_ADMIN))
>> -               no_module = request_module("netdev-%s", name);
>> +               no_module = __request_module(true, CAP_NET_ADMIN,
>> +                                            "netdev-%s", name);
>>         if (no_module && capable(CAP_SYS_MODULE))
>>                 request_module("%s", name);
>>  }
>> diff --git a/security/security.c b/security/security.c
>> index 714433e..cedb790 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1021,9 +1021,9 @@ 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 allow_cap)
>>  {
>> -       return call_int_hook(kernel_module_request, 0, kmod_name);
>> +       return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
>>  }
>>
>>  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 158f6a0..85eeff6 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3842,7 +3842,7 @@ 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 allow_cap)
>>  {
>>         struct common_audit_data ad;
>>
>> --
>> 2.10.2
>>
>
> Otherwise, looks good!
>

Thanks Kees, please let me know if you agree on the refactoring bits.
Kees Cook May 23, 2017, 7:19 p.m. UTC | #3
On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> This is a preparation patch for the module auto-load restriction feature.
>>>
>>> In order to restrict module auto-load operations we need to check if the
>>> caller has CAP_SYS_MODULE capability. This allows to align security
>>> checks of automatic module loading with the checks of the explicit operations.
>>>
>>> However for "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 only load "netdev-%s" modules with CAP_NET_ADMIN
>>> capability which is considered a privileged operation, we have two
>>> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
>>> the capability form request_module() to security_kernel_module_request()
>>> hook and let the capability subsystem decide.
>>>
>>> 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.
>>>
>>> The patch does not update request_module(), it updates the internal
>>> __request_module() that will take an extra "allow_cap" argument. If
>>> positive, then automatic module load operation can be allowed.
>>
>> I find this refactor slightly confusing. I would expect to collapse
>> the existing caps checks in net/core/dev_ioctl.c and
>> net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
>> add a new non-__ function instead of requiring callers use
>> __request_module.
>>
>> request_module_capable(int cap_required, fmt, args);
>>
>> adjust __request_module() for the new arg, and when cap_required !=
>> -1, perform a cap check.
>>
>> Then make request_module pass -1 to __request_module(), and change
>> dev_ioctl.c (and tcp_cong.c) from:
>>
>>         if (no_module && capable(CAP_NET_ADMIN))
>>                 no_module = request_module("netdev-%s", name);
>>         if (no_module && capable(CAP_SYS_MODULE))
>>                 request_module("%s", name);
>>
>> to:
>>
>>         if (no_module)
>>                 no_module = request_module_capable(CAP_NET_ADMIN,
>> "netdev-%s", name);
>>         if (no_module)
>>                 no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);
>>
>> that'll make the code cleaner, too.
>
> The refactoring in the patch is more for backward compatibility with
> CAP_NET_ADMIN,
> as discussed here: https://lkml.org/lkml/2017/4/26/147

I think Rusty and I are saying the same thing here, and I must be not
understanding something you're trying to explain. Apologies for being
dense.

> I think if there is an interface request_module_capable() , then code
> will use it. The DCCP code path did not check capabilities at all and
> called request_module(), other code does the same.
>
> A new interface can be abused, the result of this: we may break
> "modules_autoload_mode" in mode 0 and 1. In the long term code will
> want to change may_autoload_module() to also allow mode 1 to load a
> module with CAP_NET_ADMIN or other caps in its own userns, resulting
> in "modules_autoload_mode == 0 == 1". Without userns in the game we
> may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
> already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
> get the appropriate protocol.... and no one will be able to review all
> this code or track new patches with request_module_capable() callers.

I'm having some trouble following what you're saying here, but if I
understand, you're worried about getting the kernel into a state where
autoload state 0 == 1. Autoload 0 is "business as usual", and autoload
1 is "CAP_SYS_MODULE required to be able to trigger a module auto-load
operation, or CAP_NET_ADMIN for modules with a 'netdev-%s' alias."

In the v4 patch, under autoload==1, CAP_NET_ADMIN is needed to load
netdev- modules:

        if (no_module && capable(CAP_NET_ADMIN))
               no_module = __request_module(true, CAP_NET_ADMIN,
                                            "netdev-%s", name);

and in the LSM hook, CAP_NET_ADMIN is passed as an allowable "alias"
for the CAP_SYS_MODULE requirement:

       else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
               /* Check CAP_SYS_MODULE then allow_cap if valid */
               if (capable(CAP_SYS_MODULE) ||
                   (allow_cap > 0 && capable(allow_cap)))
                      return 0;
       }

What I see is some needless double-checking. Since you're making
changes to the request_module() API, it would be possible to have
request_module_cap(), which could be checked instead of open-coding
it:

     if (no_module)
        no_module = request_module_cap(CAP_NET_ADMIN, "netdev-%s", name);

If I'm understanding your objection correctly, it's that you want to
ONLY ever provide this one-time alias for CAP_SYS_MODULE with the
netdev-%s things, and you don't want to risk having other module
loading start using request_module_cap() which would lead to
CAP_SYS_MODULE aliases in other places?

If the goal is to make sure that only privileged processes are
autoloading, I don't think adding a well defined interface for
cap-checks (request_module_cap()) would lead to a slippery slope. The
worst case scenario (which would never happen) would be all
request_module() users would convert to request_module_cap(). This
would mean that all module loading would require specific privileges.
That seems in line with autoload==1. They would not be tied to
CAP_SYS_MODULE, though, which is, I suspect, what you're concerned
about.

Even in the existing code, there is a sense about CAP_NET_ADMIN and
CAP_SYS_MODULE having different privilege levels, in that
CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
load any module. What about refining request_module_cap() to _require_
an explicit string prefix instead of an arbitrary format string? e.g.
request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
make requests for ("netdev-%s", name)

I see a few options:

1) keep what you have for v4, and hope other places don't use
__request_module. (I'm not a fan of this.)

2) switch the logic on autoload==1 from OR to AND: both the specified
caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
autoload==1 less useful.)

3) use the request_module_cap() outlined above, which requires that
modules being loaded under a CAP_SYS_MODULE-aliased capability are at
least restricted to a subset of kernel module names.

4) same as 3 but also insert autoload==2 level that switches from OR
to AND (bumping existing ==2 to ==3).

What do you think?

-Kees
Djalal Harouni May 24, 2017, 2:16 p.m. UTC | #4
On Tue, May 23, 2017 at 9:19 PM, Kees Cook <keescook@google.com> wrote:
> On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
[...]

>> I think if there is an interface request_module_capable() , then code
>> will use it. The DCCP code path did not check capabilities at all and
>> called request_module(), other code does the same.
>>
>> A new interface can be abused, the result of this: we may break
>> "modules_autoload_mode" in mode 0 and 1. In the long term code will
>> want to change may_autoload_module() to also allow mode 1 to load a
>> module with CAP_NET_ADMIN or other caps in its own userns, resulting
>> in "modules_autoload_mode == 0 == 1". Without userns in the game we
>> may just see request_module_capable(CAP_SYS_ADMIN, ...)  . There is
>> already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
>> get the appropriate protocol.... and no one will be able to review all
>> this code or track new patches with request_module_capable() callers.
>
> I'm having some trouble following what you're saying here, but if I
> understand, you're worried about getting the kernel into a state where
> autoload state 0 == 1. Autoload 0 is "business as usual", and autoload
> 1 is "CAP_SYS_MODULE required to be able to trigger a module auto-load
> operation, or CAP_NET_ADMIN for modules with a 'netdev-%s' alias."

Indeed.


> In the v4 patch, under autoload==1, CAP_NET_ADMIN is needed to load
> netdev- modules:
>
>         if (no_module && capable(CAP_NET_ADMIN))
>                no_module = __request_module(true, CAP_NET_ADMIN,
>                                             "netdev-%s", name);
>
> and in the LSM hook, CAP_NET_ADMIN is passed as an allowable "alias"
> for the CAP_SYS_MODULE requirement:
>
>        else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
>                /* Check CAP_SYS_MODULE then allow_cap if valid */
>                if (capable(CAP_SYS_MODULE) ||
>                    (allow_cap > 0 && capable(allow_cap)))
>                       return 0;
>        }
>
> What I see is some needless double-checking. Since you're making
> changes to the request_module() API, it would be possible to have

That check is *not* a double check and it is *really* needed in v4
since how may_autoload_module() was implemented. It first checks if
'autoload' == 0 == ALLOWED, if so then it allows the operation
regardless of the capability. That's why I didn't want to touch
current network logic and assumed that net code knows what it should
do.


> request_module_cap(), which could be checked instead of open-coding
> it:
>
>      if (no_module)
>         no_module = request_module_cap(CAP_NET_ADMIN, "netdev-%s", name);
>
> If I'm understanding your objection correctly, it's that you want to
> ONLY ever provide this one-time alias for CAP_SYS_MODULE with the
> netdev-%s things, and you don't want to risk having other module
> loading start using request_module_cap() which would lead to
> CAP_SYS_MODULE aliases in other places?

Yes. We can't really track capabilities usage or new code.


> If the goal is to make sure that only privileged processes are
> autoloading, I don't think adding a well defined interface for
> cap-checks (request_module_cap()) would lead to a slippery slope. The
> worst case scenario (which would never happen) would be all
> request_module() users would convert to request_module_cap(). This

I am also concerned a bit with new code. In the documentation we
explicitly say CAP_SYS_MODULE, and new code should not break that
assumption.


> would mean that all module loading would require specific privileges.
> That seems in line with autoload==1. They would not be tied to
> CAP_SYS_MODULE, though, which is, I suspect, what you're concerned
> about.

Indeed, it is just easy to say hey it needs CAP_SYS_MODULE. The
capability usage in the module subsystem more precisely with explicit
loading is clean. CAP_SYS_MODULE is not overloaded, it has clear
focus. As you say it, we should be concerned if we blindly trust
callers and end up *aliasing* CAP_SYS_MODULE with some other cap...


> Even in the existing code, there is a sense about CAP_NET_ADMIN and
> CAP_SYS_MODULE having different privilege levels, in that
> CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
> load any module. What about refining request_module_cap() to _require_
> an explicit string prefix instead of an arbitrary format string? e.g.
> request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
> make requests for ("netdev-%s", name)
>
> I see a few options:
>
> 1) keep what you have for v4, and hope other places don't use
> __request_module. (I'm not a fan of this.)

Yes even if it is documented I wouldn't bet on it, though. :-)


> 2) switch the logic on autoload==1 from OR to AND: both the specified
> caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
> autoload==1 less useful.)

That will restrict some userspace that works only with CAP_NET_ADMIN.


> 3) use the request_module_cap() outlined above, which requires that
> modules being loaded under a CAP_SYS_MODULE-aliased capability are at
> least restricted to a subset of kernel module names.

This one tends to allow usability.


> 4) same as 3 but also insert autoload==2 level that switches from OR
> to AND (bumping existing ==2 to ==3).

I wouldn't expose autoload to callers, I think it is better if it
stays a property of the module subsystem. But lets use the bump idea,
please see below.


> What do you think?

Ok so given that we already have modules_autoload_mode=2 disabled,
maybe we go with 3)  like this ?

int __request_module(bool wait, int required_cap, const char *prefix,
const char *name, ...);
#define request_module(mod...) \
        __request_module(true, -1, NULL, mod)
#define request_module_cap(required_cap, prefix, mod...) \
        __request_module(true, required_cap, prefix, mod)

and we require allow_cap and prefix to be set.

request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
net/core/dev_ioctl.c:dev_load()

request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
net/ipv4/tcp_cong.c  functions.


Then
__request_module()
  -> security_kernel_module_request(module_name, required_cap, prefix)
     -> may_autoload_module(current, module_name, required_cap, prefix)


And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
and CAP_SYS_MODULE inside and make them the only capabilities needed
for a privileged auto-load operation.

request_module_cap(CAP_SYS_MODULE, ...) or
request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
privileged operation.

Kees will this work ?

Jessica,  Rusty,  Serge. What do you think ? I definitively think that
module_autoload should be contained only inside the module subsystem..


+int may_autoload_module(struct task_struct *task, char *kmod_name,
+                       int require_cap, char *prefix)
+{
+       unsigned int autoload;
+       int module_require_cap = 0;
+
+       if (require_cap > 0) {
+               if (prefix == NULL || *prefix == '\0')
+                       return -EPERM;
+
+               /*
+                * We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
+                * 'netdev-%s' modules for backward compatibility.
+                * Please do not overload capabilities.
+                */
+               if (require_cap == CAP_SYS_MODULE ||
+                   require_cap == CAP_NET_ADMIN)
+                       module_require_cap = require_cap;
+               else
+                       return -EPERM;
+       }
+
+       /* Get max value of sysctl and task "modules_autoload_mode" */
+       autoload = max_t(unsigned int, modules_autoload_mode,
+                        task->modules_autoload_mode);
+
+       /*
+        * If autoload is disabled then fail here and not bother at all
+        */
+       if (autoload == MODULES_AUTOLOAD_DISABLED)
+               return -EPERM;
+
+       /*
+        * If caller require capabilities then we may not allow
+        * automatic module loading. We should not bypass callers.
+        * This allows to support networking code that uses CAP_NET_ADMIN
+        * for some aliased 'netdev-%s' modules.
+        *
+        * Explicitly bump autoload here if necessary
+        */
+       if (module_require_cap && autoload == MODULES_AUTOLOAD_ALLOWED)
+               autoload = MODULES_AUTOLOAD_PRIVILEGED;
+
+       if (autoload == MODULES_AUTOLOAD_ALLOWED)
+               return 0;
+       else if(autoload == MODULES_AUTOLOAD_PRIVILEGED) {
+               /*
+                * If module auto-load is a privileged operation then check
+                * if capabilities are set.
+                */
+               if (capable(CAP_SYS_MODULE) ||
+                   (module_require_cap && capable(module_require_cap)))
+                       return 0;
+       }
+
+       return -EPERM;
+}
+
--
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 May 30, 2017, 5:59 p.m. UTC | #5
On Wed, May 24, 2017 at 7:16 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Tue, May 23, 2017 at 9:19 PM, Kees Cook <keescook@google.com> wrote:
>> On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> Even in the existing code, there is a sense about CAP_NET_ADMIN and
>> CAP_SYS_MODULE having different privilege levels, in that
>> CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
>> load any module. What about refining request_module_cap() to _require_
>> an explicit string prefix instead of an arbitrary format string? e.g.
>> request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
>> make requests for ("netdev-%s", name)
>>
>> I see a few options:
>>
>> 1) keep what you have for v4, and hope other places don't use
>> __request_module. (I'm not a fan of this.)
>
> Yes even if it is documented I wouldn't bet on it, though. :-)

Okay, we seem to agree: we'll not use #1.

>> 2) switch the logic on autoload==1 from OR to AND: both the specified
>> caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
>> autoload==1 less useful.)
>
> That will restrict some userspace that works only with CAP_NET_ADMIN.

Nor #2.

>> 3) use the request_module_cap() outlined above, which requires that
>> modules being loaded under a CAP_SYS_MODULE-aliased capability are at
>> least restricted to a subset of kernel module names.
>
> This one tends to allow usability.

Right, discussed below...

>> 4) same as 3 but also insert autoload==2 level that switches from OR
>> to AND (bumping existing ==2 to ==3).
>
> I wouldn't expose autoload to callers, I think it is better if it
> stays a property of the module subsystem. But lets use the bump idea,
> please see below.

If we can't agree below, I think #4 would be a good way to allow for
both states.

>> What do you think?
>
> Ok so given that we already have modules_autoload_mode=2 disabled,
> maybe we go with 3)  like this ?
>
> int __request_module(bool wait, int required_cap, const char *prefix,
> const char *name, ...);
> #define request_module(mod...) \
>         __request_module(true, -1, NULL, mod)
> #define request_module_cap(required_cap, prefix, mod...) \
>         __request_module(true, required_cap, prefix, mod)
>
> and we require allow_cap and prefix to be set.
>
> request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
> net/core/dev_ioctl.c:dev_load()
>
> request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
> net/ipv4/tcp_cong.c  functions.
>
>
> Then
> __request_module()
>   -> security_kernel_module_request(module_name, required_cap, prefix)
>      -> may_autoload_module(current, module_name, required_cap, prefix)
>
>
> And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
> and CAP_SYS_MODULE inside and make them the only capabilities needed
> for a privileged auto-load operation.

I still think making a specific exception for CAP_NET_ADMIN is not the
right solution, instead allowing for non-CAP_SYS_MODULE caps when
using a distinct prefix.

> request_module_cap(CAP_SYS_MODULE, ...) or
> request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
> privileged operation.
>
> Kees will this work ?
>
> Jessica,  Rusty,  Serge. What do you think ? I definitively think that
> module_autoload should be contained only inside the module subsystem..

I'd change it like this:

> +int may_autoload_module(struct task_struct *task, char *kmod_name,
> +                       int require_cap, char *prefix)
> +{
> +       unsigned int autoload;
> +       int module_require_cap = 0;

I'd initialize this to module_require_cap = CAP_SYS_MODULE;

> +
> +       if (require_cap > 0) {
> +               if (prefix == NULL || *prefix == '\0')
> +                       return -EPERM;

Since an unprefixed module load should only be CAP_SYS_MODULE, change
the above "if" to:

    if (require_cap > 0 && prefix != NULL && *prefix != '\0')

> +
> +               /*
> +                * We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
> +                * 'netdev-%s' modules for backward compatibility.
> +                * Please do not overload capabilities.
> +                */
> +               if (require_cap == CAP_SYS_MODULE ||
> +                   require_cap == CAP_NET_ADMIN)
> +                       module_require_cap = require_cap;
> +               else
> +                       return -EPERM;
> +       }

And then drop all these checks, leaving only:

        module_require_cap = require_cap;

> +
> +       /* Get max value of sysctl and task "modules_autoload_mode" */
> +       autoload = max_t(unsigned int, modules_autoload_mode,
> +                        task->modules_autoload_mode);
> +
> +       /*
> +        * If autoload is disabled then fail here and not bother at all
> +        */
> +       if (autoload == MODULES_AUTOLOAD_DISABLED)
> +               return -EPERM;
> +
> +       /*
> +        * If caller require capabilities then we may not allow
> +        * automatic module loading. We should not bypass callers.
> +        * This allows to support networking code that uses CAP_NET_ADMIN
> +        * for some aliased 'netdev-%s' modules.
> +        *
> +        * Explicitly bump autoload here if necessary
> +        */
> +       if (module_require_cap && autoload == MODULES_AUTOLOAD_ALLOWED)
> +               autoload = MODULES_AUTOLOAD_PRIVILEGED;

I don't see a reason to bump the autoload level.

> +
> +       if (autoload == MODULES_AUTOLOAD_ALLOWED)
> +               return 0;

This test can be moved to above the AUTOLOAD_DISABLED test.

> +       else if(autoload == MODULES_AUTOLOAD_PRIVILEGED) {
> +               /*
> +                * If module auto-load is a privileged operation then check
> +                * if capabilities are set.
> +                */
> +               if (capable(CAP_SYS_MODULE) ||
> +                   (module_require_cap && capable(module_require_cap)))
> +                       return 0;
> +       }

This test could drop the explicit CAP_SYS_MODULE test and just rely on
module_require_cap.

> +
> +       return -EPERM;
> +}
> +

So, I would suggest:

int may_autoload_module(struct task_struct *task, char *kmod_name,
                       int require_cap, char *prefix)
{
        unsigned int autoload;
        int module_require_cap;

        if (autoload == MODULES_AUTOLOAD_DISABLED)
                return -EPERM;

        /* Get max value of sysctl and task "modules_autoload_mode" */
        autoload = max_t(unsigned int, modules_autoload_mode,
                        task->modules_autoload_mode);

        if (autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

        /*
         * It should be impossible for autoload to have any other
         * value at this point, so explicitly reject all other states.
         */
        if (autoload != MODULES_AUTOLOAD_PRIVILEGED)
                return -EPERM;

        /* Verify that alternate capabilities requirements had a prefix. */
        if (require_cap > 0 && prefix != NULL && *prefix != '\0')
                module_require_cap = require_cap;
        else
                module_require_cap = CAP_SYS_MODULE;

        return capable(module_require_cap);
}

-Kees
Djalal Harouni June 1, 2017, 2:56 p.m. UTC | #6
On Tue, May 30, 2017 at 7:59 PM, Kees Cook <keescook@google.com> wrote:
[...]
>>> I see a few options:
>>>
>>> 1) keep what you have for v4, and hope other places don't use
>>> __request_module. (I'm not a fan of this.)
>>
>> Yes even if it is documented I wouldn't bet on it, though. :-)
>
> Okay, we seem to agree: we'll not use #1.
>
>>> 2) switch the logic on autoload==1 from OR to AND: both the specified
>>> caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
>>> autoload==1 less useful.)
>>
>> That will restrict some userspace that works only with CAP_NET_ADMIN.
>
> Nor #2.
>
>>> 3) use the request_module_cap() outlined above, which requires that
>>> modules being loaded under a CAP_SYS_MODULE-aliased capability are at
>>> least restricted to a subset of kernel module names.
>>
>> This one tends to allow usability.
>
> Right, discussed below...
>
>>> 4) same as 3 but also insert autoload==2 level that switches from OR
>>> to AND (bumping existing ==2 to ==3).
>>
>> I wouldn't expose autoload to callers, I think it is better if it
>> stays a property of the module subsystem. But lets use the bump idea,
>> please see below.
>
> If we can't agree below, I think #4 would be a good way to allow for
> both states.

Ok!


>>> What do you think?
>>
>> Ok so given that we already have modules_autoload_mode=2 disabled,
>> maybe we go with 3)  like this ?
>>
>> int __request_module(bool wait, int required_cap, const char *prefix,
>> const char *name, ...);
>> #define request_module(mod...) \
>>         __request_module(true, -1, NULL, mod)
>> #define request_module_cap(required_cap, prefix, mod...) \
>>         __request_module(true, required_cap, prefix, mod)
>>
>> and we require allow_cap and prefix to be set.
>>
>> request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
>> net/core/dev_ioctl.c:dev_load()
>>
>> request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
>> net/ipv4/tcp_cong.c  functions.
>>
>>
>> Then
>> __request_module()
>>   -> security_kernel_module_request(module_name, required_cap, prefix)
>>      -> may_autoload_module(current, module_name, required_cap, prefix)
>>
>>
>> And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
>> and CAP_SYS_MODULE inside and make them the only capabilities needed
>> for a privileged auto-load operation.
>
> I still think making a specific exception for CAP_NET_ADMIN is not the
> right solution, instead allowing for non-CAP_SYS_MODULE caps when
> using a distinct prefix.

Alright! I would have loved to avoid capabilities game, but these
patches also use them... so worst scenario is the per-task can always
be set, "task->module_autoload_mode=2" and block it if necessary.


>> request_module_cap(CAP_SYS_MODULE, ...) or
>> request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
>> privileged operation.
>>
>> Kees will this work ?
>>
>> Jessica,  Rusty,  Serge. What do you think ? I definitively think that
>> module_autoload should be contained only inside the module subsystem..
>
> I'd change it like this:
>
>> +int may_autoload_module(struct task_struct *task, char *kmod_name,
>> +                       int require_cap, char *prefix)
>> +{
>> +       unsigned int autoload;
>> +       int module_require_cap = 0;
>
> I'd initialize this to module_require_cap = CAP_SYS_MODULE;

Ok, please see below.



>> +
>> +       if (require_cap > 0) {
>> +               if (prefix == NULL || *prefix == '\0')
>> +                       return -EPERM;
>
> Since an unprefixed module load should only be CAP_SYS_MODULE, change
> the above "if" to:
>
>     if (require_cap > 0 && prefix != NULL && *prefix != '\0')
>
>> +
>> +               /*
>> +                * We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
>> +                * 'netdev-%s' modules for backward compatibility.
>> +                * Please do not overload capabilities.
>> +                */
>> +               if (require_cap == CAP_SYS_MODULE ||
>> +                   require_cap == CAP_NET_ADMIN)
>> +                       module_require_cap = require_cap;
>> +               else
>> +                       return -EPERM;
>> +       }
>
> And then drop all these checks, leaving only:
>
>         module_require_cap = require_cap;
>
>> +
>> +       /* Get max value of sysctl and task "modules_autoload_mode" */
>> +       autoload = max_t(unsigned int, modules_autoload_mode,
>> +                        task->modules_autoload_mode);
>> +
>> +       /*
>> +        * If autoload is disabled then fail here and not bother at all
>> +        */
>> +       if (autoload == MODULES_AUTOLOAD_DISABLED)
>> +               return -EPERM;
>> +
>> +       /*
>> +        * If caller require capabilities then we may not allow
>> +        * automatic module loading. We should not bypass callers.
>> +        * This allows to support networking code that uses CAP_NET_ADMIN
>> +        * for some aliased 'netdev-%s' modules.
>> +        *
>> +        * Explicitly bump autoload here if necessary
>> +        */
>> +       if (module_require_cap && autoload == MODULES_AUTOLOAD_ALLOWED)
>> +               autoload = MODULES_AUTOLOAD_PRIVILEGED;
>
> I don't see a reason to bump the autoload level.
>
>> +
>> +       if (autoload == MODULES_AUTOLOAD_ALLOWED)
>> +               return 0;
>
> This test can be moved to above the AUTOLOAD_DISABLED test.
>
>> +       else if(autoload == MODULES_AUTOLOAD_PRIVILEGED) {
>> +               /*
>> +                * If module auto-load is a privileged operation then check
>> +                * if capabilities are set.
>> +                */
>> +               if (capable(CAP_SYS_MODULE) ||
>> +                   (module_require_cap && capable(module_require_cap)))
>> +                       return 0;
>> +       }
>
> This test could drop the explicit CAP_SYS_MODULE test and just rely on
> module_require_cap.
>
>> +
>> +       return -EPERM;
>> +}
>> +
>
> So, I would suggest:

Ok Kees, I will update based on your feedback, except for the
following, please see below and let me know :-)


>
> int may_autoload_module(struct task_struct *task, char *kmod_name,
>                        int require_cap, char *prefix)
> {
>         unsigned int autoload;
>         int module_require_cap;
>
>         if (autoload == MODULES_AUTOLOAD_DISABLED)
>                 return -EPERM;
>
>         /* Get max value of sysctl and task "modules_autoload_mode" */
>         autoload = max_t(unsigned int, modules_autoload_mode,
>                         task->modules_autoload_mode);
>
>         if (autoload == MODULES_AUTOLOAD_ALLOWED)
>                 return 0;

I don't think that the MODULES_AUTOLOAD_ALLOWED check here at this
place is the best thing to do.

If we remove the capable(CAP_NET_ADMIN) from net/core/dev_ioctl:dev_load()

http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369

Or if future changes (accidental) remove that capable(CAP_NET_ADMIN)
and replace it with:
   request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name);


Then we will check the requested capability *after* autoload allowed
as it is in this example, it should be checked *before* the autoload
allowed:
        // Check required capability before this
        if (autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

This way we are still safe we do not downgrade the required capability
that was requested by the calling subsystem based on
MODULES_AUTOLOAD_ALLOWED. If networking code or any other code thinks
that we need CAP_X to load a module then we should honor it. So we do
not break current usage by introducing the "modules_autoload_mode", it
should be set regardless of the autoload mode. Especially since
modules autoload mode is 0 by default. This avoids breaking current
rule to require CAP_NET_ADMIN for 'netdevè-%'


>         /*
>          * It should be impossible for autoload to have any other
>          * value at this point, so explicitly reject all other states.
>          */
>         if (autoload != MODULES_AUTOLOAD_PRIVILEGED)
>                 return -EPERM;
>
>         /* Verify that alternate capabilities requirements had a prefix. */
>         if (require_cap > 0 && prefix != NULL && *prefix != '\0')
>                 module_require_cap = require_cap;
>         else
>                 module_require_cap = CAP_SYS_MODULE;
>
>         return capable(module_require_cap);

So with your code, but I really think that we should treat
MODULES_AUTOLOAD_ALLOWED with special care in regard of the passed
capabilities, so:


        module_require_cap = 0;

        if (autoload == MODULES_AUTOLOAD_DISABLED)
                return -EPERM;

        if (autoload == MODULES_AUTOLOAD_PRIVILEGED || require_cap > 0) {
                if (prefix != NULL && *prefix != '\0')
                        /*
                         * Allow non-CAP_SYS_MODULE caps when
                         * using a distinct prefix.
                         */
                        module_require_cap = require_cap;
                else
                        /*
                         * Otherwise always require CAP_SYS_MODULE if no
                         * valid prefix. Callers that do not provide a
valid prefix
                         * should not provide a require_cap > 0
                         */
                        module_require_cap = CAP_SYS_MODULE;
        }

        /* If autoload allowed and 'module_require_cap' was *never*
set, allow */
        if (module_require_cap == 0 && autoload == MODULES_AUTOLOAD_ALLOWED)
                return 0;

        return capable(module_require_cap) ? 0 : -EPERM;
> }
>

Maybe you will agree :-) ?

BTW Kees, also in next version I won't remove the
capable(CAP_NET_ADMIN) check from [1]
even if there is the new request_module_cap(), I would like it to be
in a different patches, this way we go incremental
and maybe it is better to merge what we have now ?  and follow up
later, and of course if other maintainers agree too!

I just need a bit of free time to check again everything and will send
a v5 with all requested changes.


Thank you Kees for your time!

[1] http://elixir.free-electrons.com/linux/v4.12-rc3/source/net/core/dev_ioctl.c#L369
--
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 June 1, 2017, 7:10 p.m. UTC | #7
On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>         module_require_cap = 0;
>
>         if (autoload == MODULES_AUTOLOAD_DISABLED)
>                 return -EPERM;
>
>         if (autoload == MODULES_AUTOLOAD_PRIVILEGED || require_cap > 0) {
>                 if (prefix != NULL && *prefix != '\0')
>                         /*
>                          * Allow non-CAP_SYS_MODULE caps when
>                          * using a distinct prefix.
>                          */
>                         module_require_cap = require_cap;
>                 else
>                         /*
>                          * Otherwise always require CAP_SYS_MODULE if no
>                          * valid prefix. Callers that do not provide a valid prefix
>                          * should not provide a require_cap > 0
>                          */
>                         module_require_cap = CAP_SYS_MODULE;
>         }
>
>         /* If autoload allowed and 'module_require_cap' was *never* set, allow */
>         if (module_require_cap == 0 && autoload == MODULES_AUTOLOAD_ALLOWED)
>                 return 0;
>
>         return capable(module_require_cap) ? 0 : -EPERM;
>
> Maybe you will agree :-) ?

Yes! Looks good. I was accidentally still thinking about the caps
checks being in the net code, but obviously, that wouldn't be the case
anymore. Thanks for the catch. :)

> BTW Kees, also in next version I won't remove the
> capable(CAP_NET_ADMIN) check from [1]
> even if there is the new request_module_cap(), I would like it to be
> in a different patches, this way we go incremental
> and maybe it is better to merge what we have now ?  and follow up
> later, and of course if other maintainers agree too!

Yes, incremental. I would suggest first creating the API changes to
move a basic require_cap test into the LSM (which would drop the
open-coded capable() checks in the net code), and then add the
autoload logic in the following patches. That way the "infrastructure"
changes happen separately and do not change any behaviors, but moves
the caps test down where its wanted in the LSM, before then augmenting
the logic.

> I just need a bit of free time to check again everything and will send
> a v5 with all requested changes.

Great, thank you!

-Kees
Djalal Harouni Sept. 2, 2017, 6:31 a.m. UTC | #8
Hi Kees,

On Thu, Jun 1, 2017 at 9:10 PM, Kees Cook <keescook@google.com> wrote:
> On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
...
>
>> BTW Kees, also in next version I won't remove the
>> capable(CAP_NET_ADMIN) check from [1]
>> even if there is the new request_module_cap(), I would like it to be
>> in a different patches, this way we go incremental
>> and maybe it is better to merge what we have now ?  and follow up
>> later, and of course if other maintainers agree too!
>
> Yes, incremental. I would suggest first creating the API changes to
> move a basic require_cap test into the LSM (which would drop the
> open-coded capable() checks in the net code), and then add the
> autoload logic in the following patches. That way the "infrastructure"
> changes happen separately and do not change any behaviors, but moves
> the caps test down where its wanted in the LSM, before then augmenting
> the logic.
>
>> I just need a bit of free time to check again everything and will send
>> a v5 with all requested changes.
>
> Great, thank you!
>

So sorry was busy these last months, I picked it again, will send v5 after the
merge window.

Kees I am looking on a way to integrate a test for it, we should use
something like
the example here [1] or maybe something else ? and which module to use ?

I still did not sort this out, if anyone has some suggestions, thank
you in advance!


[1] http://openwall.com/lists/kernel-hardening/2017/05/22/7
diff mbox

Patch

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index c4e441e..a314432 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -32,18 +32,19 @@ 
 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(3, 4)
+int __request_module(bool wait, int allow_cap, const char *name, ...);
 #define try_then_request_module(x, mod...) \
-	((x) ?: (__request_module(true, mod), (x)))
+	((x) ?: (__request_module(true, -1, 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(3, 4)
+int __request_module(bool wait, int allow_cap, const char *name, ...)
+{ return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
 #endif
 
+#define request_module(mod...) __request_module(true, -1, mod)
+#define request_module_nowait(mod...) __request_module(false, -1, mod)
 
 struct cred;
 struct file;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f7914d9..7688f79 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -578,6 +578,8 @@ 
  *	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
+ *	@allow_cap capability that allows to automatically load a kernel
+ *	module.
  *	Return 0 if successful.
  * @kernel_read_file:
  *	Read a file specified by userspace.
@@ -1516,7 +1518,7 @@  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 allow_cap);
 	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 549cb82..2f4c9d3 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -325,7 +325,7 @@  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 allow_cap);
 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);
@@ -926,7 +926,7 @@  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 allow_cap)
 {
 	return 0;
 }
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 563f97e..15c96e8 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -110,6 +110,7 @@  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
+ * @allow_cap: if positive, may allow modprobe if this capability is set.
  * @fmt: printf style format string for the name of the module
  * @...: arguments as specified in the format string
  *
@@ -120,10 +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 "allow_cap" is positive, The security subsystem will trust the caller
+ * that "allow_cap" may allow to load some modules with a specific alias,
+ * the security subsystem will make some exceptions based on that. This is
+ * primally useful for backward compatibility. A permission check should not
+ * be that strict and userspace should be able to continue to trigger module
+ * auto-loading if needed.
+ *
  * If module auto-loading support is disabled then this function
  * becomes a no-operation.
+ *
+ * This function should not be directly used by other subsystems, for that
+ * please call request_module().
  */
-int __request_module(bool wait, const char *fmt, ...)
+int __request_module(bool wait, int allow_cap, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
@@ -150,7 +161,7 @@  int __request_module(bool wait, const char *fmt, ...)
 	if (ret >= MODULE_NAME_LEN)
 		return -ENAMETOOLONG;
 
-	ret = security_kernel_module_request(module_name);
+	ret = security_kernel_module_request(module_name, allow_cap);
 	if (ret)
 		return ret;
 
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index b94b1d2..c494351 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -366,8 +366,16 @@  void dev_load(struct net *net, const char *name)
 	rcu_read_unlock();
 
 	no_module = !dev;
+	/*
+	 * First do the CAP_NET_ADMIN check, then let the security
+	 * subsystem checks know that this can be allowed since this is
+	 * a "netdev-%s" module and CAP_NET_ADMIN is set.
+	 *
+	 * For this exception call __request_module().
+	 */
 	if (no_module && capable(CAP_NET_ADMIN))
-		no_module = request_module("netdev-%s", name);
+		no_module = __request_module(true, CAP_NET_ADMIN,
+					     "netdev-%s", name);
 	if (no_module && capable(CAP_SYS_MODULE))
 		request_module("%s", name);
 }
diff --git a/security/security.c b/security/security.c
index 714433e..cedb790 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1021,9 +1021,9 @@  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 allow_cap)
 {
-	return call_int_hook(kernel_module_request, 0, kmod_name);
+	return call_int_hook(kernel_module_request, 0, kmod_name, allow_cap);
 }
 
 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 158f6a0..85eeff6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3842,7 +3842,7 @@  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 allow_cap)
 {
 	struct common_audit_data ad;