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+FuTSfQrSqDau=e7-MXNannZ8kCqKizEMX5KZD4OzNVkyMeiA@mail.gmail.com>
Date:   Thu, 22 Oct 2020 11:46:50 -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 Thu, Oct 22, 2020 at 5:12 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> Hi Willem,
>
> Thanks for the comments, see replies below.
>
> On Wed, Oct 21, 2020 at 10:02:55AM -0400, Willem de Bruijn wrote:
> > > +       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.
>
> Ah, yes, I forgot we can use this check.
>
> > > +       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?
>
> Do you mean check the header like
>
> if (nexthdr == IPPROTO_ICMPV6)
>         offset = offset + seizeof(struct icmp6hdr);
> else if (nexthdr == ...)
>         offset = ...
> else
>         offset += 1;
>
> if (frag_off == htons(IP6_MF) && offset > skb->len) {
>         icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
>         return -1;
> }
>
> Another questions is how to define the list, does TCP/UDP/SCTP/ICMPv6 enough?

Exactly. But only if it's possible without adding a ton of #include's.
It is best effort.

If feasible, TCP + UDP alone would suffice to cover most traffic.