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

[Djalal Harouni]

On Tue, May 30, 2017 at 7:59 PM, Kees Cook <keescook@xxxxxxxxxx> 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