[PATCH net 0/1 v3] rtnetlink: require unique netns identifier

[PATCH net 0/1 v3] rtnetlink: require unique netns identifier

[Christian Brauner]

Hey,

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing and a potential security liability
given that pids might be recycled while the netlink request is served or
the process might do a setns() It also lets us indicate that network namespace
ids are the preferred way of interacting with network namespaces in rtnetlink
requests. The regression potential is quite minimal since the rtnetlink
requests in question either won't allow IFLA_IF_NETNSID requests before 4.16 is
out (RTM_{NEW,SET}LINK) or don't support IFLA_NET_NS_{PID,FD}
(RTM_{DEL,GET}LINK) in the first place.

We obviously cannot prevent users from passing both IFLA_NET_NS_PID and
IFLA_NET_NS_FD since we have supported this somehow for a long time for
a subset of rtnetlink requests. So the check I'm proposing is to only
fail when both IFLA_IF_NETNSID, and IFLA_NET_NS_PID or IFLA_NET_NS_FD are
passed.

Thanks!
Christian

---
ChangeLog v2->v3:
* Specifying target network namespaces with pids or fds seems racy since the
  process might die and the pid get recycled or the process does a setns() in
  which case the tests would be invalid. So only check whether multiple
  properties are specified and report a helpful error in this case.
ChangeLog v1->v2:
* return errno when the specified network namespace id is invalid
* fill in struct netlink_ext_ack if the network namespace id is invalid
* rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
  indicate that a request without any network namespace identifying attributes
  is also considered valid.
ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace
---

Christian Brauner (1):
  rtnetlink: require unique netns identifier

 net/core/rtnetlink.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

-- 
2.14.1

[PATCH net 1/1 v3] rtnetlink: require unique netns identifier

[Christian Brauner]

Since we've added support for IFLA_IF_NETNSID for RTM_{DEL,GET,SET,NEW}LINK
it is possible for userspace to send us requests with three different
properties to identify a target network namespace. This affects at least
RTM_{NEW,SET}LINK. Each of them could potentially refer to a different
network namespace which is confusing and a potential security liability
given that pids might be recycled while the netlink request is served or
the process might do a setns. It also lets us indicate that network
namespace ids are the preferred way of interacting with network namespaces
in rtnetlink requests. The regression potential is quite minimal since the
rtnetlink requests in question either won't allow IFLA_IF_NETNSID requests
before 4.16 is out (RTM_{NEW,SET}LINK) or don't support
IFLA_NET_NS_{PID,FD} (RTM_{DEL,GET}LINK) in the first place.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
ChangeLog v2->v3:
* Specifying target network namespaces with pids or fds seems racy since the
  process might die and the pid get recycled or the process does a setns() in
  which case the tests would be invalid. So only check whether multiple
  properties are specified and report a helpful error in this case.
ChangeLog v1->v2:
* return errno when the specified network namespace id is invalid
* fill in struct netlink_ext_ack if the network namespace id is invalid
* rename rtnl_ensure_unique_netns_attr() to rtnl_ensure_unique_netns() to
  indicate that a request without any network namespace identifying attributes
  is also considered valid.
ChangeLog v0->v1:
* report a descriptive error to userspace via struct netlink_ext_ack
* do not fail when multiple properties specifiy the same network namespace
---
 net/core/rtnetlink.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 56af8e41abfc..d7c3c8e266a3 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1951,6 +1951,28 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
 	return net;
 }
 
+/* Verify that rtnetlink requests supporting network namespace ids
+ * do not pass additional properties potentially referring to different
+ * network namespaces.
+ */
+static int rtnl_ensure_unique_netns(struct nlattr *tb[],
+				    struct netlink_ext_ack *extack)
+{
+	/* Requests without network namespace ids have been able to specify
+	 * multiple properties referring to different network namespaces so
+	 * don't regress them.
+	 */
+	if (!tb[IFLA_IF_NETNSID])
+		return 0;
+
+	/* Caller operates on the current network namespace. */
+	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+		return 0;
+
+	NL_SET_ERR_MSG(extack, "multiple netns identifying attributes specified");
+	return -EINVAL;
+}
+
 static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 {
 	if (dev) {
@@ -2553,6 +2575,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
+	err = rtnl_ensure_unique_netns(tb, extack);
+	if (err < 0)
+		goto errout;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -2649,6 +2675,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 
@@ -2802,6 +2832,10 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -3045,6 +3079,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IF_NETNSID]) {
 		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
 		tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);
-- 
2.14.1

Re: [PATCH net 1/1 v3] rtnetlink: require unique netns identifier

[Jiri Benc]

