Re: [PATCH net] ipv6/netfilter: Discard first fragment not including all headers

["Georg Kohmann (geokohma)" <geokohma@cisco.com>]

On 04.11.2020 14:41, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, Nov 04, 2020 at 02:01:28PM +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."
>>
>> Utilize the fragment offset returned by find_prev_fhdr() to let
>> ipv6_skip_exthdr() start it's traverse from the fragment header.
>> Apply the same logic for checking that all headers are included as used
>> in commit 2efdaaaf883a ("IPv6: reply ICMP error if the first fragment don't
>> include all headers"). Check that TCP, UDP and ICMP headers are completely
>> included in the fragment and all other headers are included with at least
>> one byte.
>>
>> Return 0 to drop the fragment. 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")
>> Signed-off-by: Georg Kohmann <geokohma@cisco.com>
>> ---
>>  net/ipv6/netfilter/nf_conntrack_reasm.c | 28 +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> index 054d287..dffa3a8 100644
>> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
>> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> @@ -440,11 +440,13 @@ find_prev_fhdr(struct sk_buff *skb, u8 *prevhdrp, int *prevhoff, int *fhoff)
>>  int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
>>  {
>>  	u16 savethdr = skb->transport_header;
>> -	int fhoff, nhoff, ret;
>> +	int fhoff, nhoff, ret, offset;
>>  	struct frag_hdr *fhdr;
>>  	struct frag_queue *fq;
>>  	struct ipv6hdr *hdr;
>>  	u8 prevhdr;
>> +	u8 nexthdr = NEXTHDR_FRAGMENT;
>> +	__be16 frag_off;
>>  
>>  	/* Jumbo payload inhibits frag. header */
>>  	if (ipv6_hdr(skb)->payload_len == 0) {
>> @@ -455,6 +457,30 @@ 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
>> +	 */
>> +	offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
>> +	if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
>> +		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) {
>> +			pr_debug("Drop incomplete fragment\n");
>> +			return 0;
>> +		}
>> +	}
> This code looks very similar to 2efdaaaf883a.
>
> Would you wrap it in a function as call it use to reuse it? Something
> like this sketch?
>
> static bool ipv6_frag_validate(struct sk_buff *skb, ...)
> {
>         ...
>
> 	offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
> 	if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
> 		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;
> }
>
> then, from ipv6:
>
>         if (!ipv6_frag_validate(skb, ...)) {
>                 __IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
>                                         IPSTATS_MIB_INHDRERRORS);
>                 icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
>                 reurn -1;
>         }
>
> and from netfilter:
>
>         if (!ipv6_frag_validate(skb, ...))
>                 return -1;
>
> Thanks.
>
Thanks for feedback, I will do as suggested.

>>  	if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
>>  		return -ENOMEM;
>>  
>> -- 
>> 2.10.2
>>