unintended ipv4 broadcast policy change

unintended ipv4 broadcast policy change

[David Miller]

The context is that I'm looking into cleaning up up the mess we have
wrt. DHCP listening on packet sockets and (in some cases) seeing every
packet that hits the system.

Anyways, back in 2007 this commit was made:

commit 8030f54499925d073a88c09f30d5d844fb1b3190
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu Feb 22 01:53:47 2007 +0900

    [IPV4] devinet: Register inetdev earlier.

So now every net device registered has inetdev_init() called on it.

This has a subtle policy side effect that has some interesting
implications.  The route input slow path has this check:

	/* IP on this device is disabled. */

	if (!in_dev)
		goto out;

But now this will never, ever, be true.

Which means that previously we would not accept even broadcast
or multicast packets on an interface that hasn't had at least
one IP address configured.

Now we will.

I think we have a hard decision to make.  One option is to
fix the input routing check, by changing it to test if the
ipv4 address list is empty.

The second option is to remove the check entirely and keep the
new behavior.

This subtle new behavior is interesting because it means that
a DHCP client could be implemented entirely with plain UDP
sockets.

Contrary to previous discussions I see no code in the ISC dhcp
client that needs to get at the MAC address.  It merely wants
it there so that it's packet parsing engine can skip over it
to get at the ipv4/UDP bits.

In fact the stock ISC dhcp code has a bug in that it creates
it's AF_PACKET socket with SOCK_PACKET as the type, which causes
the BPF filter it registers to never be used.  It should be
using SOCK_RAW as the type.  It seems to use SOCK_PACKET in
order to use the name based socket address in it's bind()
call.

As a side effect of an unrelated change, this bug got fixed in
Fedora/RHEL (it wants access to the tpacket aux data in order to see
the checksum flags, this is not available with SOCK_PACKET type
AF_PACKET sockets).

But debian definitely still has this bug.  On debian, as a result,
every packet received gets parsed.

Actually this bug is more serious, since the code in ISC dhcp
elides the UDP protocol and port validation if it is compiled
with the BPF code in place (which it is).  This means it's
parsing garbage most of the time.

It also creates the packet socket with ETH_P_ALL, making this
an even more severe issue.

Re: unintended ipv4 broadcast policy change

[Herbert Xu]

On Wed, Jun 22, 2011 at 04:39:35PM -0700, David Miller wrote:
> 
> The context is that I'm looking into cleaning up up the mess we have
> wrt. DHCP listening on packet sockets and (in some cases) seeing every
> packet that hits the system.
> 
> Anyways, back in 2007 this commit was made:
> 
> commit 8030f54499925d073a88c09f30d5d844fb1b3190
> Author: Herbert Xu <herbert@gondor.apana.org.au>
> Date:   Thu Feb 22 01:53:47 2007 +0900
> 
>     [IPV4] devinet: Register inetdev earlier.

It appears that the intention was to allow sysctl control prior
to device open.

> So now every net device registered has inetdev_init() called on it.
> 
> This has a subtle policy side effect that has some interesting
> implications.  The route input slow path has this check:
> 
> 	/* IP on this device is disabled. */
> 
> 	if (!in_dev)
> 		goto out;
> 
> But now this will never, ever, be true.

If we ever wanted to disable IPv4 we could always add a sysctl
for that, just like IPv6.

> Which means that previously we would not accept even broadcast
> or multicast packets on an interface that hasn't had at least
> one IP address configured.
>
> Now we will.

This indeed is an unintended side-effect.

> I think we have a hard decision to make.  One option is to
> fix the input routing check, by changing it to test if the
> ipv4 address list is empty.
> 
> The second option is to remove the check entirely and keep the
> new behavior.

We could also add a disable_ipv4 sysctl and then replace this
check in the routing code with a disable_ipv4 check at the very
top of the IPv4 receive path, just like IPv6.

> This subtle new behavior is interesting because it means that
> a DHCP client could be implemented entirely with plain UDP
> sockets.

Yes this is indeed possible.  However, for compatibility purposes
I'm not sure whether we can safely rely on this new behaviour.
Maybe if we add the disable_ipv4 sysctl we can use it to signal
the presence of this new behaviour.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: unintended ipv4 broadcast policy change

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 22 Jun 2011 16:39:35 -0700 (PDT)

> Contrary to previous discussions I see no code in the ISC dhcp
> client that needs to get at the MAC address.  It merely wants
> it there so that it's packet parsing engine can skip over it
> to get at the ipv4/UDP bits.

I take this back, it does want the "from" MAC address.  When
using plain BSD sockets, it zeros out the address since as we
know the MAC info is not available with normal UDP sockets.

But the ISC DHCP code never actually uses this information at all,
it only stores it away in the "struct packet" structure, never to
be used again.

