| 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
| ||
|
Date: Mon, 9 Nov 2020 12:50:09 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Georg Kohmann <geokohma@...co.com>
Cc: netdev@...r.kernel.org, pablo@...filter.org, kadlec@...filter.org,
fw@...len.de, davem@...emloft.net, kuznet@....inr.ac.ru,
yoshfuji@...ux-ipv6.org, netfilter-devel@...r.kernel.org,
coreteam@...filter.org
Subject: Re: [PATCH net v3] ipv6/netfilter: Discard first fragment not
including all headers
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.
(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;
Powered by blists - more mailing lists