| Message ID | 1492747124-31821-2-git-send-email-jbainbri@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
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
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.
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...
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.
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
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);
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(-)