[DoS] slirp (arp): do not special-case bogus IP addresses

Do not special-case addresses with zero host part, as we do not
necessarily know how big it is, and the guest can fake them anyway.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---

This is particularly bad actually, one can for instance simply do this
inside a Linux guest

ip addr add 192.0.0.0/1 dev eth0

and crash qemu (thus a DoS) by just emitting a packet (thus from
192.0.0.0), getting:

qemu-system-x86_64: /usr/src/qemu/slirp/arp_table.c:77: arp_table_search: Assertion `(ip_addr & __bswap_32 (~(0xfU << 28))) != 0' failed.

so it should probably go to all stable maintained versions.

 arp_table.c |    8 --------
 1 file changed, 8 deletions(-)

On Thu, May 08, 2014 at 12:15:09AM +0200, Samuel Thibault wrote:
> Do not special-case addresses with zero host part, as we do not
> necessarily know how big it is, and the guest can fake them anyway.


Hi Samuel,

The search part looks OK to me but when adding to the arp table, don't
you at least want to avoid adding mappings for 0.0.0.0/32? to avoid
for ex garps to pollute the cache with invalid entries?

Cheers,
Edgar



> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> 
> This is particularly bad actually, one can for instance simply do this
> inside a Linux guest
> 
> ip addr add 192.0.0.0/1 dev eth0
> 
> and crash qemu (thus a DoS) by just emitting a packet (thus from
> 192.0.0.0), getting:
> 
> qemu-system-x86_64: /usr/src/qemu/slirp/arp_table.c:77: arp_table_search: Assertion `(ip_addr & __bswap_32 (~(0xfU << 28))) != 0' failed.
> 
> so it should probably go to all stable maintained versions.
> 
>  arp_table.c |    8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
> index ecdb0ba..243cbbc 100644
> --- a/slirp/arp_table.c
> +++ b/slirp/arp_table.c
> @@ -37,11 +37,6 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN])
>                  ethaddr[0], ethaddr[1], ethaddr[2],
>                  ethaddr[3], ethaddr[4], ethaddr[5]));
>  
> -    /* Check 0.0.0.0/8 invalid source-only addresses */
> -    if ((ip_addr & htonl(~(0xfU << 28))) == 0) {
> -        return;
> -    }
> -
>      if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
>          /* Do not register broadcast addresses */
>          return;
> @@ -73,9 +68,6 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>      DEBUG_CALL("arp_table_search");
>      DEBUG_ARG("ip = 0x%x", ip_addr);
>  
> -    /* Check 0.0.0.0/8 invalid source-only addresses */
> -    assert((ip_addr & htonl(~(0xfU << 28))) != 0);
> -
>      /* If broadcast address */
>      if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
>          /* return Ethernet broadcast address */
>

Edgar E. Iglesias, le Thu 08 May 2014 06:10:18 +0000, a écrit :
> The search part looks OK to me but when adding to the arp table, don't
> you at least want to avoid adding mappings for 0.0.0.0/32?

I don't see the gain, actually.  It would mean burning some CPU all the
time just to save a small potential memory loss and CPU burning in the
rare case when the guest behaves oddly.

> to avoid for ex garps to pollute the cache with invalid entries?

Only one entry will be created and updated by garps. The guest already
has a lot of ways to pollute the cache :)

samuel

On Thu, May 08, 2014 at 08:50:33AM +0200, Samuel Thibault wrote:
> Edgar E. Iglesias, le Thu 08 May 2014 06:10:18 +0000, a écrit :
> > The search part looks OK to me but when adding to the arp table, don't
> > you at least want to avoid adding mappings for 0.0.0.0/32?
> 
> I don't see the gain, actually.  It would mean burning some CPU all the
> time just to save a small potential memory loss and CPU burning in the
> rare case when the guest behaves oddly.
> 
> > to avoid for ex garps to pollute the cache with invalid entries?
> 
> Only one entry will be created and updated by garps. The guest already
> has a lot of ways to pollute the cache :)

Hi,

I was under the impression that entries for 0.0.0.0 are strictly
invalid (not about performance). I might be wrong though.

Cheers,
Edgar