On Tue,  6 Feb 2018 14:19:02 +0100, Christian Brauner wrote:
> +/* Verify that rtnetlink requests supporting network namespace ids
> + * do not pass additional properties potentially referring to different
> + * network namespaces.
> + */
> +static int rtnl_ensure_unique_netns(struct nlattr *tb[],
> +				    struct netlink_ext_ack *extack)
> +{
> +	/* Requests without network namespace ids have been able to specify
> +	 * multiple properties referring to different network namespaces so
> +	 * don't regress them.
> +	 */
> +	if (!tb[IFLA_IF_NETNSID])
> +		return 0;

I agree with Eric that we should enforce this also for the existing
pid/fd attributes.

> +
> +	/* Caller operates on the current network namespace. */
> +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> +		return 0;
> +
> +	NL_SET_ERR_MSG(extack, "multiple netns identifying attributes specified");
> +	return -EINVAL;

But if we don't reach an agreement on that, this version is the next
best one. No reason to compare the namespaces whether they're the same,
a message with more than one such attribute is just invalid.

> @@ -2649,6 +2675,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err < 0)
>  		return err;
>  
> +	err = rtnl_ensure_unique_netns(tb, extack);
> +	if (err < 0)
> +		return err;
> +
>  	if (tb[IFLA_IFNAME])
>  		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
>  
> @@ -3045,6 +3079,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err < 0)
>  		return err;
>  
> +	err = rtnl_ensure_unique_netns(tb, extack);
> +	if (err < 0)
> +		return err;
> +
>  	if (tb[IFLA_IF_NETNSID]) {
>  		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
>  		tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);

dellink and getlink support only netnsid, we should just reject a
message with pid or fd set.

 Jiri

Re: [PATCH net 1/1 v3] rtnetlink: require unique netns identifier

[Christian Brauner]

On Wed, Feb 07, 2018 at 12:19:25PM +0100, Jiri Benc wrote:
> On Tue,  6 Feb 2018 14:19:02 +0100, Christian Brauner wrote:
> > +/* Verify that rtnetlink requests supporting network namespace ids
> > + * do not pass additional properties potentially referring to different
> > + * network namespaces.
> > + */
> > +static int rtnl_ensure_unique_netns(struct nlattr *tb[],
> > +				    struct netlink_ext_ack *extack)
> > +{
> > +	/* Requests without network namespace ids have been able to specify
> > +	 * multiple properties referring to different network namespaces so
> > +	 * don't regress them.
> > +	 */
> > +	if (!tb[IFLA_IF_NETNSID])
> > +		return 0;
> 
> I agree with Eric that we should enforce this also for the existing
> pid/fd attributes.

Yes, I would prefer this too but in the Linux spirit of never regressing
userspace I was afraid that there might already be userspace
applications that stick a pid and an fd at the same time into an
rtnetlink request. If we are ok with potentially breaking them then we
should just go for it. It is definitely the cleaner solution.

> 
> > +
> > +	/* Caller operates on the current network namespace. */
> > +	if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> > +		return 0;
> > +
> > +	NL_SET_ERR_MSG(extack, "multiple netns identifying attributes specified");
> > +	return -EINVAL;
> 
> But if we don't reach an agreement on that, this version is the next
> best one. No reason to compare the namespaces whether they're the same,
> a message with more than one such attribute is just invalid.
> 
> > @@ -2649,6 +2675,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	err = rtnl_ensure_unique_netns(tb, extack);
> > +	if (err < 0)
> > +		return err;
> > +
> >  	if (tb[IFLA_IFNAME])
> >  		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
> >  
> > @@ -3045,6 +3079,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	err = rtnl_ensure_unique_netns(tb, extack);
> > +	if (err < 0)
> > +		return err;
> > +
> >  	if (tb[IFLA_IF_NETNSID]) {
> >  		netnsid = nla_get_s32(tb[IFLA_IF_NETNSID]);
> >  		tgt_net = get_target_net(NETLINK_CB(skb).sk, netnsid);
> 
> dellink and getlink support only netnsid, we should just reject a
> message with pid or fd set.

Thanks for the feedback, I'll adapt the patch with the requested
changes.

Re: [PATCH net 1/1 v3] rtnetlink: require unique netns identifier

[Eric W. Biederman]

Christian Brauner <christian.brauner@canonical.com> writes:

> On Wed, Feb 07, 2018 at 12:19:25PM +0100, Jiri Benc wrote:
>> On Tue,  6 Feb 2018 14:19:02 +0100, Christian Brauner wrote:
>> > +/* Verify that rtnetlink requests supporting network namespace ids
>> > + * do not pass additional properties potentially referring to different
>> > + * network namespaces.
>> > + */
>> > +static int rtnl_ensure_unique_netns(struct nlattr *tb[],
>> > +				    struct netlink_ext_ack *extack)
>> > +{
>> > +	/* Requests without network namespace ids have been able to specify
>> > +	 * multiple properties referring to different network namespaces so
>> > +	 * don't regress them.
>> > +	 */
>> > +	if (!tb[IFLA_IF_NETNSID])
>> > +		return 0;
>> 
>> I agree with Eric that we should enforce this also for the existing
>> pid/fd attributes.
>
> Yes, I would prefer this too but in the Linux spirit of never regressing
> userspace I was afraid that there might already be userspace
> applications that stick a pid and an fd at the same time into an
> rtnetlink request. If we are ok with potentially breaking them then we
> should just go for it. It is definitely the cleaner solution.

Odds are low that anything does anything so silly.  If we accidentally
cause a regression then we fix it.  Unless you have reason to suspect
someone actually does something silly we should be good.

Eric