4:18 p.m.
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>
>>> ---
>>> 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.
Tks -
John