[libvirt] [PATCH V1 2/3] nwfilter: accept broadcasted DHCP replies in DHCP snooping code

[Daniel Veillard]

On Thu, Aug 30, 2012 at 02:29:50PM -0400, Stefan Berger wrote:
> Some DHCP servers send their DHCP replies to the broadcast MAC address
> rather than to the MAC address of the VM. The existing DHCP snooping
> code assumes that the reply always goes to the MAC address of the VM
> thus filtering the traffic of some DHCP servers' replies.
> 
> The below patch adapts the code to
> 
> 1) filter DHCP replies by comparing the MAC address in the reply against
>    the MAC address of the VM (held in the snoop request)
> 
> 2) adapts the pcap filter for traffic towards the VM to accept DHCP replies
>    sent to any MAC address; for further filtering we rely on 1)
> 
> 3) creates initial rules that are active while waiting for DHCP replies;
>    these rules now accept DHCP replies to the VM's MAC address or to the
>    MAC broadcast address
> 
> ---
>  src/nwfilter/nwfilter_dhcpsnoop.c         |   36 +++++++++++++++++++++++-------
>  src/nwfilter/nwfilter_ebiptables_driver.c |   35 +++++++++++++++++------------
>  2 files changed, 49 insertions(+), 22 deletions(-)
> 
> Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
> ===================================================================
> --- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -1010,6 +1010,17 @@ virNWFilterSnoopDHCPDecode(virNWFilterSn
>      if (len < 0)
>          return -2;                 /* invalid packet length */
>  
> +    /*
> +     * some DHCP servers send their responses as MAC broadcast replies
> +     * filter messages from the server also by the destination MAC
> +     * inside the DHCP response
> +     */
> +    if (!fromVM) {
> +        if (virMacAddrCmpRaw(&req->macaddr,
> +                             (unsigned char *)&pd->d_chaddr) != 0)
> +            return -2;
> +    }
> +
>      if (virNWFilterSnoopDHCPGetOpt(pd, len, &mtype, &leasetime) < 0)
>          return -2;
>  
> @@ -1069,7 +1080,6 @@ virNWFilterSnoopDHCPOpen(const char *ifn
>      char pcap_errbuf[PCAP_ERRBUF_SIZE];
>      char *ext_filter = NULL;
>      char macaddr[VIR_MAC_STRING_BUFLEN];
> -    const char *ext;
>  
>      virMacAddrFormat(mac, macaddr);
>  
> @@ -1080,14 +1090,24 @@ virNWFilterSnoopDHCPOpen(const char *ifn
>           * extend the filter with the macaddr of the VM; filter the
>           * more unlikely parameters first, then go for the MAC
>           */
> -        ext = "and ether src";
> +        if (virAsprintf(&ext_filter,
> +                        "%s and ether src %s", filter, macaddr) < 0) {
> +            virReportOOMError();
> +            return NULL;
> +        }
>      } else {
> -        ext = "and ether dst";
> -    }
> -
> -    if (virAsprintf(&ext_filter, "%s %s %s", filter, ext, macaddr) < 0) {
> -        virReportOOMError();
> -        return NULL;
> +        /*
> +         * Some DHCP servers respond via MAC broadcast; we rely on later
> +         * filtering of responses by comparing the MAC address inside the
> +         * DHCP response against the one of the VM. Assuming that the
> +         * bridge learns the VM's MAC address quickly this should not
> +         * generate much more traffic than if we filtered by VM and
> +         * braodcast MAC as well
> +         */
> +        if (virAsprintf(&ext_filter, "%s", filter) < 0) {
> +            virReportOOMError();
> +            return NULL;
> +        }
>      }
>  
>      handle = pcap_create(ifname, pcap_errbuf);
> Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> ===================================================================
> --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -3345,6 +3345,7 @@ ebtablesApplyDHCPOnlyRules(const char *i
>  
>      while (true) {
>          char *srcIPParam = NULL;
> +        int ctr;
>  
>          if (idx < num_dhcpsrvrs) {
>              const char *dhcpserver;
> @@ -3357,20 +3358,26 @@ ebtablesApplyDHCPOnlyRules(const char *i
>              }
>          }
>  
> -        virBufferAsprintf(&buf,
> -                          CMD_DEF("$EBT -t nat -A %s"
> -                                  " -d %s"
> -                                  " -p ipv4 --ip-protocol udp"
> -                                  " %s"
> -                                  " --ip-sport 67 --ip-dport 68"
> -                                  " -j ACCEPT") CMD_SEPARATOR
> -                          CMD_EXEC
> -                          "%s",
> -
> -                          chain_out,
> -                          macaddr_str,
> -                          srcIPParam != NULL ? srcIPParam : "",
> -                          CMD_STOPONERR(1));
> +        /*
> +         * create two rules allowing response to MAC address of VM
> +         * or to broadcast MAC address
> +         */
> +        for (ctr = 0; ctr < 2; ctr++) {
> +            virBufferAsprintf(&buf,
> +                              CMD_DEF("$EBT -t nat -A %s"
> +                                      " -d %s"
> +                                      " -p ipv4 --ip-protocol udp"
> +                                      " %s"
> +                                      " --ip-sport 67 --ip-dport 68"
> +                                      " -j ACCEPT") CMD_SEPARATOR
> +                              CMD_EXEC
> +                              "%s",
> +
> +                              chain_out,
> +                              (ctr == 0) ? macaddr_str : "ff:ff:ff:ff:ff:ff",
> +                              srcIPParam != NULL ? srcIPParam : "",
> +                              CMD_STOPONERR(1));
> +        }
>  
>          VIR_FREE(srcIPParam);
>  
> 

  Okay, the only small risk I see is being able to crash early boot of
some vulnerable OSes with broadcast flooding of a nasty guest on that
network. Fairly limited and easy to detect.

Patch looks okay, ACK,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/