Re: [libvirt] [PATCH] network: fix problems with SRV

On 03/18/2014 01:09 AM, Martin Kletzander wrote: > On Tue, Mar 18, 2014 at 12:07:17AM -0600, Laine Stump wrote: >> A patch submitted by Steven Malin last week pointed out a problem with >> libvirt's DNS SRV record configuration: >> >> https://www.redhat.com/archives/libvir-list/2014-March/msg00536.html >> >> When searching for that message later, I found another series that had >> been posted by Guannan Ren back in 2012 that somehow slipped between >> the cracks: >> >> https://www.redhat.com/archives/libvir-list/2012-July/msg00236.html >> >> That patch was very much out of date, but also pointed out some real >> problems. >> >> This patch fixes all the noted problems by refactoring >> virNetworkDNSSrvDefParseXML() and networkDnsmasqConfContents(), then >> verifies those fixes by added several new records to the test case. >> >> Problems fixed: >> >> * both service and protocol now have an underscore ("_") prepended on >> the commandline, as required by RFC2782. >> >> <srv service='sip' protocol='udp' domain='example.com' >> target='tests.example.com' port='5060' priority='10' >> weight='150'/> >> >> before: srv-host=sip.udp.example.com,tests.example.com,5060,10,150 >> after: srv-host=_sip._udp.example.com,tests.example.com,5060,10,150 >> >> * if "domain" wasn't specified in the <srv> element, the extra >> trailing "." will no longer be added to the dnsmasq commandline. >> >> <srv service='sip' protocol='udp' target='tests.example.com' >> port='5060' priority='10' weight='150'/> >> >> before: srv-host=sip.udp.,tests.example.com,5060,10,150 >> after: srv-host=_sip._udp,tests.example.com,5060,10,150 >> >> * when optional attributes aren't specified, the separating comma is >> also now not placed on the dnsmasq commandline. If optional >> attributes in the middle of the line are not specified, they are >> replaced with 0 in the commandline. >> >> <srv service='sip' protocol='udp' target='tests.example.com' >> port='5060'/> >> >> before: srv-host=sip.udp.,tests.example.com,5060,, >> after: srv-host=_sip._udp,tests.example.com,5060 >> >> (actually this would have generated an error, because "optional" >> attributes weren't really optional). >> >> * As a safeguard, the parser checks for a leading "_" on service and >> protocol, and fails if it is found. (Note that we can be guaranteed >> that no existing valid configuration will have this, since until >> now the parser had only allowed "tcp" or "udp" for protocol, and >> the bridge driver wasn't prepending "_", making it a 100% certainty >> that there was no example of working SRV record use in the wild). >> > That's valid for protocol, but not for service. For service there was > a check for length only and it was not escaped at all. Even though > there couldn't be any abuse for that, settings that resulted in > generating invalid config file for dnsmasq were parsed and saved > without any error from libvirt. Just to make sure what I wrote made sense: my point was that, due to the fact that the current code would only be able to generate records like this: [some-name].tcp.[some-domain] [some-name].udp.[some-domain] (i.e. the protocol could only be the exact string "tcp" or "udp"), and since the RFC *requires* that the protocol be prefixed with an underscore, it is impossible that anyone has an actual working config using the existing code (although it may have been saved with no error, it wouldn't do anything useful). Because it is impossible that it could have worked before, we don't need to worry about backward compatibility problems - nothing we do could break a working config. But I *think* what you're pointing out is that, although the config may not be working, it's possible that someone could have made a config like this: <srv service='_sip' protocol='udp' domain='example.com' target='sip.example.com' port='5060'/> which they could have saved, even though it would do nothing useful, it would be saved. Then when that system was updated to the new libvirt, that network's config would generate an error while loading, so the network would be ignored. Gah!!

So I *can't* add this new restriction in the parser. I could check for it during networkValidate() in the bridge driver, though. Do you think this is worthwhile? (I think I'll do it as a separate patch in any case).

>> - >> - /* Check whether protocol value is the supported one */ >> - if (def->protocol && STRNEQ(def->protocol, "tcp") && >> - (STRNEQ(def->protocol, "udp"))) { >> + if (def->protocol && def->protocol[0] == '_') { >> virReportError(VIR_ERR_XML_DETAIL, > I'm not sure we want to allow anything not starting with '_'. OTOH, > we allowed anything in the service (even dots and commas and so on > (non-working configuration). I was bothered by this as well, and spent quite awhile searching RFCs to get a definitive answer on what characters are allowed in a service name or a protocol name. I couldn't find this information, though. I could only find lists of currently assigned service/protocol names. I guess since it is the number that goes over the wire, the RFCs are more concerned about the number than about exactly what it is called (dhcp option names suffer the same problem - there are only official option *numbers*, but not official names). I then considered limiting the character set to what can currently be found in /etc/services and /etc/protocol on a Linux system, but after taking a quick look, it seemed like there wasn't a very strict code about it - there are even some names that use "+". Eric did find a bit in "man services" that says that basically any printable ASCII character is okay, but you should be conservative in what you use (didn't stop people from adding things like "sql*net" and "whois++" to /etc/services, though :-/)

So how about this - I will define a character set that contains only alphanumerics, and those special characters currently used in /etc/services and /etc/protocol, and check it against that. (except ".", because dnsmasq uses that as a separator on the commandline, and there is no method of escaping it). (As discussed above, I have to remove the restriction on _ as the leading character.)

>> + if (dns->srvs[i].port || >> + dns->srvs[i].priority || dns->srvs[i].weight) >> + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].port); > According to dnsmasq(1), port defaults to 1, but you default it here > to 0, which doesn't make sense. Good point, and although our config should be affected by the RFC rather than by dnsmasq, the RFC doesn't say anything about default values because in the DNS record all the fields are mandatory. Since a service for port 0 makes no sense (port 0 is reserved for both tcp and udp), I think we can safely make the value "0" mean "unspecified, use default of 1", and then disallow config with an explicit "port='0'" to avoid ambiguity. Then if we need to put something on the commandline to fill the space, we will put "1" instead of "0".