That's probably because the info it needs is present in the "chaddr"
field of the DHCP packet.  And only the server side actually needs
this.

Re: unintended ipv4 broadcast policy change

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Thu, 23 Jun 2011 08:41:34 +0800

> On Wed, Jun 22, 2011 at 04:39:35PM -0700, David Miller wrote:
>> This subtle new behavior is interesting because it means that
>> a DHCP client could be implemented entirely with plain UDP
>> sockets.
> 
> Yes this is indeed possible.  However, for compatibility purposes
> I'm not sure whether we can safely rely on this new behaviour.
> Maybe if we add the disable_ipv4 sysctl we can use it to signal
> the presence of this new behaviour.

The easiest thing to do is to have the DHCP server first go:

	ip addr add 0.0.0.0/0 broadcast 255.255.255.255 dev $(DEVICE)

and in fact this is essentially what ISC DHCP does on NetBSD and
similar.

Re: unintended ipv4 broadcast policy change

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 22 Jun 2011 19:41:58 -0700 (PDT)

> From: Herbert Xu <herbert@gondor.hengli.com.au>
> Date: Thu, 23 Jun 2011 08:41:34 +0800
> 
>> On Wed, Jun 22, 2011 at 04:39:35PM -0700, David Miller wrote:
>>> This subtle new behavior is interesting because it means that
>>> a DHCP client could be implemented entirely with plain UDP
>>> sockets.
>> 
>> Yes this is indeed possible.  However, for compatibility purposes
>> I'm not sure whether we can safely rely on this new behaviour.
>> Maybe if we add the disable_ipv4 sysctl we can use it to signal
>> the presence of this new behaviour.
> 
> The easiest thing to do is to have the DHCP server first go:
> 
> 	ip addr add 0.0.0.0/0 broadcast 255.255.255.255 dev $(DEVICE)
> 
> and in fact this is essentially what ISC DHCP does on NetBSD and
> similar.

Except that my experiments show that we don't allow this.

In fact, it fails silently because of the code in __inet_insert_ifa()
that goes:

	if (!ifa->ifa_local) {
		inet_free_ifa(ifa);
		return 0;
	}

Sigh...

The only problematic case is receiving the initial broadcast
responses.

Sends are easy because if you use SO_BINDTODEVICE and MSG_DONTROUTE we
can convince the kernel to build us a route, even if no addresses and
routes are configured on the interface.

Will think about this some more.

Re: unintended ipv4 broadcast policy change

[Stephen Hemminger]

On Wed, 22 Jun 2011 16:39:35 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> But debian definitely still has this bug.  On debian, as a result,
> every packet received gets parsed.

Are you saying the DHCP client ends up parsing every packet?
This doesn't appear to be true.

I checked and the dhclient spends its life waiting on select for DHCP port.

Re: unintended ipv4 broadcast policy change

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 23 Jun 2011 08:16:14 -0700

> On Wed, 22 Jun 2011 16:39:35 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
>> But debian definitely still has this bug.  On debian, as a result,
>> every packet received gets parsed.
> 
> Are you saying the DHCP client ends up parsing every packet?
> This doesn't appear to be true.
> 
> I checked and the dhclient spends its life waiting on select for DHCP port.

Which dhcp client do you have installed?  There are about 6 or 7 of
them available in debian.

Unless it closes the AF_PACKET socket after it gets a lease, it's
going to get every packet.  Because it uses a type argument of
"SOCK_PACKET" to the socket() call, the AF_PACKET layer will not use
the packet filter it installs during receive processing.

Check the source if you don't believe me, maybe whatever repo you're
using has different code in this area.

Re: unintended ipv4 broadcast policy change

[Stephen Hemminger]

On Thu, 23 Jun 2011 13:45:04 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 23 Jun 2011 08:16:14 -0700
> 
> > On Wed, 22 Jun 2011 16:39:35 -0700 (PDT)
> > David Miller <davem@davemloft.net> wrote:
> > 
> >> But debian definitely still has this bug.  On debian, as a result,
> >> every packet received gets parsed.
> > 
> > Are you saying the DHCP client ends up parsing every packet?
> > This doesn't appear to be true.
> > 
> > I checked and the dhclient spends its life waiting on select for DHCP port.
> 
> Which dhcp client do you have installed?  There are about 6 or 7 of
> them available in debian.
> 
> Unless it closes the AF_PACKET socket after it gets a lease, it's
> going to get every packet.  Because it uses a type argument of
> "SOCK_PACKET" to the socket() call, the AF_PACKET layer will not use
> the packet filter it installs during receive processing.
> 
> Check the source if you don't believe me, maybe whatever repo you're
> using has different code in this area.

Standard Debian stable (Squeeze) installation.
$ dpkg -S /sbin/dhclient
isc-dhcp-client: /sbin/dhclient

