REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

[Maciej Żenczykowski]

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

Re: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

[Riccardo Paolo Bestetti]

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

Re: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

[Maciej Żenczykowski]

On Sat, May 28, 2022 at 6:07 PM Riccardo Paolo Bestetti <pbl@bestov.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.

Re: REGRESSION?? ping ipv4 sockets and binding to 255.255.255.255 without IP_TRANSPARENT

[Riccardo Paolo Bestetti]

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