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@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'/>
         </element>
       </optional>
       <optional>
@@ -223,7 +223,7 @@
   <define name='commonmetadata'>
     <interleave>
       <element name='name'>
-        <ref name='genericName'/>
+        <ref name='poolName'/>
       </element>
       <optional>
         <element name='uuid'>
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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.

No, if there is code to reject slashes, you only need to include it in
the regex.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list