netdev - Re: [PATCH V3 net-next 5/5] icmp: add response to RFC 8335 PROBE messages

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] [day] [month] [year] [list]

Date:   Thu, 25 Feb 2021 12:38:45 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Andreas Roeseler <andreas.a.roeseler@...il.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        David Miller <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        dsahern@...nel.org, Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel test robot <lkp@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH V3 net-next 5/5] icmp: add response to RFC 8335 PROBE messages

On Wed, Feb 24, 2021 at 6:21 PM Andreas Roeseler
<andreas.a.roeseler@...il.com> wrote:
>
> On Sun, 2021-02-21 at 23:49 -0500, Willem de Bruijn wrote:
> On Wed, Feb 17, 2021 at 1:14 PM Andreas Roeseler
> <andreas.a.roeseler@...il.com> wrote:
> >
> > Modify the icmp_rcv function to check for PROBE messages and call
> > icmp_echo if a PROBE request is detected.
> >
> > Modify the existing icmp_echo function to respond to both ping and
> > PROBE
> > requests.
> >
> > This was tested using a custom modification of the iputils package
> > and
> > wireshark. It supports IPV4 probing by name, ifindex, and probing by
> > both IPV4 and IPV6
> > addresses. It currently does not support responding to probes off the
> > proxy node
> > (See RFC 8335 Section 2).
> >
> > Signed-off-by: Andreas Roeseler <andreas.a.roeseler@...il.com>
> > ---
> > Changes since v1:
> >  - Reorder variable declarations to follow coding style
> >  - Switch to functions such as dev_get_by_name and ip_dev_find to
> > lookup
> >    net devices
> >
> > Changes since v2:
> > Suggested by Willem de Brujin <willemdebrujin.kernel@...il.com>
> >  - Add verification of incoming messages before looking up netdev
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> >  - Include net/addrconf.h library for ipv6_dev_find


> > +               if (!buff)
> > +                       return -ENOMEM;
> > +               memcpy(buff, &iio->ident.name, ident_len);
> > +               dev = dev_get_by_name(net, buff);
> > +               kfree(buff);
> > +               break;
> > +       case EXT_ECHO_CTYPE_INDEX:
> > +               if (ident_len != sizeof(iio->ident.ifIndex)) {
>
> this checks that length is 4B, but RFC says "If the Interface
> Identification Object identifies the probed interface by index, the
> length is equal to 8 and the payload contains the if-index"
>
> ident_len stores the value of the identifier of the interface only,
> i.e. it stores the length of the iio minus the length of the iio
> header. Therefore, we can check its size against the expected size of
> an if_Index (4 octets)

Great. Thanks for clarifying.

> > @@ -1096,7 +1200,6 @@ int icmp_rcv(struct sk_buff *skb)
> >         if (icmph->type > NR_ICMP_TYPES)
> >                 goto error;
> >
> > -
> >         /*
> >          *      Parse the ICMP message
> >          */
> > @@ -1123,6 +1226,7 @@ int icmp_rcv(struct sk_buff *skb)
> >
> >         success = icmp_pointers[icmph->type].handler(skb);
> >
> > +success_check:
> >         if (success)  {
> >                 consume_skb(skb);
> >                 return NET_RX_SUCCESS;
> > @@ -1136,6 +1240,13 @@ int icmp_rcv(struct sk_buff *skb)
> >  error:
> >         __ICMP_INC_STATS(net, ICMP_MIB_INERRORS);
> >         goto drop;
> > +probe:
> > +       /*
> > +        * We can't use icmp_pointers[].handler() because the codes
> > for PROBE
> > +        *   messages are 42 or 160
> > +        */
>
> ICMPv6 message 160 (ICMPV6_EXT_ECHO_REQUEST) must be handled in
> icmpv6_rcv, not icmp_rcv. Then the ICMPv4 message 42 can be handled in
> the usual way.
>
>
> You are correct that we should handle ICMPV6_EXT_ECHO_REQUEST in the
> icmpv6.c file, but shouldn't we still have a special handler for the
> ICMPv4 message? The current icmp_pointers[].handler is an array of size
> NR_ICMP_TYPES + 1 (or 19 elements), so I don't think it would be a good
> idea to extend it to 42.

Interesting. So almost all numbers between NR_ICMP_TYPES (18) and 42
are deprecated:

  https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml

Eventually we might get more extensions after 42. So you can go either way.
The table is the clean approach. But I see the practical point of extending it
for one case, too. The current branch approach looks fine to me.