Message-ID: <3c81d2ae-ba14-60d8-247d-87fabf407fea@cisco.com>
Date:   Mon, 9 Nov 2020 22:08:47 +0000
From:   "Georg Kohmann (geokohma)" <geokohma@...co.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "pablo@...filter.org" <pablo@...filter.org>,
        "kadlec@...filter.org" <kadlec@...filter.org>,
        "fw@...len.de" <fw@...len.de>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuznet@....inr.ac.ru" <kuznet@....inr.ac.ru>,
        "yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>,
        "netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
        "coreteam@...filter.org" <coreteam@...filter.org>
Subject: Re: [PATCH net v3] ipv6/netfilter: Discard first fragment not
 including all headers

On 09.11.2020 21:50, Jakub Kicinski wrote:
> On Mon,  9 Nov 2020 12:52:49 +0100 Georg Kohmann wrote:
>> Packets are processed even though the first fragment don't include all
>> headers through the upper layer header. This breaks TAHI IPv6 Core
>> Conformance Test v6LC.1.3.6.
>>
>> Referring to RFC8200 SECTION 4.5: "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."
>>
>> The fragment needs to be validated the same way it is done in
>> commit 2efdaaaf883a ("IPv6: reply ICMP error if the first fragment don't
>> include all headers") for ipv6. Wrap the validation into a common function,
>> ipv6_frag_validate(). A closer inspection of the existing validation show
>> that it does not fullfill all aspects of RFC 8200, section 4.5, but is at
>> the moment sufficient to pass mentioned TAHI test.
>>
>> In netfilter, utilize the fragment offset returned by find_prev_fhdr() to
>> let ipv6_frag_validate() start it's traverse from the fragment header.
>>
>> Return 0 to drop the fragment in the netfilter. This is the same behaviour
>> as used on other protocol errors in this function, e.g. when
>> nf_ct_frag6_queue() returns -EPROTO. The Fragment will later be picked up
>> by ipv6_frag_rcv() in reassembly.c. ipv6_frag_rcv() will then send an
>> appropriate ICMP Parameter Problem message back to the source.
>>
>> References commit 2efdaaaf883a ("IPv6: reply ICMP error if the first
>> fragment don't include all headers")
> new line here, since the line above is not really a tag.
>
>> Signed-off-by: Georg Kohmann <geokohma@...co.com>
>> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
>> index bd1f396..489f3f9 100644
>> --- a/include/net/ipv6.h
>> +++ b/include/net/ipv6.h
>> @@ -1064,6 +1064,8 @@ int ipv6_skip_exthdr(const struct sk_buff *, int start, u8 *nexthdrp,
>>  
>>  bool ipv6_ext_hdr(u8 nexthdr);
>>  
>> +bool ipv6_frag_validate(struct sk_buff *skb, int start, u8 *nexthdrp);
>> +
>>  enum {
>>  	IP6_FH_F_FRAG		= (1 << 0),
>>  	IP6_FH_F_AUTH		= (1 << 1),
>> diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
>> index da46c42..7a94fdf 100644
>> --- a/net/ipv6/exthdrs_core.c
>> +++ b/net/ipv6/exthdrs_core.c
>> @@ -278,3 +278,46 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
>>  	return nexthdr;
>>  }
>>  EXPORT_SYMBOL(ipv6_find_hdr);
>> +
>> +/* Validate that the upper layer header is not truncated in fragment.
>> + *
>> + * This function returns false if a TCP, UDP or ICMP header is truncated
>> + * just before or in the middle of the header. It also returns false if
>> + * any other upper layer header is truncated just before the first byte.
>> + *
>> + * Notes:
>> + * -It does NOT return false if the first fragment where truncated
> More spaces needed, i.e.
>
> Notes:
>  - It...
>
>> + * elsewhere, i.e. between or in the middle of one of the extension
>> + * headers or in the middle of one of the upper layer headers, except for
>> + * TCP, UDP and ICMP.
>> + * -The function also returns true if the fragment is not the first
>> + * fragment.
>> + */
>> +
> no need for a new line here
>
>> +bool ipv6_frag_validate(struct sk_buff *skb, int start, u8 *nexthdrp)
> (a) why place this function in exthdrs_core? I don't see any header
>     specific code here, IMO it belongs in reassembly.c.

ipv6_frag_validate() is used in both reassembly.c and nf_conntrack_reasm.c
Where should I put the prototype so it can be used both places?

>
> (b) the name is a bit broad, how about ipv6_frag_thdr_tuncated() or
>     some such?
Yes agree, much better.
>> +{
>> +	int offset;
>> +	u8 nexthdr = *nexthdrp;
>> +	__be16 frag_off;
> order these longest line to shortest (rev xmas tree) please.
>
>> +
>> +	offset = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off);
>> +	if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
> nit: since this is a function now you can reverse the condition, return
> early, and save the indentation level in all the code below
>
>> +		switch (nexthdr) {
>> +		case NEXTHDR_TCP:
>> +			offset += sizeof(struct tcphdr);
>> +			break;
>> +		case NEXTHDR_UDP:
>> +			offset += sizeof(struct udphdr);
>> +			break;
>> +		case NEXTHDR_ICMP:
>> +			offset += sizeof(struct icmp6hdr);
>> +			break;
>> +		default:
>> +			offset += 1;
>> +		}
>> +		if (offset > skb->len)
>> +			return false;
>> +	}
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(ipv6_frag_validate);
>> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> index 054d287..f6cae28 100644
>> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
>> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> @@ -445,6 +445,7 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
>>  	struct frag_queue *fq;
>>  	struct ipv6hdr *hdr;
>>  	u8 prevhdr;
>> +	u8 nexthdr = NEXTHDR_FRAGMENT;
> rev xmas tree
>
>>  	/* Jumbo payload inhibits frag. header */
>>  	if (ipv6_hdr(skb)->payload_len == 0) {
>> @@ -455,6 +456,14 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
>>  	if (find_prev_fhdr(skb, &prevhdr, &nhoff, &fhoff) < 0)
>>  		return 0;
>>  
>> +	/* Discard the first fragment if it does not include all headers
>> +	 * RFC 8200, Section 4.5
>> +	 */
>> +	if (!ipv6_frag_validate(skb, fhoff, &nexthdr)) {
>> +		pr_debug("Drop incomplete fragment\n");
>> +		return 0;
>> +	}
>>
>>  	if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
>>  		return -ENOMEM;
>>  
>> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
>> index c8cf1bb..04e078e 100644
>> --- a/net/ipv6/reassembly.c
>> +++ b/net/ipv6/reassembly.c
>> @@ -324,8 +324,7 @@ 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);
>> -	__be16 frag_off;
>> -	int iif, offset;
>> +	int iif;
> rev xmas tree
>
>>  	u8 nexthdr;
>>  
>>  	if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>> @@ -362,24 +361,11 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>>  	 * 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_OFFSET)) && 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;
>> -		}
>> +	if (!ipv6_frag_validate(skb, skb_transport_offset(skb), &nexthdr)) {
>> +		__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;
Thanks for reviewing, with good input. Please see question above.

Georg

Powered by blists - more mailing lists