[PATCH 1/2] ipv6: check raw payload size correctly in ioctl

[PATCH 1/2] ipv6: check raw payload size correctly in ioctl

[Jamie Bainbridge]

In situations where an skb is paged, the transport header pointer and
tail pointer will be the same because the payload is in skb frags.

This results in ioctl(SIOCINQ/FIONREAD) returning a length of 0 when
the length to receive is actually greater than zero.

skb->len is already correctly set in ip6_input_finish() with
pskb_pull(), so use skb->len as it always returns the correct result
for both linear and paged data.

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

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index f174e76e6505d4045e940c9fceef765d2aaa937d..0da6a12b5472e322d679572c7244e5c9bc467741 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -1178,8 +1178,7 @@ static int rawv6_ioctl(struct sock *sk, int cmd, unsigned long arg)
 		spin_lock_bh(&sk->sk_receive_queue.lock);
 		skb = skb_peek(&sk->sk_receive_queue);
 		if (skb)
-			amount = skb_tail_pointer(skb) -
-				skb_transport_header(skb);
+			amount = skb->len;
 		spin_unlock_bh(&sk->sk_receive_queue.lock);
 		return put_user(amount, (int __user *)arg);
 	}
-- 
1.8.3.1

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

[Jamie Bainbridge]

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(-)

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);
-- 
1.8.3.1

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

[Sabrina Dubroca]

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?

-- 
Sabrina

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

[Jamie Bainbridge]

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

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

[Sabrina Dubroca]

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.


-- 
Sabrina

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

[David Miller]

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...

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

[David Miller]

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.

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

[Jamie Bainbridge]

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