Message-ID: <CAGXu5jL9jRDkHj20kf4O6VrjiEpvXGzN3nn5Lw4fT2sx_Xg=+A@mail.gmail.com>
Date:   Mon, 22 May 2017 15:20:56 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Djalal Harouni <tixxdz@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        Andy Lutomirski <luto@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Rusty Russell <rusty@...tcorp.com.au>,
        "Serge E. Hallyn" <serge@...lyn.com>, Jessica Yu <jeyu@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        James Morris <james.l.morris@...cle.com>,
        Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Ingo Molnar <mingo@...nel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Dongsu Park <dpark@...teo.net>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Jonathan Corbet <corbet@....net>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Zendyani <zendyani@...il.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Ben Hutchings <ben.hutchings@...ethink.co.uk>
Subject: Re: [PATCH v4 next 1/3] modules:capabilities: allow
 __request_module() to take a capability argument

On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@...il.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@...lyn.com>
> Cc: Andy Lutomirski <luto@...nel.org>
> Suggested-by: Rusty Russell <rusty@...tcorp.com.au>
> Suggested-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Djalal Harouni <tixxdz@...il.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

-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists