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

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 7 Feb 2018 12:50:51 +0100
From:   Christian Brauner <christian.brauner@...onical.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        netdev@...r.kernel.org, ktkhai@...tuozzo.com,
        stephen@...workplumber.org, w.bumiller@...xmox.com,
        ebiederm@...ssion.com, nicolas.dichtel@...nd.com,
        linux-kernel@...r.kernel.org, dsahern@...il.com,
        davem@...emloft.net
Subject: Re: [PATCH net 1/1 v3] rtnetlink: require unique netns identifier

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.