On Thu, Jan 12, 2023 at 10:53:07PM +0100, Vegard Nossum wrote:
> On 1/12/23 19:45, Luis Chamberlain wrote:
> > On Thu, Jan 12, 2023 at 02:19:11PM +0100, Vegard Nossum wrote:
> > > +ns_modules_allowed
> > > +==================
> > > +
> > > +Control whether processes may trigger module loading inside a user namespace.
> >
> > This is false documentation. The place it is trying to protect simply
> > prevents trying to call modprobe for auto-loading within the kernel.
>
> I don't think this is false -- but yes, this only protects module
> auto-loading in user namespaces; all auto-loading calls within the
> kernel should be going through this __request_module() -> modprobe path.
>
> init_module()/finit_module(), the mechanism used by modprobe, are
> themselves already restricted inside user namespaces, see below.
The documentation should be clear about the exact nature of what
mechanism is prevented to load.
> > > + /*
> > > + * Disallow module loading if we're in a user namespace.
> > > + */
> > > + if (current_user_ns() != &init_user_ns &&
> > > + !sysctl_ns_modules_allowed) {
> > > + pr_warn_ratelimited("request_module: pid %d (%s) in user namespace requested kernel module %s; denied due to kernel.ns_modules_allowed sysctl\n",
> > > + task_pid_nr(current), current->comm, module_name);
> > > + return -EPERM;
> > > + }
> > > +
> > > if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
> > > pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s, throttling...",
> > > atomic_read(&kmod_concurrent_max),
> >
> > Have you seen what call_modprobe() does?
>
> Yes.
>
> > This is just a limitting the auto-loading through calling modprobe.
> > If the concern is to load modules wouldn't you be better off just
> > putting a stop gap at finit_module() which actually receives the
> > load attempt from modprobe? Ie, an evil namespace, if it has access
> > to /sbin/modprobe could simply just try calling /sbin/modprobe on its
> > own.
>
> No.
>
> Root inside a user namespace can't call finit_module() as it won't have
> the necessary capabilities in the init namespace, see may_init_module().
I think the documentation to this knob you are adding should explain
this as well to give a bette context as to why it is useful.
And if may_init_module() suffices, why not just add a may_init_module()
instead of your check? Why would we allow a successful call to modprobe
with your sysctl but end up having it ignored in the end by
finit_module()? If what is being allowed here is to overcome may_init_module()
CAP_SYS_MODULE check by using call_modprobe() on the user namespace
the commit log should just mention this, and mention that by design
user namespaces have never been intended to allow loading modules, even if
they somehow end up triggering auto-loading of modules via request_module().
If this is true then currently we are enabling auto-loading as a
mistake, and the sysctl is only valuable to prevent regressions with
existing behaviour which should have been disabled from the start.
Is there an example successful exploit which takes avantage of having
a user namespace auto-load a kernel module and that triggers a security
flaw? What's an example of one?
> > What this *could* do is race to force a failure on some other *real*
> > modprobe request we do wish to honor when the above kmod kmod_concurrent_max
> > is triggered.
>
> How? My new check is done before the kmod_concurrent_max check/critical
> section... the check doesn't cause any more modprobe requests to happen
> in the first place, the only thing it can do is make them exit early.
> There is no way my patch can make this worse.
You are missing my point. My point is that abuse in general over having
a user namespace calling modprobe could cause harm that it can possibly
Denial-of-Service valid requests.
If user namespaces should never have been allowed to even auto-load the
commit log should mention that.
> > So in terms of justification, this commit log needs a bit more work as I
> > just can't see how this alone is fixing any CVE.
>
> [...]
>
> > So let's take a step back and think this through. What exaclty and why
> > would this commit fix *any* security issue? Itemizing CVEs won't cut it.
>
> I can include my explanations above in the changelog if you think that
> will help.
Answering the above questions would help.
Luis
