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

[PATCH net 0/1 v4] 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.

Thanks!
Christian

---
ChangeLog v3->v4:
* Based on discussions with Eric and Jiri: disallow passing multiple network
  namespace identifying properties for all requests, i.e. always enforce
  uniqueness.
* disable passing IFLA_NET_NS_{FD,PID} for RTM_{DEL,GET}LINK completely since
  they never supported it
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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

-- 
2.14.1

[PATCH net 1/1 v4] 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. For legacy reasons the kernel will
pick the IFLA_NET_NS_PID property first and then look for the
IFLA_NET_NS_FD property but there is no reason to extend this type of
behavior to network namespace ids. 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 v3->v4:
* Based on discussions with Eric and Jiri: disallow passing multiple network
  namespace identifying properties for all requests, i.e. always enforce
  uniqueness.
* disable passing IFLA_NET_NS_{FD,PID} for RTM_{DEL,GET}LINK completely since
  they never supported it
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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 56af8e41abfc..bc290413a49d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1951,6 +1951,38 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
 	return net;
 }
 
+/* Verify that rtnetlink requests 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,
+				    bool netns_id_only)
+{
+
+	if (netns_id_only) {
+		if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
+			return 0;
+
+		NL_SET_ERR_MSG(extack, "specified netns attribute not supported");
+		return -EOPNOTSUPP;
+	}
+
+	if (tb[IFLA_IF_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]))
+		goto invalid_attr;
+
+	if (tb[IFLA_NET_NS_PID] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_FD]))
+		goto invalid_attr;
+
+	if (tb[IFLA_NET_NS_FD] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_PID]))
+		goto invalid_attr;
+
+	return 0;
+
+invalid_attr:
+	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 +2585,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		goto errout;
 
+	err = rtnl_ensure_unique_netns(tb, extack, false);
+	if (err < 0)
+		goto errout;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -2649,6 +2685,10 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack, true);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 
@@ -2802,6 +2842,10 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack, false);
+	if (err < 0)
+		return err;
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
@@ -3045,6 +3089,10 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err < 0)
 		return err;
 
+	err = rtnl_ensure_unique_netns(tb, extack, true);
+	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 v4] rtnetlink: require unique netns identifier

[Kirill Tkhai]

On 07.02.2018 15:53, Christian Brauner wrote:
> 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. For legacy reasons the kernel will
> pick the IFLA_NET_NS_PID property first and then look for the
> IFLA_NET_NS_FD property but there is no reason to extend this type of
> behavior to network namespace ids. 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 v3->v4:
> * Based on discussions with Eric and Jiri: disallow passing multiple network
>   namespace identifying properties for all requests, i.e. always enforce
>   uniqueness.
> * disable passing IFLA_NET_NS_{FD,PID} for RTM_{DEL,GET}LINK completely since
>   they never supported it
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 56af8e41abfc..bc290413a49d 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1951,6 +1951,38 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
>  	return net;
>  }
>  
> +/* Verify that rtnetlink requests 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,
> +				    bool netns_id_only)
> +{
> +
> +	if (netns_id_only) {
> +		if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> +			return 0;
> +
> +		NL_SET_ERR_MSG(extack, "specified netns attribute not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (tb[IFLA_IF_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]))
> +		goto invalid_attr;
> +
> +	if (tb[IFLA_NET_NS_PID] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_FD]))
> +		goto invalid_attr;
> +
> +	if (tb[IFLA_NET_NS_FD] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_PID]))
> +		goto invalid_attr;

Can't we write these 3 above branches more compact? Something like this:

	if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1)
		return 0;

Also, do we really need two different error values and error messages?

Kirill

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

[Christian Brauner]

On Wed, Feb 07, 2018 at 04:20:01PM +0300, Kirill Tkhai wrote:
> 
> 
> On 07.02.2018 15:53, Christian Brauner wrote:
> > 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. For legacy reasons the kernel will
> > pick the IFLA_NET_NS_PID property first and then look for the
> > IFLA_NET_NS_FD property but there is no reason to extend this type of
> > behavior to network namespace ids. 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 v3->v4:
> > * Based on discussions with Eric and Jiri: disallow passing multiple network
> >   namespace identifying properties for all requests, i.e. always enforce
> >   uniqueness.
> > * disable passing IFLA_NET_NS_{FD,PID} for RTM_{DEL,GET}LINK completely since
> >   they never supported it
> > 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index 56af8e41abfc..bc290413a49d 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1951,6 +1951,38 @@ static struct net *rtnl_link_get_net_capable(const struct sk_buff *skb,
> >  	return net;
> >  }
> >  
> > +/* Verify that rtnetlink requests 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,
> > +				    bool netns_id_only)
> > +{
> > +
> > +	if (netns_id_only) {
> > +		if (!tb[IFLA_NET_NS_PID] && !tb[IFLA_NET_NS_FD])
> > +			return 0;
> > +
> > +		NL_SET_ERR_MSG(extack, "specified netns attribute not supported");
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	if (tb[IFLA_IF_NETNSID] && (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]))
> > +		goto invalid_attr;
> > +
> > +	if (tb[IFLA_NET_NS_PID] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_FD]))
> > +		goto invalid_attr;
> > +
> > +	if (tb[IFLA_NET_NS_FD] && (tb[IFLA_IF_NETNSID] || tb[IFLA_NET_NS_PID]))
> > +		goto invalid_attr;
> 
> Can't we write these 3 above branches more compact? Something like this:
> 
> 	if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1)
> 		return 0;

I always prefer for conditions to be separate and readable even if it
means additional code. But if others feel that there's value in avoiding
two additional conditions I'm happy to adapt the patch.

> 
> Also, do we really need two different error values and error messages?

Before I added support for netns ids for additional requests Jiri made
it so that all request that specified properties that they did not
support returned ENOTSUPP instead of EINVAL. This just keeps things
consistent. Users would now suddenly receive EINVAL. That's potentially
confusing.
As for the case of passing multiple netns identifying properties into
the same request EINVAL seems the perfect candidate and the error
message seems instructive to userspace programs.

Thanks!
Christian

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

[Jiri Benc]

On Wed, 7 Feb 2018 14:36:21 +0100, Christian Brauner wrote:
> On Wed, Feb 07, 2018 at 04:20:01PM +0300, Kirill Tkhai wrote:
> > Can't we write these 3 above branches more compact? Something like this:
> > 
> > 	if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1)
> > 		return 0;  
> 
> I always prefer for conditions to be separate and readable even if it
> means additional code. But if others feel that there's value in avoiding
> two additional conditions I'm happy to adapt the patch.

FWIW, I don't like the n x n conditions much. But Kirill's proposal
seems not to be much better. I was thinking about:

	int cnt = 0;
	if (tb[IFLA_NET_NS_FD])
		cnt++;
	if (tb[IFLA_NET_NS_PID])
		cnt++;
	if (tb[IFLA_NET_NETNSID])
		cnt++;
	if (cnt > 1) {
		...errorr...
	}

but that's not better, either. As we're unlikely to add a fourth value,
I guess I'm okay with the current approach in the patch.

> Before I added support for netns ids for additional requests Jiri made
> it so that all request that specified properties that they did not
> support returned ENOTSUPP instead of EINVAL. This just keeps things
> consistent. Users would now suddenly receive EINVAL. That's potentially
> confusing.

Yes, please, keep -EOPNOTSUPP.

> As for the case of passing multiple netns identifying properties into
> the same request EINVAL seems the perfect candidate and the error
> message seems instructive to userspace programs.

Agreed.

Acked-by: Jiri Benc <jbenc@redhat.com>

Thanks,

 Jiri

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

[David Miller]

From: Christian Brauner <christian.brauner@ubuntu.com>
Date: Wed,  7 Feb 2018 13:53:20 +0100

> 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. For legacy reasons the kernel will
> pick the IFLA_NET_NS_PID property first and then look for the
> IFLA_NET_NS_FD property but there is no reason to extend this type of
> behavior to network namespace ids. 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>

Applied, thanks Christian.

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

[Christian Brauner]

On Thu, Feb 8, 2018 at 8:33 PM, David Miller <davem@davemloft.net> wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> Date: Wed,  7 Feb 2018 13:53:20 +0100
>
>> 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. For legacy reasons the kernel will
>> pick the IFLA_NET_NS_PID property first and then look for the
>> IFLA_NET_NS_FD property but there is no reason to extend this type of
>> behavior to network namespace ids. 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>
>
> Applied, thanks Christian.

Thanks for applying, David.