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: <CANP3RGcBjYL0hpd-J_GvXCJsbOg3ztS5yhXr4S8M5G5_F1ZwLQ@mail.gmail.com>
Date:   Sat, 28 May 2022 23:14:19 -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: Re: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255
 without IP_TRANSPARENT

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, 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?
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".

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.

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