netdev - Re: [PATCH] icmp: support rfc 5837

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTSfcqotSBVtEWhbC8LdjDyOrsfuSv1QHt1Ga+s01phbmaA@mail.gmail.com>
Date:   Thu, 11 Mar 2021 21:47:45 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     ishaangandhi <ishaangandhi@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] icmp: support rfc 5837

On Thu, Mar 11, 2021 at 7:47 PM ishaangandhi <ishaangandhi@...il.com> wrote:
>
> From: Ishaan Gandhi <ishaangandhi@...il.com>
>
> This patch identifies the interface a packet arrived on when sending
> ICMP time exceeded, destination unreachable, and parameter problem
> messages, in accordance with RFC 5837.
>
> It was tested by pinging a machine with a ttl of 1, and observing the
> response in Wireshark.
>
> Signed-off-by: Ishaan Gandhi <ishaangandhi@...il.com>

Such additional optional data (MAY in RFC 5837) should be optional and
off by default.

See also the security considerations in Sec 6. Those suggestions are
too fine grained to implement, but at the least a sysctl to
disable/enable the entire feature.


> +void icmp_identify_arrival_interface(struct sk_buff *skb, struct net *net, int room,
> +                                    struct icmphdr *icmph)
> +{
> +       unsigned int ext_len, if_index, orig_len, offset, extra_space_needed,
> +                    word_aligned_orig_len, mtu, name_len, name_subobj_len;
> +       struct interface_ipv4_addr_sub_obj ip_addr;
> +       struct icmp_extobj_hdr *iio_hdr;
> +       struct icmp_ext_hdr *ext_hdr;
> +       struct net_device *dev;
> +       void *subobj_offset;
> +       char *name, ctype;
> +
> +       skb_linearize(skb);
> +       if_index = inet_iif(skb);

skb->skb_iif is overwritten in __netif_receive_skb on each round, so
this will not necessarily point to the physical device on which the
packet arrived.

> +               name = dev->name;
> +               if (name) {
> +                       name_len = strlen(name);
> +                       name_subobj_len = min_t(unsigned int, name_len, ICMP_5837_MAX_NAME_LEN) + 1;

device name length is always <= IFNAMSIZ (incl terminating 0) <
ICMP_5837_MAX_NAME_LEN

> +                       name_subobj_len = (name_subobj_len + 3) & ~0x03;
> +                       ctype |= ICMP_5837_NAME_CTYPE;
> +                       ext_len += name_subobj_len;
> +               }
> +
> +               mtu = dev->mtu;
> +               if (mtu) {

always true

> +                       ctype |= ICMP_5837_MTU_CTYPE;
> +                       ext_len += 4;

sizeof(__be32)

>  /*
>   *     Send an ICMP message in response to a situation
>   *
> @@ -731,6 +864,10 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>                 room = 576;
>         room -= sizeof(struct iphdr) + icmp_param.replyopts.opt.opt.optlen;
>         room -= sizeof(struct icmphdr);
> +       if (type == ICMP_DEST_UNREACH || type == ICMP_TIME_EXCEEDED ||
> +           type == ICMP_PARAMETERPROB) {
> +               icmp_identify_arrival_interface(skb_in, net, room, &icmp_param.data.icmph);
> +       }

If adding a feature for ICMP that is also valid for ICMPv6, please
introduce to both protocols in the same patch series. IPv6 is a first
class citizen.