On Thu, Aug 30, 2012 at 02:29:49PM -0400, Stefan Berger wrote:

Add function for testing for Ethernet broadcast address --- src/libvirt_private.syms | 1 + src/util/virmacaddr.c | 9 +++++++++ src/util/virmacaddr.h | 2 ++ 3 files changed, 12 insertions(+)

Index: libvirt-acl/src/util/virmacaddr.c =================================================================== --- libvirt-acl.orig/src/util/virmacaddr.c +++ libvirt-acl/src/util/virmacaddr.c @@ -30,6 +30,9 @@ #include "virmacaddr.h" #include "virrandom.h"

+static const unsigned char virMacAddrBroadcastAddrRaw[VIR_MAC_BUFLEN] = + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; + /* Compare two MAC addresses, ignoring differences in case, * as well as leading zeros. */ @@ -218,3 +221,9 @@ virMacAddrIsUnicast(const virMacAddrPtr { return !(mac->addr[0] & 1); } + +bool +virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]) +{ + return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0; +} Index: libvirt-acl/src/util/virmacaddr.h =================================================================== --- libvirt-acl.orig/src/util/virmacaddr.h +++ libvirt-acl/src/util/virmacaddr.h @@ -52,4 +52,6 @@ int virMacAddrParse(const char* str, virMacAddrPtr addr) ATTRIBUTE_RETURN_CHECK; bool virMacAddrIsUnicast(const virMacAddrPtr addr); bool virMacAddrIsMulticast(const virMacAddrPtr addr); +bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]); +

Humpf, we really ought to add virMacAddrIsBroadcast instead, but I understand from patch 3/3 that you don't have the structure handy at that point (and doing a pointer cast to an inexistant structure would be ugly), so okay for the purpose of fixing that bug, but I would expect a followup patch with a proper bool virMacAddrIsBroadcast(const virMacAddrPtr addr); added too

#endif /* __VIR_MACADDR_H__ */ Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -1329,6 +1329,7 @@ virMacAddrCompare; virMacAddrFormat; virMacAddrGenerate; virMacAddrGetRaw; +virMacAddrIsBroadcastRaw; virMacAddrIsMulticast; virMacAddrIsUnicast; virMacAddrParse;

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

On Fri, Aug 31, 2012 at 06:37:29AM -0400, Stefan Berger wrote:

On 08/30/2012 11:33 PM, Daniel Veillard wrote:

...

On Thu, Aug 30, 2012 at 02:29:49PM -0400, Stefan Berger wrote:

...

Index: libvirt-acl/src/util/virmacaddr.h =================================================================== --- libvirt-acl.orig/src/util/virmacaddr.h +++ libvirt-acl/src/util/virmacaddr.h @@ -52,4 +52,6 @@ int virMacAddrParse(const char* str, virMacAddrPtr addr) ATTRIBUTE_RETURN_CHECK; bool virMacAddrIsUnicast(const virMacAddrPtr addr); bool virMacAddrIsMulticast(const virMacAddrPtr addr); +bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]); + Humpf, we really ought to add virMacAddrIsBroadcast instead, but I understand from patch 3/3 that you don't have the structure handy at that point (and doing a pointer cast to an inexistant structure would be ugly), so okay for the purpose of fixing that bug, but I would expect a followup patch with a proper

bool virMacAddrIsBroadcast(const virMacAddrPtr addr);

added too

Yes, had thought of that but since there's no caller at the moment, I skipped it.

yeah, understood

...

...

#endif /* __VIR_MACADDR_H__ */ Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -1329,6 +1329,7 @@ virMacAddrCompare; virMacAddrFormat; virMacAddrGenerate; virMacAddrGetRaw; +virMacAddrIsBroadcastRaw; virMacAddrIsMulticast; virMacAddrIsUnicast; virMacAddrParse; ACK,

Daniel

Thanks. Will push.

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

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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, Aug 30, 2012 at 02:29:51PM -0400, Stefan Berger wrote:

Adapt the IP learning code to also accept broadcasted DHCP replies

--- src/nwfilter/nwfilter_learnipaddr.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)

Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -414,9 +414,7 @@ learnIPAddressThread(void *arg) req->status = EINVAL; goto done; } - virBufferAsprintf(&buf, " ether dst %s" - " and src port 67 and dst port 68", - macaddr); + virBufferAsprintf(&buf, "src port 67 and dst port 68"); break; default: if (techdriver->applyBasicRules(req->ifname, @@ -424,7 +422,8 @@ learnIPAddressThread(void *arg) req->status = EINVAL; goto done; } - virBufferAsprintf(&buf, "ether host %s", macaddr); + virBufferAsprintf(&buf, "ether host %s or ether dst ff:ff:ff:ff:ff:ff", + macaddr); }

if (virBufferError(&buf)) { @@ -529,7 +528,9 @@ learnIPAddressThread(void *arg) } } } else if (virMacAddrCmpRaw(&req->macaddr, - ether_hdr->ether_dhost) == 0) { + ether_hdr->ether_dhost) == 0 || + /* allow Broadcast replies from DHCP server */ + virMacAddrIsBroadcastRaw(ether_hdr->ether_dhost)) { /* packets to the VM */ if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize +

Okay, c.f. comment on patch 1/3 ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/