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

Date:   Sun, 29 May 2022 09:32:08 +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 Sun May 29, 2022 at 8:14 AM CEST, Maciej Żenczykowski wrote:
> On Sat, May 28, 2022 at 6:07 PM Riccardo Paolo Bestetti <pbl@...tov.io> wrote:
> > I confirm that, indeed, it was unintended.
>
> Good to hear this.
>
> > 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?)
>
> I think sending with ping multicast/broadcast source is unlikely to get replies
> due to valid worries that it is an attempt at a multiplication DoS attack.
> (send one unicast packet to target, get target to reply with broadcast 'storm')

I agree. I find it likely that other implementation also would disallow
to bind on such addresses. I'll see if I can find out more this week.

>
> > 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.
>
> I'm failing to see a way to write it in a more obvious way...
>
> ipv4_is_zeroaddr() should probably be tree-wide renamed to ipv4_addr_any()
> to match ipv6_addr_any() which now saves the same purpose.
> [since it's just a check for 0.0.0.0/32 now after Dave Taht's commit]
>
> Then the following:
>
> if (ipv4_is_zeronet(addr) || ipv4_is_lbcast(addr))
> return RTN_BROADCAST;
>
> is more immediately weird.
>
> Why do we classify INADDR_ANY as broadcast?

That's what I was also wondering. But inet_addr_type() is not only
referenced 8 times inside the kernel, but also exported. So ultimately
the reason for returning RTN_BROADCAST is irrelevant as it's here to
stay. That's probably why the original code was "correcting" its
semantics on the fly by explicitly checking for 0.0.0.0 and remarketing
it as RTN_LOCAL.

As for ipv4_is_zeronet() vs ipv4_addr_any(), I do agree ipv4_addr_any()
is more consistent with other stuff and a better name by itself.

> It should either be classified as a new RTN_ADDR_ANY or as RTN_LOCAL,
> with the understanding that in practice 0.0.0.0/32 just means "don't care
> assign something for me".

Again, inet_addr_type() is exported...

>
> I guess one could do:
>
>  if (!ipv4_addr_any() && (!inet_can_nonlocal_bind(net, isk) &&
>                     chk_addr_ret != RTN_LOCAL) ||
>                    chk_addr_ret == RTN_MULTICAST
>                    chk_addr_ret == RTN_BROADCAST))
>                         return -EADDRNOTAVAIL;
>
> But that's not really any more meaningfully readable than the old code.

I think this is already significantly better than checking for 0.0.0.0
separately and changing chk_addr_ret depending on that. I also feel that
a comment, in this case, might go a long way. And all the
nonlocal-is-allowed-or-address-is-local logic could go in one line.
Maybe along the lines of:

/* never accept multicast and broadcast addresses */
if (!ipv4_addr_any() &&
    (chk_addr_ret != RTN_LOCAL && !inet_can_nonlocal_bind(...)) ||
    chk_addr_ret == RTN_MULTICAST || chk_addr_ret == RTN_BROADCAST))
                         return -EADDRNOTAVAIL;

This, to me, is loads more readable than the pre-5.17 code.

Riccardo P. Bestetti

>
> > 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.