Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

Greg Kroah-Hartman [off-list ref] writes:

On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:

quoted

From: Mahesh Bandewar <redacted>

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?

quoted

Signed-off-by: Mahesh Bandewar <redacted>
---
 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();

On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:

I don't object to this if the networking developers don't mind the
change in functionality.  They can handle the fallout :)

thanks,

greg k-h

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Mon, 15 May 2017 08:10:59 +0200

On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:

quoted

Greg Kroah-Hartman [off-list ref] 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.

On Mon, May 15, 2017 at 6:52 AM, David Miller [off-list ref] wrote:

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Mon, 15 May 2017 08:10:59 +0200

quoted

On Sun, May 14, 2017 at 08:57:34AM -0500, Eric W. Biederman wrote:

quoted

Greg Kroah-Hartman [off-list ref] 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.

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);
                                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.

From: Mahesh Bandewar (महेश बंडेवार) <redacted>
Date: Mon, 15 May 2017 10:59:55 -0700

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.

Yes, that is certainly undesirable.

But is it really a module loading problem?  Perhaps we need to look
more deeply into how conntract behaves by default wrt. namespaces.

If we've given the user the ability to be root in his or her own
namespace, then we should let them do root stuff in there.

The only problem is when "doing root stuff in there" has an
undesirable impact upon the rest of the system.

And that's needs to be looked into on a facility by facility basis,
rather then just sprinkling "no module loading" test here and there,
or even unconditionally.