| Message ID | 20201111115025.28879-1-geokohma@cisco.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v4] ipv6/netfilter: Discard first fragment not including all headers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3180 this patch: 3180 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3544 this patch: 3544 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, 11 Nov 2020 12:50:25 +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_thdr_truncated() to check for truncation in the upper layer > header. This validation 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_thdr_truncated() 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") > > Signed-off-by: Georg Kohmann <geokohma@cisco.com> LGTM, awaiting ack from Pablo.
On Wed, Nov 11, 2020 at 12:50:25PM +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_thdr_truncated() to check for truncation in the upper layer > header. This validation 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_thdr_truncated() 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") > > Signed-off-by: Georg Kohmann <geokohma@cisco.com> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org> In case you would like to follow up with another patch for the IPv6 reassembly in netfilter.o net/ipv6/netfilter/nf_conntrack_reasm.c uses pr_debug() everywhere. net/ipv4/netfilter/nf_defrag_ipv4.c however uses ip_defrag() which is updating IPSTATS_MIB_*, so IPv4 and IPv6 code behave differently with regards to the stats. It would be probably good to get them aligned, by replacing the existing pr_debug() in the net/ipv6/netfilter/nf_conntrack_reasm.c code by IPSTATS_MIB_*. Thanks.
On Sun, 15 Nov 2020 12:15:20 +0100 Pablo Neira Ayuso wrote: > On Wed, Nov 11, 2020 at 12:50:25PM +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_thdr_truncated() to check for truncation in the upper layer > > header. This validation 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_thdr_truncated() 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") > > > > Signed-off-by: Georg Kohmann <geokohma@cisco.com> > > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org> Applied, thanks!
Hi, On Wed, Nov 11, 2020 at 12:50:25PM +0100, Georg Kohmann wrote: [...] > diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c > index c8cf1bb..e3869ba 100644 > --- a/net/ipv6/reassembly.c > +++ b/net/ipv6/reassembly.c > @@ -318,15 +318,43 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb, > return -1; > } > > +/* Check if the upper layer header is truncated in the first fragment. */ > +bool ipv6_frag_thdr_truncated(struct sk_buff *skb, int start, u8 *nexthdrp) Please, follow up and send a patch to place this function in include/net/ipv6_frag.h as static inline. See: https://marc.info/?l=netfilter-devel&m=160571942728516&w=2
On 18.11.2020 19:16, Pablo Neira Ayuso wrote: > Hi, > > On Wed, Nov 11, 2020 at 12:50:25PM +0100, Georg Kohmann wrote: > [...] >> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c >> index c8cf1bb..e3869ba 100644 >> --- a/net/ipv6/reassembly.c >> +++ b/net/ipv6/reassembly.c >> @@ -318,15 +318,43 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb, >> return -1; >> } >> >> +/* Check if the upper layer header is truncated in the first fragment. */ >> +bool ipv6_frag_thdr_truncated(struct sk_buff *skb, int start, u8 *nexthdrp) > Please, follow up and send a patch to place this function in > include/net/ipv6_frag.h as static inline. > > See: https://marc.info/?l=netfilter-devel&m=160571942728516&w=2 Thanks for the link to the similar problem. I have been looking into this all day. I am working on a patch now.
diff --git a/include/net/ipv6.h b/include/net/ipv6.h index bd1f396..637cc6d 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_thdr_truncated(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/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c index 054d287..b9cc0b3 100644 --- a/net/ipv6/netfilter/nf_conntrack_reasm.c +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c @@ -440,6 +440,7 @@ 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; + u8 nexthdr = NEXTHDR_FRAGMENT; int fhoff, nhoff, ret; struct frag_hdr *fhdr; struct frag_queue *fq; @@ -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_thdr_truncated(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..e3869ba 100644 --- a/net/ipv6/reassembly.c +++ b/net/ipv6/reassembly.c @@ -318,15 +318,43 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct sk_buff *skb, return -1; } +/* Check if the upper layer header is truncated in the first fragment. */ +bool ipv6_frag_thdr_truncated(struct sk_buff *skb, int start, u8 *nexthdrp) +{ + u8 nexthdr = *nexthdrp; + __be16 frag_off; + int offset; + + offset = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off); + if (offset < 0 || (frag_off & htons(IP6_OFFSET))) + return false; + 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 true; + return false; +} +EXPORT_SYMBOL(ipv6_frag_thdr_truncated); + static int ipv6_frag_rcv(struct sk_buff *skb) { struct frag_hdr *fhdr; 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; u8 nexthdr; + int iif; if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED) goto fail_hdr; @@ -362,24 +390,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_thdr_truncated(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;
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_thdr_truncated() to check for truncation in the upper layer header. This validation 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_thdr_truncated() 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") Signed-off-by: Georg Kohmann <geokohma@cisco.com> --- Notes: v2: Wrap fragment validation code into exthdrs_code.c for use by both ipv6 and netfiter. v3: Remove unused variable frag_off from ipv6_frag_rcv(). v4: a) Rename ipv6_frag_validate() to ipv6_frag_thdr_truncated() and invert returned bool values. Refrazing function comment. b) Move ipv6_frag_thdr_truncated() to reassembly.c c) Reverse conditions to return early and reduce indentation after ipv6_skip_exthdr() call. include/net/ipv6.h | 2 ++ net/ipv6/netfilter/nf_conntrack_reasm.c | 9 ++++++ net/ipv6/reassembly.c | 55 +++++++++++++++++++++------------ 3 files changed, 46 insertions(+), 20 deletions(-)