7:27 a.m.
In addition to rejecting domains containing '/', this series also filter
out domains starting with a '.'.
Christophe
7:27 a.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
8:57 a.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
12:30 p.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
12:44 p.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
12:50 p.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
8:48 p.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
12:37 p.m.
New subject: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'
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
7:27 a.m.
New subject: [libvirt] [PATCHv2 2/2] virDomainSaveXML: Reject domains which name start with '.'
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
4330
days inactive
4337
days old
8 comments
4 participants
participants (4)
-
Christophe Fergeau -
Eric Blake -
Jim Fehlig -
John Ferlan