On Tue, May 30, 2017 at 7:59 PM, Kees Cook <[email protected]> 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
On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <[email protected]> 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
--
Kees Cook
Pixel Security
Hi Kees,
On Thu, Jun 1, 2017 at 9:10 PM, Kees Cook <[email protected]> wrote:
> On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <[email protected]> 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
--
tixxdz