Re: [kernel-hardening] Re: [PATCH v5 next 5/5] net: modules: use request_module_cap() to load 'netdev-%s' modules

[Linus Torvalds]

On Wed, Nov 29, 2017 at 1:17 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> So, what we have now is that the permission verification already
> happens at and around the existing request_module() calls.

Usually, yes.

I liked the "request_module_cap()" interface partly because that made
the net/core/dev_ioctl.c ones more explicit, and maybe it could be
convenient if we make other places do similar things.

I was hoping some other users could be converted, but grepping around,
there's no obvious cases. There is tcp_cong.c and tcp_ulp.c, but they
want some extra locking in between the checking..

> It still sounds like you'd like to see an explicit change, similar to
> the proposed request_module_cap(), that identifies the privilege
> expectations on a per-call-site basis. How about this plan:

Yes.

I'd be perfectly happy to have a long-range plan where the existing
"request_module()" ends up requiring more capabilities.

I just don't think it's a good first step, exactly because *if* it's a
first step, it basically has to be disabled by default.

And once you disable it by default, and it becomes purely opt-in, that
means that nothing will change for most cases. Some embedded people
that do their own thing (ie Android) might change, but normal
distributions probably won't.

Yes, Android may be 99% of the users, and yes, the embedded world in
general needs to be secure, but I'd still like this to be something
that helps _everybody_.

So:

> 1) Add request_module_cap(required_cap, module_name_prefix, fmt, fmt_args...)
>
> 2) Convert known privileged-but-not-CAP_SYS_MODULE request_module()
> callers to request_module_cap(the_needed_cap, prefix, ...)

Yes. The upside seems to be very limited here, but at least it makes
the users that use CAP_NET_ADMIN instead of CAP_SYS_MODULE able to
specify so.

> 2) Convert known unprivileged callers to use request_module_cap(0, ...)

0 is CAP_CHOWN, so it would have to be -1.

And I wouldn't actually want to see that as-is. Not only would I not
want to see people have that "-1" in random driver subsystems, I'd
much prefer to have actual helper naming that descibes why something
is ok

Because as mentioned, I think there are valid permission reasons that
are _not_ about capabilities that make you able to load a module.

If you can open a character device node, then "misc_open()" will do

                request_module("char-major-%d-%d", MISC_MAJOR, minor);

and there is nothing about capabilities in the CAP_SYS_MODULE sense
about the user. But the user _did_ have the privileges to open that
character device file.

That's why I suggested something like request_module_dev(): it's not
at all the same thing as request_module_cap(-1, ...), saying "I don't
need/have a capability". It's saying something else entirely, it's
basically saying "I have the right based on device permissions".

And something like request_module_dev() might even have real semantic
meaning, exactly because it says "this module request comes from
trying to open a device node".

Why would that be? If we know we're on a system where /dev is
auto-populated through udev, then the device nodes should have been
created by the drivers, not the other way around. So we might even
have a rule that notices that automatically, and simply disables
request_module_dev() entirely.

Anyway, I'm not saying that is necessarily something we should do, but
I do suspect that we could adapt to modern systems without having to
have tons of magic settings, and try to be as strict as possible
without breaking them.

Because I dislike "system tuning" in general. I hate knobs that do
kernel performance tuning - we try very hard to just DTRT wrt sizing
hashes etc instead of expecting the system admin to set flags.

And I think we can try to avoid some system tuning in this area too.

I suspect that for a lot of our existing request_module() cases, they
really are pretty trivial. In most cases, it's probably really about
whether you have the hardware or not.

So for the hardware driver cases, either the hardware enumerates
itself, or it is presumably set up by the system scripts anyway, and
CAP_SYS_MODULE is all fine. The "open device node" case is one special
case, though.

That mainly leaves the protocol ones we need to look out for, I suspect.

> 3) Add WARN_RATELIMIT for request_module() calls without
> CAP_SYS_MODULE to shake out other places where request_module_cap() is
> needed.

Yes.

And this is where I hope that there really aren't actually all that
many cases that will warn, and that it's hopefully easy to simply just
look at a handful of reports and say "ok, that case is obviously
fine".

And I may be wrong.

> 4) Adapt the original patch series to add the per-process flag that
> can block privilege elevations.

Yes.

            Linus