From patchwork Mon May 15 17:59:55 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= X-Patchwork-Id: 762668 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3wRSzd5hX5z9s65 for ; Tue, 16 May 2017 04:00:29 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="ZbU1IyBu"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934595AbdEOSAT (ORCPT ); Mon, 15 May 2017 14:00:19 -0400 Received: from mail-it0-f53.google.com ([209.85.214.53]:38266 "EHLO mail-it0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933985AbdEOSAR (ORCPT ); Mon, 15 May 2017 14:00:17 -0400 Received: by mail-it0-f53.google.com with SMTP id e65so73576137ita.1 for ; Mon, 15 May 2017 11:00:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=RM1bQQxWPlNhszEEQDupDTD0PoQMXoWhJUyW91xAq6Q=; b=ZbU1IyBui22uwEL1yk4L4FSd+nuZl5TEMyn0zhH/hDn1TlylVdVcJIDom/QYtfRCd2 owJC3EW/k93SQd7z0TNluhh9M02o8CBeyJGNzJlEqj+DbC5gs+wDUynhnvNOMDaqK3SZ VlC3RxYUO26L/inBqz7onNKnNS2gnhRc+uoW/aljWOTJaFAsb6lLEugFFh9NRKXuElyb VCfuAUlvae/6Hh+SHiMEBXvs9Zcq/WvMSEAY849x/QogNeuX3yiZB4Dv6CMkUzPLvve0 cvakAnc8D95iQzvQIVFcj090/++4nNg+TxP+LTCGzKntctAlRmXXG7eT01NLLnYtW9qJ 9xDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=RM1bQQxWPlNhszEEQDupDTD0PoQMXoWhJUyW91xAq6Q=; b=V3JC1EDpFboQ0sUlrZ2c3A6Le5e+De2FulS8nI+XYku7xotm7MxP/dNOd15HJIcmZr fe4Vw9MsSKftfziurACi7wuR6eJwEcqV8HsNYFkpyO/kYFTpmfoyo3Fqkagk1bgkoXtC l2IxQHL+FamMJb+7A62IpXL2DaeVAyXjMCQWHd9hMBEIQA3hBXBUYPUTAcsxjlsUo3Vy BPEjnhjWsbGBVSvYRqv+3PDcL92VWKpd/zBlrD3jbISqlUze9sCJWFNE6gg4lUL6m8wk J2vn/NcZX21aynJN62386SiqpkF2TOHn/OHjXPTLT6zmmbJ8wUZEhxLcz3YQxZWCp4LZ SCnQ== X-Gm-Message-State: AODbwcAkS9vGf1IEmoQiLQtSsir/1jMJ33YbCud2JVo97TXRXZkxBNfG hi5d7GmeKjt3uW/tn0Kq3f626ah4Nn30 X-Received: by 10.36.249.72 with SMTP id l69mr6622008ith.119.1494871215941; Mon, 15 May 2017 11:00:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.138.144 with HTTP; Mon, 15 May 2017 10:59:55 -0700 (PDT) In-Reply-To: <20170515.095228.1483686375235860235.davem@davemloft.net> References: <20170514104537.GA29323@kroah.com> <87d1bbo81d.fsf@xmission.com> <20170515061059.GB28741@kroah.com> <20170515.095228.1483686375235860235.davem@davemloft.net> From: =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= Date: Mon, 15 May 2017 10:59:55 -0700 Message-ID: Subject: Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE To: David Miller Cc: gregkh@linuxfoundation.org, ebiederm@xmission.com, mahesh@bandewar.net, mingo@kernel.org, linux-kernel@vger.kernel.org, linux-netdev , keescook@chromium.org, Eric Dumazet Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, May 15, 2017 at 6:52 AM, David Miller wrote: > From: Greg Kroah-Hartman > Date: Mon, 15 May 2017 08:10:59 +0200 > >> On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote: >>> Greg Kroah-Hartman writes: >>> >>> 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(); >> >> I don't object to this if the networking developers don't mind the >> change in functionality. They can handle the fallout :) > > As I've said in another email, I am pretty sure this can break things. The current behavior is already breaking things. e.g. unprivileged process can be root inside it's own user-ns. This will allow it to create IPtable rules causing contracking module to be loaded in default-ns affecting every flow on the server (not just the namespace that user or an unprivileged process is attached to). Cases that I mentioned above are just the tip of an iceberg. In a non-namespace world this wouldn't happen as capability checks are performed correctly but the moment an unprivileged user can create it's own user-ns and becomes root inside, it could make use of these things and perform privileged operations in default-ns. So to protect "global namespace" from making such things happen, we have to protect using global capability check. Alternatively we can preserve the existing behavior by adding this check for non-default-user-ns only. e.g. rtnl_lock(); if we have to do this in net-subsystem then it's not just this call site and there are lot more. But if this is an acceptable alternative, I can think of better implementation for all those sites. diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 6e67315ec368..263f0d175091 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2595,7 +2595,9 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, if (!ops) { #ifdef CONFIG_MODULES - if (kind[0]) { + if (kind[0] && + ((net->user_ns == &init_user_ns) || + capable(CAP_SYS_MODULE))) { __rtnl_unlock(); request_module("rtnl-link-%s", kind);