| 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
| ||
|
Message-ID: <CA+FuTSeBRCDcu7uKp9=7UZWR3zmSrk41ArqrseW9jHYgK+WPpg@mail.gmail.com>
Date: Sun, 28 Mar 2021 13:00:27 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Andreas Roeseler <andreas.a.roeseler@...il.com>
Cc: Network Development <netdev@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
David Ahern <dsahern@...nel.org>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next V5 6/6] icmp: add response to RFC 8335 PROBE messages
On Wed, Mar 24, 2021 at 2:20 PM Andreas Roeseler
<andreas.a.roeseler@...il.com> wrote:
>
> Modify the icmp_rcv function to check PROBE messages and call icmp_echo
> if a PROBE request is detected.
>
> Modify the existing icmp_echo function to respond ot both ping and PROBE
> requests.
>
> This was tested using a custom modification to 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).
>
> The modification to the iputils package is still in development and can
> be found here: https://github.com/Juniper-Clinic-2020/iputils.git. It
> supports full sending functionality of PROBE requests, but currently
> does not parse the response messages, which is why Wireshark is required
> to verify the sent and recieved PROBE messages. The modification adds
> the ``-e'' flag to the command which allows the user to specify the
> interface identifier to query the probed host. An example usage would be
> <./ping -4 -e 1 [destination]> to send a PROBE request of ifindex 1 to the
> destination node.
>
> Signed-off-by: Andreas Roeseler <andreas.a.roeseler@...il.com>
> static bool icmp_echo(struct sk_buff *skb)
> {
> + struct icmp_ext_hdr *ext_hdr, _ext_hdr;
> + struct icmp_ext_echo_iio *iio, _iio;
> + struct icmp_bxm icmp_param;
> + struct net_device *dev;
> + char buff[IFNAMSIZ];
> struct net *net;
> + u16 ident_len;
> + u8 status;
>
> net = dev_net(skb_dst(skb)->dev);
> - if (!net->ipv4.sysctl_icmp_echo_ignore_all) {
> - struct icmp_bxm icmp_param;
> + /* should there be an ICMP stat for ignored echos? */
> + if (net->ipv4.sysctl_icmp_echo_ignore_all)
> + return true;
> +
> + icmp_param.data.icmph = *icmp_hdr(skb);
> + icmp_param.skb = skb;
> + icmp_param.offset = 0;
> + icmp_param.data_len = skb->len;
> + icmp_param.head_len = sizeof(struct icmphdr);
>
> - icmp_param.data.icmph = *icmp_hdr(skb);
> + if (icmp_param.data.icmph.type == ICMP_ECHO) {
> icmp_param.data.icmph.type = ICMP_ECHOREPLY;
> - icmp_param.skb = skb;
> - icmp_param.offset = 0;
> - icmp_param.data_len = skb->len;
> - icmp_param.head_len = sizeof(struct icmphdr);
> - icmp_reply(&icmp_param, skb);
> + goto send_reply;
> }
> - /* should there be an ICMP stat for ignored echos? */
> - return true;
> + if (!net->ipv4.sysctl_icmp_echo_enable_probe)
> + return true;
> + /* We currently only support probing interfaces on the proxy node
> + * Check to ensure L-bit is set
> + */
> + if (!(ntohs(icmp_param.data.icmph.un.echo.sequence) & 1))
> + return true;
> + /* Clear status bits in reply message */
> + icmp_param.data.icmph.un.echo.sequence &= htons(0xFF00);
> + icmp_param.data.icmph.type = ICMP_EXT_ECHOREPLY;
> + ext_hdr = skb_header_pointer(skb, 0, sizeof(_ext_hdr), &_ext_hdr);
> + iio = skb_header_pointer(skb, sizeof(_ext_hdr), sizeof(_iio), &_iio);
This requires that the packet holds a full sizeof(_iio) that is
capable of storing an ipv6 address regardless of the class_type. That
is not required by the spec, I assume.
If not requiring that, do have to do bounds checking for each
individual case, e.g., that an ifname fits in the packet if that may
be shorter than IFNAMSIZ.
> + if (!ext_hdr || !iio)
> + goto send_mal_query;
> + if (ntohs(iio->extobj_hdr.length) <= sizeof(iio->extobj_hdr))
> + goto send_mal_query;
> + ident_len = ntohs(iio->extobj_hdr.length) - sizeof(iio->extobj_hdr);
As asked in v3: this can have negative overflow?
> + status = 0;
> + dev = NULL;
> + switch (iio->extobj_hdr.class_type) {
> + case EXT_ECHO_CTYPE_NAME:
> + if (ident_len >= IFNAMSIZ)
> + goto send_mal_query;
> + memset(buff, 0, sizeof(buff));
> + memcpy(buff, &iio->ident.name, ident_len);
> + dev = dev_get_by_name(net, buff);
> + break;
> + case EXT_ECHO_CTYPE_INDEX:
> + if (ident_len != sizeof(iio->ident.ifindex))
> + goto send_mal_query;
> + dev = dev_get_by_index(net, ntohl(iio->ident.ifindex));
> + break;
> + case EXT_ECHO_CTYPE_ADDR:
> + if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
> + iio->ident.addr.ctype3_hdr.addrlen)
> + goto send_mal_query;
> + switch (ntohs(iio->ident.addr.ctype3_hdr.afi)) {
> + case ICMP_AFI_IP:
> + if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
> + sizeof(struct in_addr))
> + goto send_mal_query;
> + dev = ip_dev_find(net, iio->ident.addr.ip_addr.ipv4_addr.s_addr);
> + break;
> +#if IS_ENABLED(CONFIG_IPV6)
> + case ICMP_AFI_IP6:
> + if (ident_len != sizeof(iio->ident.addr.ctype3_hdr) +
> + sizeof(struct in6_addr))
> + goto send_mal_query;
> + rcu_read_lock();
> + dev = ipv6_stub->ipv6_dev_find(net, &iio->ident.addr.ip_addr.ipv6_addr, dev);
> + if (dev)
> + dev_hold(dev);
> + rcu_read_unlock();
This rcu read-size critical is not needed, because the entire receive
path is wrapped in such a section. See, for instance,
netif_receive_skb_core.
Either that, or the __in_dev_get_rcu and rcu_deference below would
require a similar critical section.
Powered by blists - more mailing lists