| Message ID | 20201104130128.14619-1-geokohma@cisco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net] ipv6/netfilter: Discard first fragment not including all headers | expand |
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. > + > if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr))) > return -ENOMEM; > > -- > 2.10.2 >
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 >>
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; + } + } + if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr))) return -ENOMEM;
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(-)