netdev - Re: 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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <CKBUCV5XNA5W.1WFEM5DTPSCHV@enhorning>
Date:   Sun, 29 May 2022 03:06:55 +0200
From:   "Riccardo Paolo Bestetti" <pbl@...tov.io>
To:     Maciej Żenczykowski <maze@...gle.com>
Cc:     "David Ahern" <dsahern@...nel.org>,
        "Jakub Kicinski" <kuba@...nel.org>,
        "Lorenzo Colitti" <lorenzo@...gle.com>,
        "Linux NetDev" <netdev@...r.kernel.org>
Subject: Re: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255
 without IP_TRANSPARENT

On Sat May 28, 2022 at 10:48 AM CEST, Maciej Żenczykowski wrote:
> [...]
> 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 confirm that, indeed, it was unintended. While my patch was getting
reviewed, it was reported by someone that 0ce779a9f501 introduced the
following logic, which I removed in my patch:

-               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);

I proposed a revert of ping.c, but as the reporter did not respond to
the request to clarify, the patch was applied as it was. Needless to
say, the point is now clear. :)

Digging a bit further, it seems that the logic was actually already
implemented in c319b4d76b9, the original ICMP socket implementation.
Nothing about the behaviour of broadcast and multicast bind addresses is
mentioned in the commit message or linked Mac OS X documentation
(although it would be interesting to test how Mac OS X behaves - anyone
with a Mac around here that can do that?)

To recap the pre-5.17 and 5.17+ behaviours, without IP_TRANSPARENT
(and similar flags):
+--------------------------------------------+
| addr                         | pre | 5.17+ |
+------------------------------+-----+-------+
| 0.0.0.0 / any                | OK  | OK    |
| 255.255.255.255 / broadcast  | ERR | OK :( |
| 224.x.x.x / multicast        | ERR | OK :( |
| address present              | OK  | OK    |
| address absent               | ERR | ERR   |
+------------------------------+-----+-------+


>
> 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?
I agree, it doesn't make sense to be able to do that. That's probably
why the check was done that way in the first place. I think the previous
behaviour should be restored. Not by reverting (part of) the patch,
because honestly the original code sucked, but by rewriting it properly.
And more importantly I think that test cases for that should be added in
the kernel (this has been in two released minor versions before even
being caught...)

I should be able to roll up a patch inside a few days, if this sounds
like a good approach to everyone.

Riccardo P. Bestetti