netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] ipv6: drop incoming packets having a v4mapped source address
@ 2019-10-02 16:38 Eric Dumazet
  2019-10-02 18:38 ` Florian Westphal
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Eric Dumazet @ 2019-10-02 16:38 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, Florian Westphal,
	Hannes Frederic Sowa, syzbot

This began with a syzbot report. syzkaller was injecting
IPv6 TCP SYN packets having a v4mapped source address.

After an unsuccessful 4-tuple lookup, TCP creates a request
socket (SYN_RECV) and calls reqsk_queue_hash_req()

reqsk_queue_hash_req() calls sk_ehashfn(sk)

At this point we have AF_INET6 sockets, and the heuristic
used by sk_ehashfn() to either hash the IPv4 or IPv6 addresses
is to use ipv6_addr_v4mapped(&sk->sk_v6_daddr)

For the particular spoofed packet, we end up hashing V4 addresses
which were not initialized by the TCP IPv6 stack, so KMSAN fired
a warning.

I first fixed sk_ehashfn() to test both source and destination addresses,
but then faced various problems, including user-space programs
like packetdrill that had similar assumptions.

Instead of trying to fix the whole ecosystem, it is better
to admit that we have a dual stack behavior, and that we
can not build linux kernels without V4 stack anyway.

The dual stack API automatically forces the traffic to be IPv4
if v4mapped addresses are used at bind() or connect(), so it makes
no sense to allow IPv6 traffic to use the same v4mapped class.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/ipv6/ip6_input.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index d432d0011c160f41aec09640e95179dd7b364cfc..2bb0b66181a741c7fb73cacbdf34c5160f52d186 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -223,6 +223,16 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 	if (ipv6_addr_is_multicast(&hdr->saddr))
 		goto err;
 
+	/* While RFC4291 is not explicit about v4mapped addresses
+	 * in IPv6 headers, it seems clear linux dual-stack
+	 * model can not deal properly with these.
+	 * Security models could be fooled by ::ffff:127.0.0.1 for example.
+	 *
+	 * https://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02
+	 */
+	if (ipv6_addr_v4mapped(&hdr->saddr))
+		goto err;
+
 	skb->transport_header = skb->network_header + sizeof(*hdr);
 	IP6CB(skb)->nhoff = offsetof(struct ipv6hdr, nexthdr);
 
-- 
2.23.0.581.g78d2f28ef7-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: drop incoming packets having a v4mapped source address
  2019-10-02 16:38 [PATCH net] ipv6: drop incoming packets having a v4mapped source address Eric Dumazet
@ 2019-10-02 18:38 ` Florian Westphal
  2019-10-02 18:47   ` Eric Dumazet
  2019-10-03 15:42 ` David Miller
  2021-03-05 17:57 ` Jakub Kicinski
  2 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2019-10-02 18:38 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, netdev, Eric Dumazet, Florian Westphal,
	Hannes Frederic Sowa, syzbot

Eric Dumazet <edumazet@google.com> wrote:
> The dual stack API automatically forces the traffic to be IPv4
> if v4mapped addresses are used at bind() or connect(), so it makes
> no sense to allow IPv6 traffic to use the same v4mapped class.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> ---
>  net/ipv6/ip6_input.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index d432d0011c160f41aec09640e95179dd7b364cfc..2bb0b66181a741c7fb73cacbdf34c5160f52d186 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -223,6 +223,16 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>  	if (ipv6_addr_is_multicast(&hdr->saddr))
>  		goto err;
>  
> +	/* While RFC4291 is not explicit about v4mapped addresses
> +	 * in IPv6 headers, it seems clear linux dual-stack
> +	 * model can not deal properly with these.
> +	 * Security models could be fooled by ::ffff:127.0.0.1 for example.
> +	 *
> +	 * https://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02
> +	 */
> +	if (ipv6_addr_v4mapped(&hdr->saddr))
> +		goto err;
> +

Any reason to only consider ->saddr instead of checking daddr as well?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: drop incoming packets having a v4mapped source address
  2019-10-02 18:38 ` Florian Westphal
