On Thu, Sep 28, 2017 at 10:03:55 -0400, John Ferlan wrote: > > > On 09/28/2017 09:47 AM, Peter Krempa wrote: > > On Wed, Sep 27, 2017 at 15:07:35 -0400, John Ferlan wrote: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1475250 > >> > >> It's possible to define and start a pool with a '.' in the > >> name; however, when trying to add a volume to a domain using > >> the storage pool source with a name with a '.' in the name, > >> the domain RNG validation fails because RNG uses 'genericName' > >> which does not allow a '.' in the name. Pool definition has > >> no similar call to virXMLValidateAgainstSchema. Pool name > >> validation occurs in storagePoolDefineXML and only calls > >> virXMLCheckIllegalChars using the same parameter "\n" as > >> qemuDomainDefineXMLFlags would check after the RNG check > >> could be succesful. > >> > >> So in order to resolve this, create a poolName definition > >> in the RNG and allow the pool name and the volume source > >> pool name to use that definition. > >> > >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> > >> --- > >> docs/schemas/domaincommon.rng | 2 +- > >> docs/schemas/storagecommon.rng | 8 ++++++++ > >> docs/schemas/storagepool.rng | 4 ++-- > >> 3 files changed, 11 insertions(+), 3 deletions(-) > >> > >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > >> index 76852abb3..2cc8dcecf 100644 > >> --- a/docs/schemas/domaincommon.rng > >> +++ b/docs/schemas/domaincommon.rng > >> @@ -1669,7 +1669,7 @@ > >> <optional> > >> <element name="source"> > >> <attribute name="pool"> > >> - <ref name="genericName"/> > >> + <ref name="poolName"/> > >> </attribute> > >> <attribute name="volume"> > >> <ref name="volName"/> > >> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng > >> index 717f3c603..49578312e 100644 > >> --- a/docs/schemas/storagecommon.rng > >> +++ b/docs/schemas/storagecommon.rng > >> @@ -6,6 +6,14 @@ > >> <!-- This schema is not designed for standalone use; another file > >> must include both this file and basictypes.rng --> > >> > >> + <define name="poolName"> > >> + <data type="string"> > >> + <!-- Use literal newline instead of \n for bug in libxml2 2.7.6 --> > >> + <param name="pattern">[^ > >> +]+</param> > >> + </data> > >> + </define> > >> + > >> <define name='encryption'> > >> <element name='encryption'> > >> <attribute name='format'> > >> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng > >> index f0117bd69..52b2044be 100644 > >> --- a/docs/schemas/storagepool.rng > >> +++ b/docs/schemas/storagepool.rng > >> @@ -209,7 +209,7 @@ > >> <interleave> > >> <optional> > >> <element name='name'> > >> - <ref name='genericName'/> > >> + <ref name='poolName'/> > > > > This means that a name starting with a dot is invalid according to the > > schema, but the user ignored the schema and the code is not doing enough > > validation. > > > > I'm not convinced that this is the correct solution. VMs disallow dots > > since the name is used for generating filenames and using '../' as > > prefix will allow directory traversal exploits. > > > > NACK, I think we should disallow pool names with a dot even in the code. > > It will be slightly harder since there are no 'validate' callbacks for > > them and you can't disallow them in the parser. > > > > As a test I defined a domain and a pool using ".test" and it worked. > > # virsh list > Id Name State > ---------------------------------------------------- > 1 .f23 running > > # virsh dumpxml .f23 > <domain type='kvm' id='1'> > <name>.f23</name> > <uuid>e28761fe-ea53-4682-924a-744a4028f146</uuid> > <memory unit='KiB'>2097152</memory> > ... > > > The problem with disallowing in code after the fact is how do you handle > things now that we have allowed it? I have another long standing bug > where someone doesn't want a space in the name or even worse a space as > the name. Fair enough, in fact qemu driver (and others which rely on files) forbid usage of '/' in the name. In such case we can allow a '.' or whatever else, so that it's not considered as a directory. In case of the storage driver I think we can ban '/' accross all drivers and also in the parser, since storage pool containing an '/' probably would vanish after VM restart and/or fail to be created for other reasons than validation. Anyways, you need to change the validation regex and add the check to reject slashes.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
