netdev - Re: [PATCH] kmod: don't load module unless req process has CAP_SYS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d1bbo81d.fsf@xmission.com>
Date:   Sun, 14 May 2017 08:57:34 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Mahesh Bandewar <mahesh@...dewar.net>,
        Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Mahesh Bandewar <maheshb@...gle.com>
Subject: Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

Greg Kroah-Hartman <gregkh@...uxfoundation.org> writes:

> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>> From: Mahesh Bandewar <maheshb@...gle.com>
>> 
>> A process inside random user-ns should not load a module, which is
>> currently possible. As demonstrated in following scenario -
>> 
>>   Create namespaces; especially a user-ns and become root inside.
>>   $ unshare -rfUp -- unshare -unm -- bash
>> 
>>   Try to load the bridge module. It should fail and this is expected!
>>   #  modprobe bridge
>>   WARNING: Error inserting stp (/lib/modules/4.11.0-smp-DEV/kernel/net/802/stp.ko): Operation not permitted
>>   FATAL: Error inserting bridge (/lib/modules/4.11.0-smp-DEV/kernel/net/bridge/bridge.ko): Operation not permitted
>> 
>>   Verify bridge module is not loaded.
>>   # lsmod | grep bridge
>>   #
>> 
>>   Now try to create a bridge inside this newly created net-ns which would
>>   mean bridge module need to be loaded.
>>   # ip link add br0 type bridge
>>   # echo $?
>>   0
>>   # lsmod | grep bridge
>>   bridge                110592  0
>>   stp                    16384  1 bridge
>>   llc                    16384  2 bridge,stp
>>   #
>> 
>>   After this patch -
>>   # ip link add br0 type bridge
>>   RTNETLINK answers: Operation not supported
>>   # echo $?
>>   2
>>   # lsmod | grep bridge
>>   #
>
> Well, it only loads this because the kernel asked for it to be loaded,
> right?
>
>> 
>> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>> ---
>>  kernel/kmod.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 563f97e2be36..ac30157169b7 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...)
>>  #define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
>>  	static int kmod_loop_msg;
>>  
>> +	if (!capable(CAP_SYS_MODULE))
>> +		return -EPERM;
>
> At first glance this looks right, but I'm worried what this will break
> that currently relies on this.  There might be lots of systems that are
> used to this being the method that the needed module is requested.  What
> about when userspace asks for a random char device and that module is
> then loaded?  Does this patch break that functionality?

For the specific example give I think we would be better served by
adding a capability check at the call site.  In this case CAP_NET_ADMIN
as those are the capabilities iproute traditionally has.

We have something similar in dev_load in already in the networking code.

This limits the people who can't load modules to root user in user
namespaces.  I would be fine with any other code paths in a user
namespace getting a similar treatment.

Eric


diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index bcb0f610ee42..6b72528a4636 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2595,7 +2595,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 
                if (!ops) {
 #ifdef CONFIG_MODULES
-                       if (kind[0]) {
+                       if (kind[0] && capable(CAP_NET_ADMIN)) {
                                __rtnl_unlock();
                                request_module("rtnl-link-%s", kind);
                                rtnl_lock();