Re: [Qemu-devel] [PATCH, DoS] slirp (arp): do not special-case bogus IP addresses

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Wed, May 14, 2014 at 02:44:20AM +0200, Samuel Thibault wrote:
> Edgar E. Iglesias, le Wed 14 May 2014 00:30:09 +0000, a écrit :
> > > At best I could think of using the patch below, which avoids registering
> > > anything for 0.0.0.0, and use a broadcast to answer a guest which
> > > would have used 0.0.0.0 as a source for whatever reason.  I don't find
> > > anything else reasonable.  What would be preferred?
> > 
> > Specs are not super clear on this but rfc1700 says that 0.0.0.0 is a source only address.
> 
> I agree.
> 
> > What I was trying to suggest was a mix between your two versions.
> > Removing the assert in table_search and avoid adding 0.0.0.0/32 to the cache
> > in table_add. We might need to complement with something that drops datagrams
> > destined to 0.0.0.0 in upper layers so we dont keep trying, not sure.
> > Does something like that make sense?
> 
> So that would be this.
> 
> Samuel


This looks good to me. Minor nit, the comment in if_encap should say
"0.0.0.0 can not be a destination address..."

Cheers,
Edgar

> 
> 
> 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.
> Silently avoiding having 0.0.0.0 as a destination, however.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
> 
> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
> index ecdb0ba..bcaeb44 100644
> --- a/slirp/arp_table.c
> +++ b/slirp/arp_table.c
> @@ -37,12 +37,7 @@ 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) {
> +    if (ip_addr == 0 || 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 */
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 3fb48a4..2f189e0 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -778,6 +778,11 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>          return 1;
>      }
>  
> +    if (!iph->ip_dst.s_addr) {
> +        /* 0.0.0.0 can not be a source address, something went wrong, avoid
> +         * making it worse */
> +        return 1;
> +    }
>      if (!arp_table_search(slirp, iph->ip_dst.s_addr, ethaddr)) {
>          uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
>          struct ethhdr *reh = (struct ethhdr *)arp_req;

> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
> index ecdb0ba..bcaeb44 100644
> --- a/slirp/arp_table.c
> +++ b/slirp/arp_table.c
> @@ -37,12 +37,7 @@ 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) {
> +    if (ip_addr == 0 || 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 */
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 3fb48a4..2f189e0 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -778,6 +778,11 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>          return 1;
>      }
>  
> +    if (!iph->ip_dst.s_addr) {
> +        /* 0.0.0.0 can not a a source address, something went wrong, avoid
> +         * making it it worse */
> +        return 1;
> +    }
>      if (!arp_table_search(slirp, iph->ip_dst.s_addr, ethaddr)) {
>          uint8_t arp_req[ETH_HLEN + sizeof(struct arphdr)];
>          struct ethhdr *reh = (struct ethhdr *)arp_req;