netdev - Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6 addresses

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]

Date:   Fri, 09 Mar 2018 18:04:03 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Guillaume Nault <g.nault@...halink.fr>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        James Chapman <jchapman@...alix.com>,
        Wei Wang <weiwan@...gle.com>, David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net v2 2/2] l2tp: fix races with ipv4-mapped ipv6
 addresses

Hi,

On Fri, 2018-03-09 at 17:43 +0100, Guillaume Nault wrote:
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index 83421c6f0bef..9726e3f37745 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -1112,11 +1125,32 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >  		goto out_unlock;
> >  	}
> >  
> > +	/* The user-space may change the connection status for the user-space
> > +	 * provided socket at run time: we must check it under the socket lock
> > +	 */
> > +	inet = inet_sk(sk);
> > +	if (tunnel->fd >= 0) {
> > +		if (sk->sk_state != TCP_ESTABLISHED) {
> > +			ret = NET_XMIT_DROP;
> > +			goto out_unlock;
> > +		}
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +		/* If the uses space changes the ipv4-mapped ipv6 address,
> 
> I guess you meant 'user-space'.

yep ;)

> > +		 * the kernel copy of the ipv4 address is not updated.
> > +		 * Refresh it only if needed, to avoid dirtying the socket
> > +		 * on each packet.
> > +		 */
> > +		if (l2tp_sk_is_v4mapped(sk) &&
> > +		    inet->inet_daddr != sk->sk_v6_daddr.s6_addr32[3])
> 
> I can't see how to trigger this condition. Re-connecting the socket
> doesn't do, as __ip6_datagram_connect() already updates both
> ->inet_daddr and ->sk_v6_daddr.

Uh, apparently I trusted too much the original code below...

> > @@ -1131,7 +1165,7 @@ int l2tp_xmit_skb(struct l2tp_session *session, struct sk_buff *skb, int hdr_len
> >  
> >  		/* Calculate UDP checksum if configured to do so */
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -		if (sk->sk_family == PF_INET6 && !tunnel->v4mapped)
> > +		if (l2tp_sk_is_v6(sk))
> >  			udp6_set_csum(udp_get_no_check6_tx(sk),
> >  				      skb, &inet6_sk(sk)->saddr,
> >  				      &sk->sk_v6_daddr, udp_len);
> > @@ -1449,6 +1483,14 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> >  			err = -EINVAL;
> >  			goto err;
> >  		}
> > +
> > +		/* Reject unconnected sockets */
> > +		if (sock->sk->sk_state != TCP_ESTABLISHED) {
> > +			pr_debug("tunl %u: sock fd=%d is unconnected\n",
> > +			       tunnel_id, fd);
> > +			err = -EINVAL;
> 
> -ENOTCONN looks more appropriate. And BTW, the socket isn't locked here.

Ok. I think locking is not needed if we only check the sk state.
> 
> > @@ -1508,20 +1550,13 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
> >  		tunnel->debug = cfg->debug;
> >  
> >  #if IS_ENABLED(CONFIG_IPV6)
> > -	if (sk->sk_family == PF_INET6) {
> > +	if (l2tp_sk_is_v4mapped(sk)) {
> >  		struct ipv6_pinfo *np = inet6_sk(sk);
> > +		struct inet_sock *inet = inet_sk(sk);
> >  
> > -		if (ipv6_addr_v4mapped(&np->saddr) &&
> > -		    ipv6_addr_v4mapped(&sk->sk_v6_daddr)) {
> > -			struct inet_sock *inet = inet_sk(sk);
> > -
> > -			tunnel->v4mapped = true;
> > -			inet->inet_saddr = np->saddr.s6_addr32[3];
> > -			inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > -			inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> > -		} else {
> > -			tunnel->v4mapped = false;
> > -		}
> > +		inet->inet_saddr = np->saddr.s6_addr32[3];
> > +		inet->inet_rcv_saddr = sk->sk_v6_rcv_saddr.s6_addr32[3];
> > +		inet->inet_daddr = sk->sk_v6_daddr.s6_addr32[3];
> >  	}
> 
> Same question as for the l2tp_xmit_skb() part: how can one create a
> v4mapped socket with unset (or invalid) ->inet_*addr?

Double-checking the ipv6 connect, apparently we don't need to copy the
ipv4 mapped address, so the above chunk could be dropped.

The syzbot reproducer is able to trigger the condition you describe
calling connect() 2 consecutive times, the first one on an ipv4 mapped
address and the second one an (unmapped) ipv6 address.

I will send a v3.

Thanks,

Paolo