user controllable usermodehelper in br_stp_if.c

PATCH In review Richard Weinberger 6 participants 10 messages Nov 29, 2015 – Dec 8, 2015

Subsystems (3)

Networking Network drivers Ethernet bridge

lore

Share & tools

lore

Richard Weinberger

author

2015-11-29 22:43 UTC #0 lore raw

Hi!

By spawning new network and user namesapces an unprivileged user
is able to execute /sbin/bridge-stp within the initial mount namespace with global root rights.
While this cannot directly be used to break out of a container or gain global root rights it could be used by exploit writers as valuable building block.

e.g.

$ unshare -U -r -n /bin/sh
$ brctl addbr br0
$ brctl stp br0 on # this will execute /sbin/bridge-stp

As this mechanism clearly cannot work with containers and seems to be legacy code I suggest not calling call_usermodehelper() at all if we're not in the initial user namespace. What do you think?

Thanks,
//richard

Kees Cook

2015-11-30 20:14 UTC #1 lore raw

Quoted 8 lines · Richard Weinberger

Hi!
By spawning new network and user namesapces an unprivileged user
is able to execute /sbin/bridge-stp within the initial mount namespace with global root rights.
While this cannot directly be used to break out of a container or gain global root rights it could be used by exploit writers as valuable building block.

Quoted 8 lines

e.g.
$ unshare -U -r -n /bin/sh
$ brctl addbr br0
$ brctl stp br0 on # this will execute /sbin/bridge-stp
As this mechanism clearly cannot work with containers and seems to be legacy code I suggest not calling call_usermodehelper() at all if we're not in the initial user namespace. What do you think?

I'm not familiar with how bridge-stp is expected to operate with a network namespace, but if it's meaningless, then yeah, that seems like a reasonable change. Can you send a patch? (Also, if it's legacy code, maybe it could be turned off entirely, not just for containers?)

-Kees

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security

Eric W. Biederman

2015-11-30 21:38 UTC #2 lore raw

PATCH 1/1 bridge: Only call /sbin/bridge-stp for the initial network namespace

There is no defined mechanism to pass network namespace information into /sbin/bridge-stp therefore don't even try to invoke it except for bridge devices in the initial network namespace.

It is possible for unprivileged users to cause /sbin/bridge-stp to be invoked for any network device name which if /sbin/bridge-stp does not guard against unreasonable arguments or being invoked twice on the same network device could cause problems.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 net/bridge/br_stp_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

net/bridge/br_stp_if.c +3 -1

index 5396ff08af32..742fa89528ab 100644
@@ -142,7 +142,9 @@ static void br_stp_start(struct net_bridge *br)
 	char *envp[] = { NULL };
 	struct net_bridge_port *p;
 
-	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+	r = -ENOENT;
+	if (dev_net(br->dev) == &init_net)
+		r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
 
 	spin_lock_bh(&br->lock);

Signature · 1 line

2.2.1

Stephen Hemminger

2015-11-30 22:12 UTC #3 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

Quoted 20 lines · On Mon, 30 Nov 2015 15:38:15 -0600 [email protected] (Eric W. Biederman)…

There is no defined mechanism to pass network namespace information into /sbin/bridge-stp therefore don't even try to invoke it except for bridge devices in the initial network namespace.
It is possible for unprivileged users to cause /sbin/bridge-stp to be invoked for any network device name which if /sbin/bridge-stp does not guard against unreasonable arguments or being invoked twice on the same network device could cause problems.
Signed-off-by "Eric W. Biederman" <[email protected]>
---
 net/bridge/br_stp_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/bridge/br_stp_if.c +0 -0
index 5396ff08af32..742fa89528ab 100644

net/bridge/br_stp_if.c static void br_stp_start(struct net_bridge *br) · line 147

Quoted 7 lines

net/bridge/br_stp_if.c +3 -1

@@ -142,7 +142,9 @@ static void br_stp_start(struct net_bridge *br)
 	char *envp[] = { NULL };
 	struct net_bridge_port *p;
 
-	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+	r = -ENOENT;
+	if (dev_net(br->dev) == &init_net)
+		r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);

I don't think this will cause loud screams.
But it might break people that use containers to run virtual networks for testing.

One coding nit:
Why are you afraid of using an else?

Eric W. Biederman

2015-11-30 22:16 UTC #4 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

Quoted 26 lines · Stephen Hemminger

Quoted 23 lines · On Mon, 30 Nov 2015 15:38:15 -0600 [email protected] (Eric W. Biederman)…
There is no defined mechanism to pass network namespace information into /sbin/bridge-stp therefore don't even try to invoke it except for bridge devices in the initial network namespace.
It is possible for unprivileged users to cause /sbin/bridge-stp to be invoked for any network device name which if /sbin/bridge-stp does not guard against unreasonable arguments or being invoked twice on the same network device could cause problems.
Signed-off-by "Eric W. Biederman" <[email protected]>
---
 net/bridge/br_stp_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/bridge/br_stp_if.c +0 -0
index 5396ff08af32..742fa89528ab 100644
@@ -142,7 +142,9 @@ static void br_stp_start(struct net_bridge *br)
 	char *envp[] = { NULL };
 	struct net_bridge_port *p;
 

Quoted 8 lines

