[libvirt] [PATCH] docs, rng: Allow a pool name to be line domain name

[John Ferlan]

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 at 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