Message-ID: <CA+FuTSdOHYF-7rcjuuSraJjnAhcToV+tTvvj=JGCN0v6HZu_Kw@mail.gmail.com>
Date:   Thu, 16 Jul 2020 10:09:20 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net-next v2] icmp: support rfc 4884

On Fri, Jul 10, 2020 at 9:29 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> From: Willem de Bruijn <willemb@...gle.com>
>
> Add setsockopt SOL_IP/IP_RECVERR_4884 to return the offset to an
> extension struct if present.
>
> ICMP messages may include an extension structure after the original
> datagram. RFC 4884 standardized this behavior. It stores the offset
> in words to the extension header in u8 icmphdr.un.reserved[1].
>
> The field is valid only for ICMP types destination unreachable, time
> exceeded and parameter problem, if length is at least 128 bytes and
> entire packet does not exceed 576 bytes.
>
> Return the offset to the start of the extension struct when reading an
> ICMP error from the error queue, if it matches the above constraints.
>
> Do not return the raw u8 field. Return the offset from the start of
> the user buffer, in bytes. The kernel does not return the network and
> transport headers, so subtract those.
>
> Also validate the headers. Return the offset regardless of validation,
> as an invalid extension must still not be misinterpreted as part of
> the original datagram. Note that !invalid does not imply valid. If
> the extension version does not match, no validation can take place,
> for instance.
>
> For backward compatibility, make this optional, set by setsockopt
> SOL_IP/IP_RECVERR_RFC4884. For API example and feature test, see
> github.com/wdebruij/kerneltools/blob/master/tests/recv_icmp_v2.c
>
> For forward compatibility, reserve only setsockopt value 1, leaving
> other bits for additional icmp extensions.
>
> Changes
>   v1->v2:
>   - convert word offset to byte offset from start of user buffer
>     - return in ee_data as u8 may be insufficient
>   - define extension struct and object header structs
>   - return len only if constraints met
>   - if returning len, also validate
>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>

Tom, Eric, does this address your concerns from v1?

To summarize:

- cleaner api
    - only return offset if rfc 4884 constraints met
    - return as byte offset from start of user buffer, not raw packet off
    - define self-describing new union in sock_extended_err
    - define rfc 4884 extension struct and objects in uapi

- validate
    - verify checksum
    - verify object boundaries

Does not

- validate individual users from subsequent RFCs: a number of classes
and subtypes are defined in an IANA registry [1]. But unlike rfc 4884
those are at best proposed standards, some not updated since 2015. I
don't think those are mature enough to encode in the kernel ABI.

- truncate packet for socket matching: I think that should be a
separate (stable) commit if a real issue. Personally, I'm not
convinced yet that it is. The 128B original datagram + outer header
minimum offset concerns IPv4, where packet is at most 576B. For IPv6,
which has the extension header issue, the relevant rfc 4443 states
that that the length should be 1280. More importantly, it would take a
malicious/buggy sender to craft a packet with an extension header that
overlaps the headers. But it does not need to mess with the extension
header offset field to create such a payload to begin with.

[1] https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-ext-classes

Powered by blists - more mailing lists