diff mbox series

[PATCHv5,net,2/2] IPv6: reply ICMP error if the first fragment don't include all headers

Message ID 20201027022833.3697522-3-liuhangbin@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series IPv6: reply ICMP error if fragment doesn't contain all headers | expand

Commit Message

Hangbin Liu Oct. 27, 2020, 2:28 a.m. UTC
Based on RFC 8200, Section 4.5 Fragment Header:

  -  If the first fragment does not include all headers through an
     Upper-Layer header, then that fragment should be discarded and
     an ICMP Parameter Problem, Code 3, message should be sent to
     the source of the fragment, with the Pointer field set to zero.

As the packet may be any kind of L4 protocol, I only checked some common
protocols' header length and handle others by (offset + 1) > skb->len.
Checking each packet header in IPv6 fast path will have performance impact,
so I put the checking in ipv6_frag_rcv().

When send ICMP error message, if the 1st truncated fragment is ICMP message,
icmp6_send() will break as is_ineligible() return true. So I added a check
in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
return false.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v5:
Only check nexthdr if ipv6_skip_exthdr() does not return -1. For
IPPROTO_NONE/NEXTHDR_NONE, later code will handle and ignore it.

v4:
remove unused variable

v3:
a) use frag_off to check if this is a fragment packet
b) check some common protocols' header length

v2:
a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
   return value
b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()

---
 net/ipv6/icmp.c       |  8 +++++++-
 net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

Comments

Georg Kohmann (geokohma) Oct. 27, 2020, 7:57 a.m. UTC | #1
On 27.10.2020 03:28, Hangbin Liu wrote:
> Based on RFC 8200, Section 4.5 Fragment Header:
>
>   -  If the first fragment does not include all headers through an
>      Upper-Layer header, then that fragment should be discarded and
>      an ICMP Parameter Problem, Code 3, message should be sent to
>      the source of the fragment, with the Pointer field set to zero.
>
> As the packet may be any kind of L4 protocol, I only checked some common
> protocols' header length and handle others by (offset + 1) > skb->len.
> Checking each packet header in IPv6 fast path will have performance impact,
> so I put the checking in ipv6_frag_rcv().
>
> When send ICMP error message, if the 1st truncated fragment is ICMP message,
> icmp6_send() will break as is_ineligible() return true. So I added a check
> in is_ineligible() to let fragment packet with nexthdr ICMP but no ICMP header
> return false.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> v5:
> Only check nexthdr if ipv6_skip_exthdr() does not return -1. For
> IPPROTO_NONE/NEXTHDR_NONE, later code will handle and ignore it.
>
> v4:
> remove unused variable
>
> v3:
> a) use frag_off to check if this is a fragment packet
> b) check some common protocols' header length
>
> v2:
> a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
>    return value
> b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()
>
> ---
>  net/ipv6/icmp.c       |  8 +++++++-
>  net/ipv6/reassembly.c | 33 ++++++++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..8956144ea65e 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -158,7 +158,13 @@ static bool is_ineligible(const struct sk_buff *skb)
>  		tp = skb_header_pointer(skb,
>  			ptr+offsetof(struct icmp6hdr, icmp6_type),
>  			sizeof(_type), &_type);
> -		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
> +
> +		/* Based on RFC 8200, Section 4.5 Fragment Header, return
> +		 * false if this is a fragment packet with no icmp header info.
> +		 */
> +		if (!tp && frag_off != 0)
> +			return false;
> +		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
>  			return true;
>  	}
>  	return false;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 1f5d4d196dcc..effe1d086e5d 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -42,6 +42,8 @@
>  #include <linux/skbuff.h>
>  #include <linux/slab.h>
>  #include <linux/export.h>
> +#include <linux/tcp.h>
> +#include <linux/udp.h>
>  
>  #include <net/sock.h>
>  #include <net/snmp.h>
> @@ -322,7 +324,9 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  	struct frag_queue *fq;
>  	const struct ipv6hdr *hdr = ipv6_hdr(skb);
>  	struct net *net = dev_net(skb_dst(skb)->dev);
> -	int iif;
> +	__be16 frag_off;
> +	int iif, offset;
> +	u8 nexthdr;
>  
>  	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>  		goto fail_hdr;
> @@ -351,6 +355,33 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>  		return 1;
>  	}
>  
> +	/* RFC 8200, Section 4.5 Fragment Header:
> +	 * If the first fragment does not include all headers through an
> +	 * Upper-Layer header, then that fragment should be discarded and
> +	 * an ICMP Parameter Problem, Code 3, message should be sent to
> +	 * the source of the fragment, with the Pointer field set to zero.
> +	 */
> +	nexthdr = hdr->nexthdr;
> +	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
> +	if (offset >= 0) {
> +		/* Check some common protocols' header */
> +		if (nexthdr == IPPROTO_TCP)
> +			offset += sizeof(struct tcphdr);
> +		else if (nexthdr == IPPROTO_UDP)
> +			offset += sizeof(struct udphdr);
> +		else if (nexthdr == IPPROTO_ICMPV6)
> +			offset += sizeof(struct icmp6hdr);
> +		else
> +			offset += 1;
> +
> +		if (frag_off == htons(IP6_MF) && offset > skb->len) {

This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future. I suggest you only check that the offset is 0:

frag_off & htons(IP6_OFFSET)

Sorry for not commenting on this earlier.

> +			__IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
> +					IPSTATS_MIB_INHDRERRORS);
> +			icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> +			return -1;
> +		}
> +	}
> +
>  	iif = skb->dev ? skb->dev->ifindex : 0;
>  	fq = fq_find(net, fhdr->identification, hdr, iif);
>  	if (fq) {
Hangbin Liu Oct. 27, 2020, 9:57 a.m. UTC | #2
On Tue, Oct 27, 2020 at 07:57:06AM +0000, Georg Kohmann (geokohma) wrote:
> > +	/* RFC 8200, Section 4.5 Fragment Header:
> > +	 * If the first fragment does not include all headers through an
> > +	 * Upper-Layer header, then that fragment should be discarded and
> > +	 * an ICMP Parameter Problem, Code 3, message should be sent to
> > +	 * the source of the fragment, with the Pointer field set to zero.
> > +	 */
> > +	nexthdr = hdr->nexthdr;
> > +	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
> > +	if (offset >= 0) {
> > +		/* Check some common protocols' header */
> > +		if (nexthdr == IPPROTO_TCP)
> > +			offset += sizeof(struct tcphdr);
> > +		else if (nexthdr == IPPROTO_UDP)
> > +			offset += sizeof(struct udphdr);
> > +		else if (nexthdr == IPPROTO_ICMPV6)
> > +			offset += sizeof(struct icmp6hdr);
> > +		else
> > +			offset += 1;
> > +
> > +		if (frag_off == htons(ip6_mf) && offset > skb->len) {
> 
> This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future.

Thanks, I also didn't aware this scenario.

> I suggest you only check that the offset is 0:
> frag_off & htons(IP6_OFFSET)

This will match all other fragment packets. RFC request we reply ICMP for the
first fragment packet, Do you mean

if (!frag_off & htons(IP6_OFFSET) && offset > skb->len)

Thanks
Hangbin
Georg Kohmann (geokohma) Oct. 27, 2020, 10:20 a.m. UTC | #3
On 27.10.2020 10:57, Hangbin Liu wrote:
> On Tue, Oct 27, 2020 at 07:57:06AM +0000, Georg Kohmann (geokohma) wrote:
>>> +	/* RFC 8200, Section 4.5 Fragment Header:
>>> +	 * If the first fragment does not include all headers through an
>>> +	 * Upper-Layer header, then that fragment should be discarded and
>>> +	 * an ICMP Parameter Problem, Code 3, message should be sent to
>>> +	 * the source of the fragment, with the Pointer field set to zero.
>>> +	 */
>>> +	nexthdr = hdr->nexthdr;
>>> +	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
>>> +	if (offset >= 0) {
>>> +		/* Check some common protocols' header */
>>> +		if (nexthdr == IPPROTO_TCP)
>>> +			offset += sizeof(struct tcphdr);
>>> +		else if (nexthdr == IPPROTO_UDP)
>>> +			offset += sizeof(struct udphdr);
>>> +		else if (nexthdr == IPPROTO_ICMPV6)
>>> +			offset += sizeof(struct icmp6hdr);
>>> +		else
>>> +			offset += 1;
>>> +
>>> +		if (frag_off == htons(ip6_mf) && offset > skb->len) {
>> This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future.
> Thanks, I also didn't aware this scenario.
>
>> I suggest you only check that the offset is 0:
>> frag_off & htons(IP6_OFFSET)
> This will match all other fragment packets. RFC request we reply ICMP for the
> first fragment packet, Do you mean
>
> if (!frag_off & htons(IP6_OFFSET) && offset > skb->len)

Almost, add some parentheses:

if (!(frag_off & htons(IP6_OFFSET)) && offset > skb->len)

>
> Thanks
> Hangbin
Willem de Bruijn Oct. 30, 2020, 3:31 p.m. UTC | #4
On Tue, Oct 27, 2020 at 5:57 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Tue, Oct 27, 2020 at 07:57:06AM +0000, Georg Kohmann (geokohma) wrote:
> > > +   /* RFC 8200, Section 4.5 Fragment Header:
> > > +    * If the first fragment does not include all headers through an
> > > +    * Upper-Layer header, then that fragment should be discarded and
> > > +    * an ICMP Parameter Problem, Code 3, message should be sent to
> > > +    * the source of the fragment, with the Pointer field set to zero.
> > > +    */
> > > +   nexthdr = hdr->nexthdr;
> > > +   offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
> > > +   if (offset >= 0) {
> > > +           /* Check some common protocols' header */
> > > +           if (nexthdr == IPPROTO_TCP)
> > > +                   offset += sizeof(struct tcphdr);
> > > +           else if (nexthdr == IPPROTO_UDP)
> > > +                   offset += sizeof(struct udphdr);
> > > +           else if (nexthdr == IPPROTO_ICMPV6)
> > > +                   offset += sizeof(struct icmp6hdr);
> > > +           else
> > > +                   offset += 1;
> > > +
> > > +           if (frag_off == htons(ip6_mf) && offset > skb->len) {
> >
> > This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future.
>
> Thanks, I also didn't aware this scenario.

Sorry, what are atomic fragments?

Do you mean packets with a fragment header that encapsulates the
entire packet? If so, isn't that handled in the branch right above?
("/* It is not a fragmented frame */"). That said, the test based on
IP6_OFFSET LGTM.
Georg Kohmann (geokohma) Oct. 30, 2020, 6:39 p.m. UTC | #5
On 30.10.2020 16:31, Willem de Bruijn wrote:
> On Tue, Oct 27, 2020 at 5:57 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>> On Tue, Oct 27, 2020 at 07:57:06AM +0000, Georg Kohmann (geokohma) wrote:
>>>> +   /* RFC 8200, Section 4.5 Fragment Header:
>>>> +    * If the first fragment does not include all headers through an
>>>> +    * Upper-Layer header, then that fragment should be discarded and
>>>> +    * an ICMP Parameter Problem, Code 3, message should be sent to
>>>> +    * the source of the fragment, with the Pointer field set to zero.
>>>> +    */
>>>> +   nexthdr = hdr->nexthdr;
>>>> +   offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
>>>> +   if (offset >= 0) {
>>>> +           /* Check some common protocols' header */
>>>> +           if (nexthdr == IPPROTO_TCP)
>>>> +                   offset += sizeof(struct tcphdr);
>>>> +           else if (nexthdr == IPPROTO_UDP)
>>>> +                   offset += sizeof(struct udphdr);
>>>> +           else if (nexthdr == IPPROTO_ICMPV6)
>>>> +                   offset += sizeof(struct icmp6hdr);
>>>> +           else
>>>> +                   offset += 1;
>>>> +
>>>> +           if (frag_off == htons(ip6_mf) && offset > skb->len) {
>>> This do not catch atomic fragments (fragmented packet with only one fragment). frag_off also contains two reserved bits (both 0) that might change in the future.
>> Thanks, I also didn't aware this scenario.
> Sorry, what are atomic fragments?
>
> Do you mean packets with a fragment header that encapsulates the
> entire packet? If so, isn't that handled in the branch right above?
> ("/* It is not a fragmented frame */"). That said, the test based on
> IP6_OFFSET LGTM.
Yes, an atomic fragment is a packet containing a fragment header
without actually beeing fragmented (see RFC6946 and RFC8021).

And you are right, it is handled in the branch right above, sorry for
not seeing that. But still, the test based on IP6_OFFSET is more
accurate as IP6_MF is set for all but the very last fragment.
However, it probably doesn't matter in this context.
diff mbox series

Patch

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index ec448b71bf9a..8956144ea65e 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -158,7 +158,13 @@  static bool is_ineligible(const struct sk_buff *skb)
 		tp = skb_header_pointer(skb,
 			ptr+offsetof(struct icmp6hdr, icmp6_type),
 			sizeof(_type), &_type);
-		if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
+
+		/* Based on RFC 8200, Section 4.5 Fragment Header, return
+		 * false if this is a fragment packet with no icmp header info.
+		 */
+		if (!tp && frag_off != 0)
+			return false;
+		else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
 			return true;
 	}
 	return false;
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 1f5d4d196dcc..effe1d086e5d 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -42,6 +42,8 @@ 
 #include <linux/skbuff.h>
 #include <linux/slab.h>
 #include <linux/export.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 
 #include <net/sock.h>
 #include <net/snmp.h>
@@ -322,7 +324,9 @@  static int ipv6_frag_rcv(struct sk_buff *skb)
 	struct frag_queue *fq;
 	const struct ipv6hdr *hdr = ipv6_hdr(skb);
 	struct net *net = dev_net(skb_dst(skb)->dev);
-	int iif;
+	__be16 frag_off;
+	int iif, offset;
+	u8 nexthdr;
 
 	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
 		goto fail_hdr;
@@ -351,6 +355,33 @@  static int ipv6_frag_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
+	/* RFC 8200, Section 4.5 Fragment Header:
+	 * If the first fragment does not include all headers through an
+	 * Upper-Layer header, then that fragment should be discarded and
+	 * an ICMP Parameter Problem, Code 3, message should be sent to
+	 * the source of the fragment, with the Pointer field set to zero.
+	 */
+	nexthdr = hdr->nexthdr;
+	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
+	if (offset >= 0) {
+		/* Check some common protocols' header */
+		if (nexthdr == IPPROTO_TCP)
+			offset += sizeof(struct tcphdr);
+		else if (nexthdr == IPPROTO_UDP)
+			offset += sizeof(struct udphdr);
+		else if (nexthdr == IPPROTO_ICMPV6)
+			offset += sizeof(struct icmp6hdr);
+		else
+			offset += 1;
+
+		if (frag_off == htons(IP6_MF) && offset > skb->len) {
+			__IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
+					IPSTATS_MIB_INHDRERRORS);
+			icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
+			return -1;
+		}
+	}
+
 	iif = skb->dev ? skb->dev->ifindex : 0;
 	fq = fq_find(net, fhdr->identification, hdr, iif);
 	if (fq) {