netdev - REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP

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

Message-ID: <CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com>
Date:   Sat, 28 May 2022 01:48:13 -0700
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Riccardo Paolo Bestetti <pbl@...tov.io>
Cc:     David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>,
        Lorenzo Colitti <lorenzo@...gle.com>,
        Linux NetDev <netdev@...r.kernel.org>
Subject: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

The Android net test framework fails (on 5.17+ but not on 5.16) due to:

  commit 8ff978b8b222bc9d51dd109a46b51026336c95d8
  ipv4/raw: support binding to nonlocal addresses

(quoting relevant portions)

+++ b/include/net/inet_sock.h
+static inline bool inet_addr_valid_or_nonlocal(struct net *net,
+                                              struct inet_sock *inet,
+                                              __be32 addr,
+                                              int addr_type)
+{
+       return inet_can_nonlocal_bind(net, inet) ||
+               addr == htonl(INADDR_ANY) ||
+               addr_type == RTN_LOCAL ||
+               addr_type == RTN_MULTICAST ||
+               addr_type == RTN_BROADCAST;
+}
+

+++ b/net/ipv4/ping.c
@@ -311,15 +311,11 @@ static int ping_check_bind_addr(struct sock *sk,
struct inet_sock *isk,
                pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n",
                         sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port));

-               if (addr->sin_addr.s_addr == htonl(INADDR_ANY))
-                       chk_addr_ret = RTN_LOCAL;
-               else
-                       chk_addr_ret = inet_addr_type(net,
addr->sin_addr.s_addr);
-
-               if ((!inet_can_nonlocal_bind(net, isk) &&
-                    chk_addr_ret != RTN_LOCAL) ||
-                   chk_addr_ret == RTN_MULTICAST ||   <---  note this
was == not !=
-                   chk_addr_ret == RTN_BROADCAST)   <---- ditto
+               chk_addr_ret = inet_addr_type(net, addr->sin_addr.s_addr);
+
+               if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk),
+                                                addr->sin_addr.s_addr,
+                                                chk_addr_ret))
                        return -EADDRNOTAVAIL;

The test case is:
  sudo bash -c "echo 0 $[0x7FFFFFFF] > /proc/sys/net/ipv4/ping_group_range"

  python3 <<-EOF
  import socket
  s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_ICMP)
  s.bind(("255.255.255.255", 1026))
  EOF

This is *expected* to fail with ENOADDRAVAIL (and used to), but now succeeds.
(ie. OSError: [Errno 99] Cannot assign requested address )

Looking at the commit message of the above commit, this change in behaviour
isn't actually described as something it does... so it might be an
unintended consequence (ie. bug).

I can easily relax the test to skip this test case on 5.17+...
although I'm not entirely certain
we don't depend on this somewhere... While I sort of doubt that, I
wonder if this has some security implications???.

My main problem is that binding the source of a ping socket to a
multicast or broadcast address does seem pretty bogus...
and this is not something I would want unprivileged users to be able to do...

I've verified reverting the net/ipv4/ping.c chunk of the above commit
does indeed fix the testcase.

Thoughts? Skip test? or fix the kernel to disallow it?

Maciej Żenczykowski, Kernel Networking Developer @ Google