On Wed, Nov 29, 2017 at 2:14 PM, Linus Torvalds
[off-list ref] wrote:

On Wed, Nov 29, 2017 at 1:17 PM, Kees Cook [off-list ref] wrote:

quoted

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.

Yeah, agreed; it's limited in scope.

quoted

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

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

Oops, yes, think-o.

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

So, if some catch-all is needed, maybe request_module_unpriv(...), but
I think it should be possible to identify SOME kind of defining
privilege to check.

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".

I just want to make sure I'm visualizing this correctly. Using the
misc_open() example, you're thinking something like:

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

which would then do a verification that file->f_cred matched
current_cred()? (For misc_open(), this is obviously going to always be
okay, but other cases may be further from the fops open handler...)

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.

Right, we have both directions -- hotplug module loading ("I saw this
PCI alias, load something! Oh! It needs some fancy crypto too!") and
on-use module loading. It seems like on-use module loading would get
covered by request_module_dev(), and the hotplug case would always be
privileged. Anyway... exploring this with real code is clearly
warranted.

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.

Right, and those would all be privileged, etc.

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.

Yeah, going through the call sites and asking "where does the
privilege for loading this module derive from?" for each one will help
shape the resulting API changes.

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

This is where a lot of the exposure really comes from. socket()
triggers a bunch of stuff, but doesn't have an obvious privilege
associated with it... while it already does the name templates, maybe
add request_module_socket() just to explicitly mark it?

quoted

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.

quoted

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

Yes.

Okay, excellent. Hopefully we haven't scared off Djalal, and hopefully
Jessica doesn't think this is all insane. :)

Thanks!

-Kees

-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html