[2/2] ipv6: don't deliver packets with zero length to raw sockets

Submitted by Jamie Bainbridge on April 21, 2017, 3:58 a.m.

Details

Message ID 1492747124-31821-2-git-send-email-jbainbri@redhat.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Jamie Bainbridge April 21, 2017, 3:58 a.m.
IPv6 assumes there is data after the network header and blindly delivers
skbs to raw sockets without checking the presence of data.

With an application in a common loop where it checks select/poll/epoll
then ioctl(SIOCINQ/FIONREAD) is positive before continuing to
recvfrom(), this behaviour can cause the application to loop forever
on ioctl() because there is a zero-length skb to receive.

With this, it is very easy to make a Denial of Service attack by
crafting a packet which declares a Next Header in the IPv6 header but
does not actually supply a transport header and/or payload.

skb->len is already correctly set in ip6_input_finish() with pskb_pull()
so check this length before delivering zero data to raw sockets.

Signed-off-by: Jamie Bainbridge <jbainbri@redhat.com>
---
 net/ipv6/raw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sabrina Dubroca April 21, 2017, 10:01 a.m.
Hi Jamie,

2017-04-21, 13:58:44 +1000, Jamie Bainbridge wrote:
> IPv6 assumes there is data after the network header and blindly delivers
> skbs to raw sockets without checking the presence of data.
> 
> With an application in a common loop where it checks select/poll/epoll
> then ioctl(SIOCINQ/FIONREAD) is positive before continuing to
> recvfrom(), this behaviour can cause the application to loop forever
> on ioctl() because there is a zero-length skb to receive.
> 
> With this, it is very easy to make a Denial of Service attack by
> crafting a packet which declares a Next Header in the IPv6 header but
> does not actually supply a transport header and/or payload.
> 
> skb->len is already correctly set in ip6_input_finish() with pskb_pull()
> so check this length before delivering zero data to raw sockets.

Isn't that changing behavior? recv() currently returns 0 when a packet
that stops right after the IP header arrives. After this, the userspace
program won't receive anything in this case?
Jamie Bainbridge April 21, 2017, 11:18 a.m.
Hi Sabrina,

On Fri, Apr 21, 2017 at 8:01 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> Hi Jamie,
>
> 2017-04-21, 13:58:44 +1000, Jamie Bainbridge wrote:
>> IPv6 assumes there is data after the network header and blindly delivers
>> skbs to raw sockets without checking the presence of data.
>>
>> With an application in a common loop where it checks select/poll/epoll
>> then ioctl(SIOCINQ/FIONREAD) is positive before continuing to
>> recvfrom(), this behaviour can cause the application to loop forever
>> on ioctl() because there is a zero-length skb to receive.
>>
>> With this, it is very easy to make a Denial of Service attack by
>> crafting a packet which declares a Next Header in the IPv6 header but
>> does not actually supply a transport header and/or payload.
>>
>> skb->len is already correctly set in ip6_input_finish() with pskb_pull()
>> so check this length before delivering zero data to raw sockets.
>
> Isn't that changing behavior? recv() currently returns 0 when a packet
> that stops right after the IP header arrives. After this, the userspace
> program won't receive anything in this case?

The recv() never occurs. The skb will not be cloned or passed into
rawv6_rcv(), the socket notification method (select/poll/epoll) will
not trigger, and the userspace program won't be informed the packet
has arrived. The behaviour is the same as if there was no raw socket,
or as if the Next Header did not match the raw socket's protocol.

As you know, IPv6 raw sockets do not offer access to the network
header by design (RFC3542). An IPv6 raw socket only receives data
after the network header. It's not like IPv4 where the raw socket
would still get the network header in the same situation.

If the raw socket is watching for data with valid transport headers,
or the user has implemented their own transport protocol, or the user
is sending raw data with no transport header, those are still
correctly cloned and passed to rawv6_rcv() to be received by the raw
socket. Nothing is broken for cases where there is data after the
network header, I tested both paged and unpaged skbs and both worked
properly.

I cannot see the use in delivering a skb with zero bytes after the
network header to a raw socket. That is like suggesting a TCP ACK with
no data payload should result in a 0-byte skb being delivered to a
stream socket, which is obviously wrong and would result in many
notification-ioctl loops just like it has here.

Jamie
Sabrina Dubroca April 21, 2017, 12:47 p.m.
2017-04-21, 21:18:00 +1000, Jamie Bainbridge wrote:
> Hi Sabrina,
> 
> On Fri, Apr 21, 2017 at 8:01 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > Hi Jamie,
> >
> > 2017-04-21, 13:58:44 +1000, Jamie Bainbridge wrote:
> >> IPv6 assumes there is data after the network header and blindly delivers
> >> skbs to raw sockets without checking the presence of data.
> >>
> >> With an application in a common loop where it checks select/poll/epoll
> >> then ioctl(SIOCINQ/FIONREAD) is positive before continuing to
> >> recvfrom(), this behaviour can cause the application to loop forever
> >> on ioctl() because there is a zero-length skb to receive.
> >>
> >> With this, it is very easy to make a Denial of Service attack by
> >> crafting a packet which declares a Next Header in the IPv6 header but
> >> does not actually supply a transport header and/or payload.
> >>
> >> skb->len is already correctly set in ip6_input_finish() with pskb_pull()
> >> so check this length before delivering zero data to raw sockets.
> >
> > Isn't that changing behavior? recv() currently returns 0 when a packet
> > that stops right after the IP header arrives. After this, the userspace
> > program won't receive anything in this case?
> 
> The recv() never occurs. The skb will not be cloned or passed into
> rawv6_rcv(), the socket notification method (select/poll/epoll) will
> not trigger, and the userspace program won't be informed the packet
> has arrived. The behaviour is the same as if there was no raw socket,
> or as if the Next Header did not match the raw socket's protocol.
> 
> As you know, IPv6 raw sockets do not offer access to the network
> header by design (RFC3542). An IPv6 raw socket only receives data
> after the network header. It's not like IPv4 where the raw socket
> would still get the network header in the same situation.
> 
> If the raw socket is watching for data with valid transport headers,
> or the user has implemented their own transport protocol, or the user
> is sending raw data with no transport header, those are still
> correctly cloned and passed to rawv6_rcv() to be received by the raw
> socket. Nothing is broken for cases where there is data after the
> network header, I tested both paged and unpaged skbs and both worked
> properly.
> 
> I cannot see the use in delivering a skb with zero bytes after the
> network header to a raw socket.

Knowing that a message was received, even if it's malformed. I don't
see a fundamental difference between NextHeader == UDP without any
payload at all and NextHeader == UDP with 1B payload.
Also, with recvmsg, you would get back msg_name.

> That is like suggesting a TCP ACK with
> no data payload should result in a 0-byte skb being delivered to a
> stream socket

Not really. IMHO that's a difference between datagram and stream. An
empty message is different from no message at all.

> which is obviously wrong and would result in many
> notification-ioctl loops just like it has here.
David Miller April 21, 2017, 2:48 p.m.
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Fri, 21 Apr 2017 12:01:12 +0200

> Hi Jamie,
> 
> 2017-04-21, 13:58:44 +1000, Jamie Bainbridge wrote:
>> IPv6 assumes there is data after the network header and blindly delivers
>> skbs to raw sockets without checking the presence of data.
>> 
>> With an application in a common loop where it checks select/poll/epoll
>> then ioctl(SIOCINQ/FIONREAD) is positive before continuing to
>> recvfrom(), this behaviour can cause the application to loop forever
>> on ioctl() because there is a zero-length skb to receive.
>> 
>> With this, it is very easy to make a Denial of Service attack by
>> crafting a packet which declares a Next Header in the IPv6 header but
>> does not actually supply a transport header and/or payload.
>> 
>> skb->len is already correctly set in ip6_input_finish() with pskb_pull()
>> so check this length before delivering zero data to raw sockets.
> 
> Isn't that changing behavior? recv() currently returns 0 when a packet
> that stops right after the IP header arrives. After this, the userspace
> program won't receive anything in this case?

Yes, just like UDP, zero length packets should be allowed and processed.

Not silently dropped.

And this would give us yet another behavioral difference between ipv4 and
ipv6, so no thanks...
David Miller April 21, 2017, 2:53 p.m.
From: Jamie Bainbridge <jbainbri@redhat.com>
Date: Fri, 21 Apr 2017 21:18:00 +1000

> I cannot see the use in delivering a skb with zero bytes after the
> network header to a raw socket.

Then it cannot be used to look at zero length UDP packets, which are
completely legal and used.

So we must deliver it.
Jamie Bainbridge April 22, 2017, 12:10 p.m.
On Sat, Apr 22, 2017 at 12:53 AM, David Miller <davem@davemloft.net> wrote:
> From: Jamie Bainbridge <jbainbri@redhat.com>
> Date: Fri, 21 Apr 2017 21:18:00 +1000
>
>> I cannot see the use in delivering a skb with zero bytes after the
>> network header to a raw socket.
>
> Then it cannot be used to look at zero length UDP packets, which are
> completely legal and used.
>
> So we must deliver it.

Understood, thank you both for the clarification.

That would mean the pattern of select/ioctl/recvfrom is the incorrect
way to code an IPv6 raw socket application. I will let our user know.

How about the other patch in this series? That actually is a valid bug
when skb are paged in a certain way. That patch does not change
behaviour, it just allows ioctl to return the correct result whether
data is linear or paged. Will I resubmit that patch on its own with a
revised commit message?

Jamie

Patch hide | download patch | download mbox

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 0da6a12b5472e322d679572c7244e5c9bc467741..29dfdcefe1cc5f4c082ed919026e49e70320605e 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -174,7 +174,7 @@  static bool ipv6_raw_deliver(struct sk_buff *skb, int nexthdr)
 	read_lock(&raw_v6_hashinfo.lock);
 	sk = sk_head(&raw_v6_hashinfo.ht[hash]);
 
-	if (!sk)
+	if (!sk || !(skb->len))
 		goto out;
 
 	net = dev_net(skb->dev);