net/bridge/br_stp_if.c static void br_stp_start(struct net_bridge *br) · line 147
Quoted 4 lines
net/bridge/br_stp_if.c +3 -1
-	r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+	r = -ENOENT;
+	if (dev_net(br->dev) == &init_net)
+		r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
I don't think this will cause loud screams.
But it might break people that use containers to run virtual networks for testing.

I don't see how this interface can possibly be for more than the initial network namespace. There is no network namespace information conveyed and /sbin/bridge-stp always runs in the initial network namespace.

Which is the point of this patch. Don't try when the code can not work.

The only way this code could possibly work in the presence of multiple network namespaces is if somehow the network namespace was encoded in the device name, and then the usermode helper switched to the appropriate network namespace.

I suspect that anyone knowledgable enough to know this interface exists would have sent a patch to fix the kernel to give network namespace information rather than use a horrible userspace hack like encoding the network namespace in the device name.

Quoted 2 lines

One coding nit:
Why are you afraid of using an else?

Branch stalls. Plus in this case an else is more lines and just plain uglier code.

Eric

Richard Weinberger

author

2015-11-30 22:57 UTC #5 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

In reply to Eric W. Biederman

Am 30.11.2015 um 22:38 schrieb Eric W. Biederman:

Quoted 11 lines

There is no defined mechanism to pass network namespace information into /sbin/bridge-stp therefore don't even try to invoke it except for bridge devices in the initial network namespace.
It is possible for unprivileged users to cause /sbin/bridge-stp to be invoked for any network device name which if /sbin/bridge-stp does not guard against unreasonable arguments or being invoked twice on the same network device could cause problems.
Signed-off-by "Eric W. Biederman" <[email protected]>

Just figured that /sbin/bridge-stp is a shell script. Network interfaces can contain a lot of funny characters, maybe this is after all a security issue.

Thanks,
//richard

Richard Weinberger

author

2015-11-30 23:03 UTC #9 lore raw

In reply to Kees Cook

Am 30.11.2015 um 21:14 schrieb Kees Cook:

Quoted 14 lines · Richard Weinberger

Quoted 13 lines
Hi!
By spawning new network and user namesapces an unprivileged user
is able to execute /sbin/bridge-stp within the initial mount namespace with global root rights.
While this cannot directly be used to break out of a container or gain global root rights it could be used by exploit writers as valuable building block.
e.g.
$ unshare -U -r -n /bin/sh
$ brctl addbr br0
$ brctl stp br0 on # this will execute /sbin/bridge-stp

Quoted 8 lines

Quoted 3 lines
As this mechanism clearly cannot work with containers and seems to be legacy code I suggest not calling call_usermodehelper() at all if we're not in the initial user namespace. What do you think?
I'm not familiar with how bridge-stp is expected to operate with a network namespace, but if it's meaningless, then yeah, that seems like a reasonable change. Can you send a patch? (Also, if it's legacy code, maybe it could be turned off entirely, not just for containers?)

Eric was faster than me. :-)

BTW: kernel.core_pattern is also worth a look.
If the pipe mode is used, "|/bin/core_tool", it will be executed in the initial namespace and any user/container can trigger it. Shayan reported that some weeks ago: https://lkml.org/lkml/2015/10/24/134

Thanks,
//richard

Hannes Frederic Sowa

2015-12-01 14:13 UTC #6 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

In reply to Eric W. Biederman

Quoted 10 lines · Eric W. Biederman

Signed-off-by "Eric W. Biederman" <[email protected]>
---
 net/bridge/br_stp_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/bridge/br_stp_if.c +0 -0
index 5396ff08af32..742fa89528ab 100644

net/bridge/br_stp_if.c static void br_stp_start(struct net_bridge *br) · line 146

Quoted 6 lines

net/bridge/br_stp_if.c +2 -1

@@ -142,7 +142,9 @@ static void br_stp_start(struct net_bridge *br)
 	char *envp[] = { NULL };
 	struct net_bridge_port *p;
 
-       r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+       r = -ENOENT;
+       if (dev_net(br->dev) == &init_net)

net_eq ?

net/bridge/br_stp_if.c static void br_stp_start(struct net_bridge *br) · line 149

Quoted 5 lines

net/bridge/br_stp_if.c +1 -0

+               r = call_usermodehelper(BR_STP_PROG, argv, envp,
 UMH_WAIT_PROC);
 
 	spin_lock_bh(&br->lock);

Otherwise, ack, so far.

As our /sys interfaces directories are tagged by the net namespace it would actually make sense to run bridge-stp automatically in another name space.

Bye,
Hannes

David Miller

maintainer

2015-12-03 04:50 UTC #7 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

In reply to Eric W. Biederman

From: [email protected] (Eric W. Biederman)
Date: Mon, 30 Nov 2015 15:38:15 -0600

net/bridge/br_stp_if.c static void br_stp_start(struct net_bridge *br) · line 146

Quoted 1 line

net/bridge/br_stp_if.c +1 -0
+	if (dev_net(br->dev) == &init_net)

Please respin this using net_eq() as Hannes pointed out.

Thanks.

Kees Cook

2015-12-08 20:56 UTC #8 lore raw

Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

Quoted 6 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Mon, 30 Nov 2015 15:38:15 -0600
Quoted 1 line
+ if (dev_net(br->dev) == &init_net)
Please respin this using net_eq() as Hannes pointed out.

Sorry if I missed it: this this happen yet?

-Kees

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security