netdev - Re: [PATCHv2 net 2/2] IPv6: reply ICMP error if the first fragment don't include 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: <CA+FuTSdCG4yVDb85M=fChfrkU9=F7j88TJujJy_y0pv-Ks_MwQ@mail.gmail.com>
Date:   Wed, 21 Oct 2020 10:02:55 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCHv2 net 2/2] IPv6: reply ICMP error if the first fragment
 don't include all headers

On Wed, Oct 21, 2020 at 12:20 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> Based on RFC 8200, Section 4.5 Fragment Header:
>
>   -  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.
>
> As the packet may be any kind of L4 protocol, I only checked if there
> has Upper-Layer header by (offset + 1) > skb->len. Checking each packet
> header in IPv6 fast path will have performace impact, so I put the

nit: performa[n]ce

> checking in ipv6_frag_rcv().
>
> When send ICMP error message, if the first truncated fragment is ICMP
> message, icmp6_send() will break as is_ineligible() return true. So I
> added a check in is_ineligible() to let fragment packet with nexthdr
> ICMP but no ICMP header return false.
>
> v2:
> a) Move header check to ipv6_frag_rcv(). Also check the ipv6_skip_exthdr()
>    return value
> b) Fix ipv6_find_hdr() parameter type miss match in is_ineligible()
>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  net/ipv6/icmp.c       | 13 ++++++++++++-
>  net/ipv6/reassembly.c | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index ec448b71bf9a..50d28764c8dd 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -145,7 +145,9 @@ static bool is_ineligible(const struct sk_buff *skb)
>         int ptr = (u8 *)(ipv6_hdr(skb) + 1) - skb->data;
>         int len = skb->len - ptr;
>         __u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> +       unsigned int offs = 0;
>         __be16 frag_off;
> +       bool is_frag;
>
>         if (len < 0)
>                 return true;
> @@ -153,12 +155,21 @@ static bool is_ineligible(const struct sk_buff *skb)
>         ptr = ipv6_skip_exthdr(skb, ptr, &nexthdr, &frag_off);
>         if (ptr < 0)
>                 return false;
> +
> +       is_frag = (ipv6_find_hdr(skb, &offs, NEXTHDR_FRAGMENT, NULL, NULL) == NEXTHDR_FRAGMENT);
> +

ipv6_skip_exthdr already walks all headers. Should we not already see
frag_off != 0 if skipped over a fragment header? Analogous to the test
in ipv6_frag_rcv below.

>         if (nexthdr == IPPROTO_ICMPV6) {
>                 u8 _type, *tp;
>                 tp = skb_header_pointer(skb,
>                         ptr+offsetof(struct icmp6hdr, icmp6_type),
>                         sizeof(_type), &_type);
> -               if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
> +
> +               /* Based on RFC 8200, Section 4.5 Fragment Header, return
> +                * false if this is a fragment packet with no icmp header info.
> +                */
> +               if (!tp && is_frag)
> +                       return false;
> +               else if (!tp || !(*tp & ICMPV6_INFOMSG_MASK))
>                         return true;
>         }
>         return false;
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 1f5d4d196dcc..b359bffa2f58 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -322,7 +322,9 @@ 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);
> -       int iif;
> +       __be16 frag_off;
> +       int iif, offset;
> +       u8 nexthdr;
>
>         if (IP6CB(skb)->flags & IP6SKB_FRAGMENTED)
>                 goto fail_hdr;
> @@ -351,6 +353,20 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
>                 return 1;
>         }
>
> +       /* RFC 8200, Section 4.5 Fragment Header:
> +        * 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.
> +        */
> +       nexthdr = hdr->nexthdr;
> +       offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
> +       if (offset >= 0 && frag_off == htons(IP6_MF) && (offset + 1) > skb->len) {

Offset +1 does not fully test "all headers through an upper layer
header". You note the caveat in your commit message. Perhaps for the
small list of common protocols at least use a length derived from
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;
>         fq = fq_find(net, fhdr->identification, hdr, iif);
>         if (fq) {
> --
> 2.25.4
>