[libvirt] [PATCH] schema: Relax schema for domain name
The domain schema enforced restrictions on the domain name string that the code doesn't. This patch relaxes the check, leaving the restrictions on the driver or hypervisor. --- And maybe we should consider adding some restrictions on the qemu driver, as the daemon is competely fine with creating a domain with the name "../../../../../../../test" that has its configuration stored in "/test.xml" then. docs/schemas/domaincommon.rng | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2041dfb..1922cd6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3003,9 +3003,7 @@ </data> </define> <define name="domainName"> - <data type="string"> - <param name="pattern">[A-Za-z0-9_\.\+\-&:/]+</param> - </data> + <data type="string" /> </define> <define name="diskSerial"> <data type="string"> -- 1.7.3.4
On Mon, Jan 23, 2012 at 06:53:17PM +0100, Peter Krempa wrote:
The domain schema enforced restrictions on the domain name string that the code doesn't. This patch relaxes the check, leaving the restrictions on the driver or hypervisor. --- And maybe we should consider adding some restrictions on the qemu driver, as the daemon is competely fine with creating a domain with the name "../../../../../../../test" that has its configuration stored in "/test.xml" then.
docs/schemas/domaincommon.rng | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2041dfb..1922cd6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3003,9 +3003,7 @@ </data> </define> <define name="domainName"> - <data type="string"> - <param name="pattern">[A-Za-z0-9_\.\+\-&:/]+</param> - </data> + <data type="string" /> </define> <define name="diskSerial"> <data type="string">
I don't think we should remove the pattern entirely. If we want a more general pattern though, we could do an 'allow all', and blacklist just '/' and perhaps a few other characters. I think we should also fix the drivers to check this, since once we have stricter access control support in libvirt, the kind of issue you describe with QEMU will be classed as a CVE security exploit. 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 :|
On 01/23/2012 07:09 PM, Daniel P. Berrange wrote:
On Mon, Jan 23, 2012 at 06:53:17PM +0100, Peter Krempa wrote:
The domain schema enforced restrictions on the domain name string that the code doesn't. This patch relaxes the check, leaving the restrictions on the driver or hypervisor. --- And maybe we should consider adding some restrictions on the qemu driver, as the daemon is competely fine with creating a domain with the name "../../../../../../../test" that has its configuration stored in "/test.xml" then.
I don't think we should remove the pattern entirely. If we want a more general pattern though, we could do an 'allow all', and blacklist just '/' and perhaps a few other characters.
Well, slash is one of those symbols, that some hypervisors happily take as a valid domain name without screwing up their config files. I think we should blacklist only the newline and let the hypervisor decide what they accept and what not. Peter
I think we should also fix the drivers to check this, since once we have stricter access control support in libvirt, the kind of issue you describe with QEMU will be classed as a CVE security exploit.
Daniel
On 01/23/2012 10:53 AM, Peter Krempa wrote:
The domain schema enforced restrictions on the domain name string that the code doesn't. This patch relaxes the check, leaving the restrictions on the driver or hypervisor. --- And maybe we should consider adding some restrictions on the qemu driver, as the daemon is competely fine with creating a domain with the name "../../../../../../../test" that has its configuration stored in "/test.xml" then.
Yes, that point has been brought up before in the past. Snapshot names are another culprit. But it should be a separate patch, per hypervisor; I agree that this patch allowing the XML to be permissive makes sense (since the RNG can't enforce hypervisor-specific restrictions).
docs/schemas/domaincommon.rng | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2041dfb..1922cd6 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3003,9 +3003,7 @@ </data> </define> <define name="domainName"> - <data type="string"> - <param name="pattern">[A-Za-z0-9_\.\+\-&:/]+</param> - </data> + <data type="string" />
ACK. Nit - although XML allows space before />, we typically haven't been using it, so I would have written <data type='string'/> without the space. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa