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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201104134118.GA28789@salvia>
Date:   Wed, 4 Nov 2020 14:41:18 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Georg Kohmann <geokohma@...co.com>
Cc:     netdev@...r.kernel.org, kadlec@...filter.org, fw@...len.de,
        davem@...emloft.net, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        kuba@...nel.org, netfilter-devel@...r.kernel.org,
        coreteam@...filter.org
Subject: Re: [PATCH net] ipv6/netfilter: Discard first fragment not including
 all headers

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

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