| Message ID | 87egf7183c.fsf_-_@x220.int.ebiederm.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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);
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(-)