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