On Mon, Oct 02, 2017 at 17:18:00 -0400, John Ferlan wrote: > > > On 09/28/2017 10:17 AM, Peter Krempa wrote: > > 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. > > I do note that qemuDomainSnapshotCreateXML has a check for '/' in the > name and disallows '.' as name[0]. > > It took a little bit to find, virDomainDefPostParseCheckFeatures is the > domain mechanism.... > > > > > 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. > > ...and virStoragePoolDefParseXML already has a check for not allowing > '/' in the name. > > > > > Anyways, you need to change the validation regex and add the check to > > reject slashes. > > > > So, IIUC it's at least adding a '/' after the ^ in storagecommon.rng. I > validated that works using the xmllint command that virt-xml-validate > generates. > > Do you think virDomainDiskDefParseValidate should add a specific check > that if disk->src->pool doesn't have '/'? Even if it did it wouldn't > match anything since there'd be no pool with a '/'. IDC, I can add it, > but just figured I'd get an opinion first. No, if there is code to reject slashes, you only need to include it in the regex.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
