user controllable usermodehelper in br_stp_if.c

user controllable usermodehelper in br_stp_if.c

[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.

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

Re: user controllable usermodehelper in br_stp_if.c

[Kees Cook]

On Sun, Nov 29, 2015 at 2:43 PM, Richard Weinberger <richard@nod.at> wrote:
> 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?

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

-- 
Kees Cook
Chrome OS & Brillo Security

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

[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" <ebiederm@xmission.com>
---
 net/bridge/br_stp_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 5396ff08af32..742fa89528ab 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -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);
 
-- 
2.2.1

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

[Stephen Hemminger]

On Mon, 30 Nov 2015 15:38:15 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> 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" <ebiederm@xmission.com>
> ---
>  net/bridge/br_stp_if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 5396ff08af32..742fa89528ab 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -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?

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

[Eric W. Biederman]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Mon, 30 Nov 2015 15:38:15 -0600
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> 
>> 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" <ebiederm@xmission.com>
>> ---
>>  net/bridge/br_stp_if.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
>> index 5396ff08af32..742fa89528ab 100644
>> --- a/net/bridge/br_stp_if.c
>> +++ b/net/bridge/br_stp_if.c
>> @@ -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.

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.

> 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

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

[Richard Weinberger]

Am 30.11.2015 um 22:38 schrieb 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" <ebiederm@xmission.com>

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

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

[Hannes Frederic Sowa]

On Mon, Nov 30, 2015, at 22:38, Eric W. Biederman wrote:
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>  net/bridge/br_stp_if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 5396ff08af32..742fa89528ab 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -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 ?

> +               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

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

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 30 Nov 2015 15:38:15 -0600

> +	if (dev_net(br->dev) == &init_net)

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

Thanks.

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

[Kees Cook]

On Wed, Dec 2, 2015 at 8:50 PM, David Miller <davem@davemloft.net> wrote:
> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Mon, 30 Nov 2015 15:38:15 -0600
>
>> +     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

-- 
Kees Cook
Chrome OS & Brillo Security

Re: user controllable usermodehelper in br_stp_if.c

[Richard Weinberger]

Am 30.11.2015 um 21:14 schrieb Kees Cook:
> On Sun, Nov 29, 2015 at 2:43 PM, Richard Weinberger <richard@nod.at> wrote:
>> 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?
> 
> 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

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

[Hannes Frederic Sowa]

[I stole this patch from Eric Biederman. He wrote:]

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

[Hannes: changed patch using netns_eq]

Cc: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/bridge/br_stp_if.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 905cedd37a0539..a31ac6ad76a223 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -143,7 +143,10 @@ 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);
+	if (net_eq(dev_net(br->dev), &init_net))
+		r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
+	else
+		r = -ENOENT;
 
 	spin_lock_bh(&br->lock);
 
-- 
2.5.0