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

[Samuel Thibault]

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


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 <address@hidden>
---

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;

patch
Description: Text document