[libvirt] [PATCHv2] Reject domain names containing '/' or starting with '.'
In addition to rejecting domains containing '/', this series also filter out domains starting with a '.'. Christophe
Similarly to 790f912b4 which rejects snapshots names containing, this commit changes virDomainSaveXML to reject domains with a '/' in their name. The domain name is used as a filename, so this leads to unexpected results when used in combination with '..' --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85a798d..13f4bc0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14728,6 +14728,13 @@ int virDomainSaveXML(const char *configDir, char *configFile = NULL; int ret = -1; + if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid domain name '%s': name can't contain '/'"), + def->name); + goto cleanup; + } + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup; -- 1.8.1
On 02/07/2013 08:27 AM, Christophe Fergeau wrote:
Similarly to 790f912b4 which rejects snapshots names containing, this commit changes virDomainSaveXML to reject domains with a '/' in their name. The domain name is used as a filename, so this leads to unexpected results when used in combination with '..' --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85a798d..13f4bc0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14728,6 +14728,13 @@ int virDomainSaveXML(const char *configDir, char *configFile = NULL; int ret = -1;
+ if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid domain name '%s': name can't contain '/'"), + def->name); + goto cleanup; + } + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup;
Seems this should be in a more "general" location. Would the same rules apply to other objects (networks, storage, etc.)? What other characters should be avoided? Having a comma, semi-colon, colon, etc. could have interesting results. Perhaps somewhat different rules for each, but using some sort of type of object enum to weed out object specific rules (eg, VIR_IS_ type macros). Perhaps someone who's been on the project a bit longer has a suggestion or two... John
On 02/07/2013 07:57 AM, John Ferlan wrote:
On 02/07/2013 08:27 AM, Christophe Fergeau wrote:
Similarly to 790f912b4 which rejects snapshots names containing, this commit changes virDomainSaveXML to reject domains with a '/' in their name. The domain name is used as a filename, so this leads to unexpected results when used in combination with '..' --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85a798d..13f4bc0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14728,6 +14728,13 @@ int virDomainSaveXML(const char *configDir, char *configFile = NULL; int ret = -1;
+ if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid domain name '%s': name can't contain '/'"), + def->name); + goto cleanup; + } + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup;
Seems this should be in a more "general" location. Would the same rules apply to other objects (networks, storage, etc.)? What other characters should be avoided?
In my mind, newline is the most questionable remaining character; as long as a user has proper shell quoting, and we limit things to one file per line, then all other file names are at least usable even if they remain questionable for ability to type easily.
Having a comma, semi-colon, colon, etc. could have interesting results. Perhaps somewhat different rules for each, but using some sort of type of object enum to weed out object specific rules (eg, VIR_IS_ type macros).
Indeed, a series of followup patches to make these types of restrictions more generic might be nice; but I don't think we need to wait for a followup in order to apply this series as-is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
John Ferlan wrote:
On 02/07/2013 08:27 AM, Christophe Fergeau wrote:
Similarly to 790f912b4 which rejects snapshots names containing, this commit changes virDomainSaveXML to reject domains with a '/' in their name. The domain name is used as a filename, so this leads to unexpected results when used in combination with '..' --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85a798d..13f4bc0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14728,6 +14728,13 @@ int virDomainSaveXML(const char *configDir, char *configFile = NULL; int ret = -1;
+ if (strchr(def->name, '/')) { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid domain name '%s': name can't contain '/'"), + def->name); + goto cleanup; + } + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup;
Seems this should be in a more "general" location. Would the same rules apply to other objects (networks, storage, etc.)? What other characters should be avoided? Having a comma, semi-colon, colon, etc. could have interesting results.
Yeah, comma is an interesting one for qemu since that delimits option subarguments. E.g. trying to start a qemu instance with name 'foo,bar' results in $ virsh start "foo,bar" error: Failed to start domain foo,bar error: internal error process exited while connecting to monitor: Unknown subargument bar to -name Regards, Jim
On 02/08/2013 11:44 AM, Jim Fehlig wrote:
Seems this should be in a more "general" location. Would the same rules apply to other objects (networks, storage, etc.)? What other characters should be avoided? Having a comma, semi-colon, colon, etc. could have interesting results.
Yeah, comma is an interesting one for qemu since that delimits option subarguments. E.g. trying to start a qemu instance with name 'foo,bar' results in
$ virsh start "foo,bar" error: Failed to start domain foo,bar error: internal error process exited while connecting to monitor: Unknown subargument bar to -name
That's an independent bug - we already have a function for properly quoting commas when passing to qemu; if we would use that function properly, the command line would be -name foo,,bar, at which point qemu would be using 'foo,bar' internally anywhere the name is needed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 02/08/2013 11:44 AM, Jim Fehlig wrote:
Seems this should be in a more "general" location. Would the same rules apply to other objects (networks, storage, etc.)? What other characters should be avoided? Having a comma, semi-colon, colon, etc. could have interesting results.
Yeah, comma is an interesting one for qemu since that delimits option subarguments. E.g. trying to start a qemu instance with name 'foo,bar' results in
$ virsh start "foo,bar" error: Failed to start domain foo,bar error: internal error process exited while connecting to monitor: Unknown subargument bar to -name
That's an independent bug - we already have a function for properly quoting commas when passing to qemu; if we would use that function properly, the command line would be -name foo,,bar, at which point qemu would be using 'foo,bar' internally anywhere the name is needed.
I hacked up a patch to quote the comma and qemu still rejects it virsh start "foo,bar" error: Failed to start domain foo,bar error: internal error process exited while connecting to monitor: qemu-kvm: -name foo,,bar: invalid option I should have first tried invoking qemu directly instead of fiddling with a patch qemu-kvm -name foo,,bar Unknown subargument ,bar to -name This is with 1.4 rc1 btw. Regards, Jim
On 02/07/2013 06:27 AM, Christophe Fergeau wrote:
Similarly to 790f912b4 which rejects snapshots names containing, this commit changes virDomainSaveXML to reject domains with a '/' in their name. The domain name is used as a filename, so this leads to unexpected results when used in combination with '..' --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
I like the end goal, but am not sure the approach was right. That is, adding the checks to domain_conf.c feels too strong. I can envision the set of allowed vs. restricted names being different for different hypervisors - just because the qemu driver creates a .log file based on the domain name doesn't mean that other drivers will do the same (I don't have enough experience with ESX to say for certain, but as far as I know, libvirt doesn't create any additional files based on domain names for esx:// connections and therefore does not need to restrict '/'; or ESX may have its own set of even tighter rules on what forms a valid name). I think the approach in commit 790f912b4 of sticking the restriction in qemu_driver.c is better; the parser should accept everything, and then the driver should decide whether it was valid. Also, I see no reason to split this into two patches; adding the restrictions in the snapshot case was split into two patches only because of a late-breaking review. And I'm not sure that VIR_ERR_XML_DETAIL was the best choice of error message in the earlier patches that you copied from, although fixing it to something nicer would be a separate cleanup. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The domain name is used as a filename, so this leads to unexpected results, either because hidden files are created, or because the file is created somewhere else in the directory tree when '/' is used as well. --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 13f4bc0..f5b738a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14735,6 +14735,13 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } + if (def->name[0] == '.') { + virReportError(VIR_ERR_XML_DETAIL, + _("invalid domain name '%s': name can't start with '.'"), + def->name); + goto cleanup; + } + if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup; -- 1.8.1
participants (4)
-
Christophe Fergeau -
Eric Blake -
Jim Fehlig -
John Ferlan