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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

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

(b) the name is a bit broad, how about ipv6_frag_thdr_tuncated() or
    some such?

> +{
> +	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;




[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux