On Tue, Nov 28, 2017 at 11:18 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> On Tue, Nov 28, 2017 at 10:33:27PM +0100, Djalal Harouni wrote:
>> On Tue, Nov 28, 2017 at 10:16 PM, Luis R. Rodriguez <mcg...@kernel.org>
>> wrote:
>> > On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote:
>> >> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcg...@kernel.org>
>> >> wrote:
>> >> > kmod is just a helper to poke userpsace to load a module, that's it.
>> >> >
>> >> > The old init_module() and newer finit_module() do the real handy work or
>> >> > module loading, and both currently only use may_init_module():
>> >> >
>> >> > static int may_init_module(void)
>> >> > {
>> >> > if (!capable(CAP_SYS_MODULE) || modules_disabled)
>> >> > return -EPERM;
>> >> >
>> >> > return 0;
>> >> > }
>> >> >
>> >> > This begs the question:
>> >> >
>> >> > o If userspace just tries to just use raw finit_module() do we want
>> >> > similar
>> >> > checks?
>> >> >
>> >> > Otherwise, correct me if I'm wrong this all seems pointless.
>> >>
>> >> Hm? That's direct-loading, not auto-loading. This series is only about
>> >> auto-loading.
>> >
>> > And *all* auto-loading uses aliases? What's the difference between
>> > auto-loading
>> > and direct-loading?
>>
>> Not all auto-loading uses aliases, auto-loading is when kernel code
>> calls request_module() to loads the feature that was not present,
>
> It seems the actual interest here is system call implicated request_module()
> calls? Because there are uses of request_module() which may be module hacks,
> and not implicated via system calls.
Indeed.
>> and direct-loading in this thread is the direct syscalls like
>> finit_module().
>
> OK.
>
>> >> We already have a global sysctl for blocking direct-loading
>> >> (modules_disabled).
>> >
>> > My point was that even if you have a CAP_NET_ADMIN check on
>> > request_module(),
>> > finit_module() will not check for it, so a crafty userspace could still try
>> > to just finit_module() directly, and completely then bypass the
>> > CAP_NET_ADMIN
>> > check.
>>
>> The finit_module() uses CAP_SYS_MODULE which should allow all modules
>> and in this context it should be more privileged than CAP_NET_ADMIN
>> which is only for "netdev-%s" (to not load arbitrary modules with it).
>>
>> finit_module() coming from request_module() always has the
>> CAP_NET_ADMIN, hence the check is done before.
>
> But since CAP_SYS_MODULE is more restrictive, what's the point in checking
> for CAP_NET_ADMIN?
For backward compatibility with 'netdev' modules since it is for those.
>> > So unless I'm missing something, I see no point in adding extra checks for
>> > request_module() but nothing for the respective load_module().
>>
>> I see, request_module() is called from kernel context which runs in
>> init namespace will full capabilities, the spawned userspace modprobe
>> will get CAP_SYS_MODULE and all other caps, then after comes modprobe
>> and load_module().
>
> Right, so defining the gains of adding this extra check is not very clear
> yet. It would seem a benefit exists, what is it?
it will able to filter if the request_module() should continue loading
the module or deny it which prevents spawning the *privileged*
usermode helper. This is all based on are we allowed to load new
features or not, or IOW I don't want to allow new features or modules
autoloading from now and on, as stated in the cover letter for various
benefit including security, reduce the amount of kernel code running,
but also do not allow new features for anyone like tunneling, etc.
>> Btw as suggested by Linus I will update with request_module_cap() and > I can
>> offer my help maintaining these bits too.
>
> Can you start by extending lib/test_module.c and
> tools/testing/selftests/kmod/kmod.sh with a proof of concept of the gains
> here,
> as well as ensuring things work as expected ?
Alright Luis, thanks for the hint, yes I will make sure to cover these.
For gains, kees already answered in the other email, and please check
the DCCP exploit and others linked in the cover letter.
Thank you!
> Luis
--
tixxdz