I'll squash in the changes in the attached patch and send a v2.

From c104ab2370a7dd601c0ad2aff08e176e6eae1e3b Mon Sep 17 00:00:00 2001 From: Laine Stump <laine(a)laine.org&gt; Date: Tue, 18 Mar 2014 15:06:22 -0600 Subject: [PATCH] Changes to squash into SRV patch 1) we must allow leading _ in service, just in case someone has it in an already-saved config. 2) otherwise narrow down the acceptable characters in protocol and service to alphnumerics plus those special characters already found in /etc/services and /etc/protocol (with the exception of ".", which can't be allowed due to its use as a separator character). 3) default for port is 1, not 0. Don't allow "port='0'" (so that value can be reserved to mean "default"), and adjust dnsmasq commandline and test cases accordingly. --- src/conf/network_conf.c | 30 +++++++++++++++++----- src/network/bridge_driver.c | 9 +++++-- .../nat-network-dns-srv-record.conf | 4 +-- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3a28ac7..f1e6243 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -924,6 +924,21 @@ error: return -1; } +/* This includes all characters used in the names of current + * /etc/services and /etc/protocols files (on Fedora 20), except ".", + * which we can't allow because it would conflict with the use of "." + * as a field separator in the SRV record, there appears to be no way + * to escape it in, and the protocols/services that use "." in the + * name are obscure and unlikely to be used anyway. + */ +#define PROTOCOL_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" \ + "-+/" + +#define SERVICE_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" \ + "_-+/*" + static int virNetworkDNSSrvDefParseXML(const char *networkName, xmlNodePtr node, @@ -950,10 +965,10 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->service, networkName, DNS_RECORD_LENGTH_SRV); goto error; } - if (def->service[0] == '_') { + if (strspn(def->service, SERVICE_CHARS) < strlen(def->service)) { virReportError(VIR_ERR_XML_DETAIL, - _("service attribute '%s' in network %s DNS SRV " - "record cannot start with '_'"), + _("invalid character in service attribute '%s' " + "in network %s DNS SRV record"), def->service, networkName); goto error; } @@ -966,10 +981,11 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->service, networkName); goto error; } - if (def->protocol && def->protocol[0] == '_') { + if (def->protocol && + strspn(def->protocol, PROTOCOL_CHARS) < strlen(def->protocol)) { virReportError(VIR_ERR_XML_DETAIL, - _("protocol attribute '%s' in network %s DNS SRV record " - "cannot start with '_'"), + _("invalid character in protocol attribute '%s' " + "in network %s DNS SRV record"), def->protocol, networkName); goto error; } @@ -986,7 +1002,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, def->service, networkName); goto error; } - if (ret == -2 || (ret >= 0 && def->port > 65535)) { + if (ret == -2 || (ret >= 0 && (def->port < 1 || def->port > 65535))) { virReportError(VIR_ERR_XML_DETAIL, _("Invalid DNS SRV port attribute " "for service %s in network %s"), diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 94400cf..3ad6874 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -909,11 +909,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virBufferAsprintf(&configbuf, ",%s", dns->srvs[i].target); /* port, priority, and weight are optional, but are - * identified by their position in the line + * identified by their position in the line. If an item is + * unspecified, but something later in the line *is* + * specified, we need to give the default value for the + * unspecified item. (According to the dnsmasq manpage, + * the default for port is 1). */ if (dns->srvs[i].port || dns->srvs[i].priority || dns->srvs[i].weight) - virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].port); + virBufferAsprintf(&configbuf, ",%d", + dns->srvs[i].port ? dns->srvs[i].port : 1); if (dns->srvs[i].priority || dns->srvs[i].weight) virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].priority); if (dns->srvs[i].weight) diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf index b6fe6f9..16e7dca 100644 --- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf @@ -12,9 +12,9 @@ srv-host=_name._tcp.test-domain-name.com,test.example.com,1111,11,111 srv-host=_name2._udp,test2.example.com,2222,22,222 srv-host=_name3._tcp.test3.com,test3.example.com,3333,33 srv-host=_name4._tcp.test4.com,test4.example.com,4444 -srv-host=_name5._udp,test5.example.com,0,55,555 +srv-host=_name5._udp,test5.example.com,1,55,555 srv-host=_name6._tcp.test6.com,test6.example.com,6666,0,666 -srv-host=_name7._tcp.test7.com,test7.example.com,0,0,777 +srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777 dhcp-range=192.168.122.2,192.168.122.254 dhcp-no-override dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases -- 1.8.5.3