$ dpkg -l isc-dhcp-client
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name           Version        Description
+++-==============-==============-============================================
ii  isc-dhcp-clien 4.1.1-P1-15+sq ISC DHCP client

If it did get every packet, I would see client wake up with strace and
doing work, that is not what it shows...

Maybe there is something else blocking it.

# lsof -p $(pgrep dhclient)
COMMAND   PID USER   FD   TYPE             DEVICE SIZE/OFF    NODE NAME
dhclient 1785 root  cwd    DIR                8,1     4096       2 /
dhclient 1785 root  rtd    DIR                8,1     4096       2 /
dhclient 1785 root  txt    REG                8,1   487696 3407973 /sbin/dhclient
dhclient 1785 root  mem    REG                8,1    47616  917512 /lib/libnss_files-2.11.2.so
dhclient 1785 root  mem    REG                8,1  1432968  917515 /lib/libc-2.11.2.so
dhclient 1785 root  mem    REG                8,1   128744  917527 /lib/ld-2.11.2.so
dhclient 1785 root    0u   CHR                1,3      0t0    2150 /dev/null
dhclient 1785 root    1u   CHR                1,3      0t0    2150 /dev/null
dhclient 1785 root    2u   CHR                1,3      0t0    2150 /dev/null
dhclient 1785 root    3u  unix 0xffff880129b4cf00      0t0    5308 socket
dhclient 1785 root    4r   REG                8,1     1569  536722 /var/lib/dhcp/dhclient-de7b6036-9282-4cec-83ea-ef32117a0c0d-eth0.lease
dhclient 1785 root    5w  pack               6212      0t0     ALL type=SOCK_PACKET
dhclient 1785 root    6u  IPv4               6214      0t0     UDP *:bootpc

Re: unintended ipv4 broadcast policy change

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 23 Jun 2011 17:01:37 -0400

> Standard Debian stable (Squeeze) installation.
> $ dpkg -S /sbin/dhclient
> isc-dhcp-client: /sbin/dhclient

I bet what happens is that since it isn't reading from
the AF_PACKET socket the receive queue just fills up
and it's just in the kernel dropping packets in
packet_spkt_rcv().

Re: unintended ipv4 broadcast policy change

[Stephen Hemminger]

On Thu, 23 Jun 2011 14:08:21 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 23 Jun 2011 17:01:37 -0400
> 
> > Standard Debian stable (Squeeze) installation.
> > $ dpkg -S /sbin/dhclient
> > isc-dhcp-client: /sbin/dhclient
> 
> I bet what happens is that since it isn't reading from
> the AF_PACKET socket the receive queue just fills up
> and it's just in the kernel dropping packets in
> packet_spkt_rcv().

I don't think that is correct, just instrumented /proc/net/packet
to add receive queue length is stuck at 0. The packets are getting
dropped somewhere else.



sk               RefCnt Type Proto  Iface R Rmem   User   Inode  Count
ffff880327038000 3      10   0003   3     1 0      0      5881   0

Re: unintended ipv4 broadcast policy change

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Fri, 24 Jun 2011 10:01:21 -0700

> On Thu, 23 Jun 2011 14:08:21 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Thu, 23 Jun 2011 17:01:37 -0400
>> 
>> > Standard Debian stable (Squeeze) installation.
>> > $ dpkg -S /sbin/dhclient
>> > isc-dhcp-client: /sbin/dhclient
>> 
>> I bet what happens is that since it isn't reading from
>> the AF_PACKET socket the receive queue just fills up
>> and it's just in the kernel dropping packets in
>> packet_spkt_rcv().
> 
> I don't think that is correct, just instrumented /proc/net/packet
> to add receive queue length is stuck at 0. The packets are getting
> dropped somewhere else.
> 
> sk               RefCnt Type Proto  Iface R Rmem   User   Inode  Count
> ffff880327038000 3      10   0003   3     1 0      0      5881   0

They must be because this is capturing ETH_P_ALL with type SOCK_PACKET
which means receive all packets unconditionally.

Re: unintended ipv4 broadcast policy change

[Herbert Xu]

On Fri, Jun 24, 2011 at 12:54:06PM -0700, David Miller wrote:
> They must be because this is capturing ETH_P_ALL with type SOCK_PACKET
> which means receive all packets unconditionally.

sock_queue_rcv_skb calls sk_filter, strange but true :)
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: unintended ipv4 broadcast policy change

[David Miller]

From: Herbert Xu <herbert@gondor.hengli.com.au>
Date: Sat, 25 Jun 2011 08:27:24 +0800

> On Fri, Jun 24, 2011 at 12:54:06PM -0700, David Miller wrote:
>> They must be because this is capturing ETH_P_ALL with type SOCK_PACKET
>> which means receive all packets unconditionally.
> 
> sock_queue_rcv_skb calls sk_filter, strange but true :)

Aha, that explains everything :)