From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754961AbdENOEJ (ORCPT ); Sun, 14 May 2017 10:04:09 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:41947 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751676AbdENOEH (ORCPT ); Sun, 14 May 2017 10:04:07 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Greg Kroah-Hartman Cc: Mahesh Bandewar , Ingo Molnar , LKML , netdev , Kees Cook , David Miller , Eric Dumazet , Mahesh Bandewar References: <20170512232259.10820-1-mahesh@bandewar.net> <20170514104537.GA29323@kroah.com> Date: Sun, 14 May 2017 08:57:34 -0500 In-Reply-To: <20170514104537.GA29323@kroah.com> (Greg Kroah-Hartman's message of "Sun, 14 May 2017 12:45:37 +0200") Message-ID: <87d1bbo81d.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1d9u7q-00063g-74;;;mid=<87d1bbo81d.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18zcRik84AlAN5GrDBaTg7uluCTPi3H7iw= X-SA-Exim-Connect-IP: 97.121.81.159 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.4998] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa07 1397; Body=1 Fuz1=1 Fuz2=1] * 1.0 T_XMDrugObfuBody_08 obfuscated drug references * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa07 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: *;Greg Kroah-Hartman X-Spam-Relay-Country: X-Spam-Timing: total 5302 ms - load_scoreonly_sql: 0.05 (0.0%), signal_user_changed: 3.0 (0.1%), b_tie_ro: 2.0 (0.0%), parse: 0.84 (0.0%), extract_message_metadata: 12 (0.2%), get_uri_detail_list: 1.99 (0.0%), tests_pri_-1000: 4.9 (0.1%), tests_pri_-950: 1.48 (0.0%), tests_pri_-900: 1.05 (0.0%), tests_pri_-400: 26 (0.5%), check_bayes: 25 (0.5%), b_tokenize: 9 (0.2%), b_tok_get_all: 9 (0.2%), b_comp_prob: 2.7 (0.1%), b_tok_touch_all: 2.9 (0.1%), b_finish: 0.66 (0.0%), tests_pri_0: 420 (7.9%), check_dkim_signature: 0.60 (0.0%), check_dkim_adsp: 3.2 (0.1%), tests_pri_500: 4829 (91.1%), poll_dns_idle: 4824 (91.0%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Greg Kroah-Hartman writes: > On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote: >> From: Mahesh Bandewar >> >> 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 >> --- >> 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();