5:32 a.m.
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 '..'
---
If someone is using domains with '/' in their name, this will cause a
regression for them as libvirt will no longer be able to re-write the
domain XML and will just fail. However, if someone is (ab)using this,
the domain must end up in an already existing directory as libvirt will
not create it so this is hopefully unconvenient enough that noone is
doing that.
If we only want to error out on new domain creation, this may be doable
by introducing a virDomainSaveXMLFlags, but will be more invasive.
Christophe
src/conf/domain_conf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 85a798d..86641d6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14728,6 +14728,14 @@ 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
9:45 a.m.
On 05.02.2013 12:32, 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 '..'
---
If someone is using domains with '/' in their name, this will cause a
regression for them as libvirt will no longer be able to re-write the
domain XML and will just fail. However, if someone is (ab)using this,
the domain must end up in an already existing directory as libvirt will
not create it so this is hopefully unconvenient enough that noone is
doing that.
If we only want to error out on new domain creation, this may be doable
by introducing a virDomainSaveXMLFlags, but will be more invasive.
Christophe
The more problems with naming XML files I see the more I think we should
not tie it to domain names. Basically there's no need to save domain
named XYZ to XYZ.xml. Yes, it's helpful when the names match, however if
domain name contains forbidden characters (e.g. '/'), we can use UUID
for instance for the name of XML file. To keep track which domain is
associated witch which file we can store that kind of information in
virDomainObj.
Having said that, I am not hesitant to take this patch in until somebody
implements the feature as written above.
ACK
Michal
9:47 a.m.
On Tue, Feb 05, 2013 at 04:45:18PM +0100, Michal Privoznik wrote:
On 05.02.2013 12:32, 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 '..'
> ---
>
> If someone is using domains with '/' in their name, this will cause a
> regression for them as libvirt will no longer be able to re-write the
> domain XML and will just fail. However, if someone is (ab)using this,
> the domain must end up in an already existing directory as libvirt will
> not create it so this is hopefully unconvenient enough that noone is
> doing that.
> If we only want to error out on new domain creation, this may be doable
> by introducing a virDomainSaveXMLFlags, but will be more invasive.
>
> Christophe
>
The more problems with naming XML files I see the more I think we should
not tie it to domain names. Basically there's no need to save domain
named XYZ to XYZ.xml. Yes, it's helpful when the names match, however if
domain name contains forbidden characters (e.g. '/'), we can use UUID
for instance for the name of XML file. To keep track which domain is
associated witch which file we can store that kind of information in
virDomainObj.
I think you understate the benefits here. UUID based filenames are just
absolutely *ing horrific for an admin to see and work work and I am very
glad libvirt does not use such a naming scheme.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:54 a.m.
On 05.02.2013 16:47, Daniel P. Berrange wrote:
On Tue, Feb 05, 2013 at 04:45:18PM +0100, Michal Privoznik wrote:
> On 05.02.2013 12:32, 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 '..'
>> ---
>>
>> If someone is using domains with '/' in their name, this will cause a
>> regression for them as libvirt will no longer be able to re-write the
>> domain XML and will just fail. However, if someone is (ab)using this,
>> the domain must end up in an already existing directory as libvirt will
>> not create it so this is hopefully unconvenient enough that noone is
>> doing that.
>> If we only want to error out on new domain creation, this may be doable
>> by introducing a virDomainSaveXMLFlags, but will be more invasive.
>>
>> Christophe
>>
>
> The more problems with naming XML files I see the more I think we should
> not tie it to domain names. Basically there's no need to save domain
> named XYZ to XYZ.xml. Yes, it's helpful when the names match, however if
> domain name contains forbidden characters (e.g. '/'), we can use UUID
> for instance for the name of XML file. To keep track which domain is
> associated witch which file we can store that kind of information in
> virDomainObj.
I think you understate the benefits here. UUID based filenames are just
absolutely *ing horrific for an admin to see and work work and I am very
glad libvirt does not use such a naming scheme.
Daniel
But we discourage users to touch /etc/libvirt/qemu/*.xml files anyway.
Moreover, we advise to define domain by not copying the files but using
libvirt APIs.
But then again - I am not telling we should switch to my approach for
good for all domains. We should use it only for those names, where admin
is stupid enough to name a domain using shell escape sequences or with
'/' contained, etc. These admins don't want to take look under
/etc/libvirt/ anyway.
Michal
10:30 a.m.
On 02/05/2013 08:54 AM, Michal Privoznik wrote:
> I think you understate the benefits here. UUID based filenames
are just
> absolutely *ing horrific for an admin to see and work work and I am very
> glad libvirt does not use such a naming scheme.
>
> Daniel
>
But we discourage users to touch /etc/libvirt/qemu/*.xml files anyway.
Moreover, we advise to define domain by not copying the files but using
libvirt APIs.
It's not just /etc/libvirt/qemu/*.xml. It's also
/var/log/libvirt/qemu/*.log. And while we don't mind telling people to
leave /etc alone, we actively encourage people to use /var/log, and
anything we can do to make the logs more usable (names instead of uuids)
is worth it.
I'm in full agreement with this patch, but think it doesn't go far
enough; we probably also want to forbid domain names with a leading '.'.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4311
days inactive
4311
days old
4 comments
4 participants
participants (4)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake -
Michal Privoznik