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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list