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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87si3nxhd7.fsf@x220.int.ebiederm.org>
Date:	Mon, 30 Nov 2015 16:16:52 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Stephen Hemminger <stephen@...workplumber.org>
Cc:	David Miller <davem@...emloft.net>,
	Richard Weinberger <richard@....at>,
	"netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	"kernel-hardening\@lists.openwall.com" 
	<kernel-hardening@...ts.openwall.com>,
	bridge@...ts.linux-foundation.org,
	Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH net] bridge: Only call /sbin/bridge-stp for the initial network namespace

Stephen Hemminger <stephen@...workplumber.org> writes:

> On Mon, 30 Nov 2015 15:38:15 -0600
> ebiederm@...ssion.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@...ssion.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/