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(a)redhat.com&gt; --- 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'/>

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(a)redhat.com&gt; >> --- >> 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.

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(a)redhat.com&gt; >>>> --- >>>> 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.