@ 2019-10-02 18:47   ` Eric Dumazet
  2019-10-02 21:54     ` Florian Westphal
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-10-02 18:47 UTC (permalink / raw)
  To: Florian Westphal
  Cc: David S . Miller, netdev, Eric Dumazet, Hannes Frederic Sowa, syzbot

On Wed, Oct 2, 2019 at 11:38 AM Florian Westphal <fw@strlen.de> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
> > The dual stack API automatically forces the traffic to be IPv4
> > if v4mapped addresses are used at bind() or connect(), so it makes
> > no sense to allow IPv6 traffic to use the same v4mapped class.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Florian Westphal <fw@strlen.de>
> > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > ---
> >  net/ipv6/ip6_input.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> > index d432d0011c160f41aec09640e95179dd7b364cfc..2bb0b66181a741c7fb73cacbdf34c5160f52d186 100644
> > --- a/net/ipv6/ip6_input.c
> > +++ b/net/ipv6/ip6_input.c
> > @@ -223,6 +223,16 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       if (ipv6_addr_is_multicast(&hdr->saddr))
> >               goto err;
> >
> > +     /* While RFC4291 is not explicit about v4mapped addresses
> > +      * in IPv6 headers, it seems clear linux dual-stack
> > +      * model can not deal properly with these.
> > +      * Security models could be fooled by ::ffff:127.0.0.1 for example.
> > +      *
> > +      * https://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02
> > +      */
> > +     if (ipv6_addr_v4mapped(&hdr->saddr))
> > +             goto err;
> > +
>
> Any reason to only consider ->saddr instead of checking daddr as well?

I do not see reasons the packet should be accepted for sane configurations ?

I would rather have a separate patch for daddr if someone needs it.

(This also comes at a cpu cost for all received packets :/ )

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: drop incoming packets having a v4mapped source address
  2019-10-02 18:47   ` Eric Dumazet
@ 2019-10-02 21:54     ` Florian Westphal
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2019-10-02 21:54 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Westphal, David S . Miller, netdev, Eric Dumazet,
	Hannes Frederic Sowa, syzbot

Eric Dumazet <edumazet@google.com> wrote:
> > > @@ -223,6 +223,16 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> > >       if (ipv6_addr_is_multicast(&hdr->saddr))
> > >               goto err;
> > >
> > > +     /* While RFC4291 is not explicit about v4mapped addresses
> > > +      * in IPv6 headers, it seems clear linux dual-stack
> > > +      * model can not deal properly with these.
> > > +      * Security models could be fooled by ::ffff:127.0.0.1 for example.
> > > +      *
> > > +      * https://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02
> > > +      */
> > > +     if (ipv6_addr_v4mapped(&hdr->saddr))
> > > +             goto err;
> > > +
> >
> > Any reason to only consider ->saddr instead of checking daddr as well?
> 
> I do not see reasons the packet should be accepted for sane configurations ?

Fair enough, thanks for explaining.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: drop incoming packets having a v4mapped source address
  2019-10-02 16:38 [PATCH net] ipv6: drop incoming packets having a v4mapped source address Eric Dumazet
  2019-10-02 18:38 ` Florian Westphal
@ 2019-10-03 15:42 ` David Miller
  2021-03-05 17:57 ` Jakub Kicinski
  2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-10-03 15:42 UTC (permalink / raw)
  To: edumazet; +Cc: netdev, eric.dumazet, fw, hannes, syzkaller

From: Eric Dumazet <edumazet@google.com>
Date: Wed,  2 Oct 2019 09:38:55 -0700

> This began with a syzbot report. syzkaller was injecting
> IPv6 TCP SYN packets having a v4mapped source address.
> 
> After an unsuccessful 4-tuple lookup, TCP creates a request
> socket (SYN_RECV) and calls reqsk_queue_hash_req()
> 
> reqsk_queue_hash_req() calls sk_ehashfn(sk)
> 
> At this point we have AF_INET6 sockets, and the heuristic
> used by sk_ehashfn() to either hash the IPv4 or IPv6 addresses
> is to use ipv6_addr_v4mapped(&sk->sk_v6_daddr)
> 
> For the particular spoofed packet, we end up hashing V4 addresses
> which were not initialized by the TCP IPv6 stack, so KMSAN fired
> a warning.
> 
> I first fixed sk_ehashfn() to test both source and destination addresses,
> but then faced various problems, including user-space programs
> like packetdrill that had similar assumptions.
> 
> Instead of trying to fix the whole ecosystem, it is better
> to admit that we have a dual stack behavior, and that we
> can not build linux kernels without V4 stack anyway.
> 
> The dual stack API automatically forces the traffic to be IPv4
> if v4mapped addresses are used at bind() or connect(), so it makes
> no sense to allow IPv6 traffic to use the same v4mapped class.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Reported-by: syzbot <syzkaller@googlegroups.com>

Applied and queued up for -stable, thanks Eric.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net] ipv6: drop incoming packets having a v4mapped source address
  2019-10-02 16:38 [PATCH net] ipv6: drop incoming packets having a v4mapped source address Eric Dumazet
  2019-10-02 18:38 ` Florian Westphal
  2019-10-03 15:42 ` David Miller
@ 2021-03-05 17:57 ` Jakub Kicinski
  2 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2021-03-05 17:57 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, netdev, Eric Dumazet, Florian Westphal,
	Hannes Frederic Sowa, syzbot

On Wed,  2 Oct 2019 09:38:55 -0700 Eric Dumazet wrote:
> This began with a syzbot report. syzkaller was injecting
> IPv6 TCP SYN packets having a v4mapped source address.
> 
> After an unsuccessful 4-tuple lookup, TCP creates a request
> socket (SYN_RECV) and calls reqsk_queue_hash_req()
> 
> reqsk_queue_hash_req() calls sk_ehashfn(sk)
> 
> At this point we have AF_INET6 sockets, and the heuristic
> used by sk_ehashfn() to either hash the IPv4 or IPv6 addresses
> is to use ipv6_addr_v4mapped(&sk->sk_v6_daddr)
> 
> For the particular spoofed packet, we end up hashing V4 addresses
> which were not initialized by the TCP IPv6 stack, so KMSAN fired
> a warning.
> 
> I first fixed sk_ehashfn() to test both source and destination addresses,
> but then faced various problems, including user-space programs
> like packetdrill that had similar assumptions.
> 
> Instead of trying to fix the whole ecosystem, it is better
> to admit that we have a dual stack behavior, and that we
> can not build linux kernels without V4 stack anyway.
> 
> The dual stack API automatically forces the traffic to be IPv4
> if v4mapped addresses are used at bind() or connect(), so it makes
> no sense to allow IPv6 traffic to use the same v4mapped class.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Reported-by: syzbot <syzkaller@googlegroups.com>

FTR this appears to break an UDP/sFlow application which used to work
fine with mapped addresses. Given the IETF memo perhaps a sysctl would
be appropriate, but even with that we're back to problems in TCP if the
sysctl is flipped :S

> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index d432d0011c160f41aec09640e95179dd7b364cfc..2bb0b66181a741c7fb73cacbdf34c5160f52d186 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -223,6 +223,16 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>  	if (ipv6_addr_is_multicast(&hdr->saddr))
>  		goto err;
>  
> +	/* While RFC4291 is not explicit about v4mapped addresses
> +	 * in IPv6 headers, it seems clear linux dual-stack
> +	 * model can not deal properly with these.
> +	 * Security models could be fooled by ::ffff:127.0.0.1 for example.
> +	 *
> +	 * https://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02
> +	 */
> +	if (ipv6_addr_v4mapped(&hdr->saddr))
> +		goto err;
> +
>  	skb->transport_header = skb->network_header + sizeof(*hdr);
>  	IP6CB(skb)->nhoff = offsetof(struct ipv6hdr, nexthdr);
>  


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-05 17:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-02 16:38 [PATCH net] ipv6: drop incoming packets having a v4mapped source address Eric Dumazet
2019-10-02 18:38 ` Florian Westphal
2019-10-02 18:47   ` Eric Dumazet
2019-10-02 21:54     ` Florian Westphal
2019-10-03 15:42 ` David Miller
2021-03-05 17:57 ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).