On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tixxdz@xxxxxxxxx> wrote:
> On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tixxdz@xxxxxxxxx> 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
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
