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

[Peter Krempa]

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.

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170928/9287fbfc/attachment-0001.sig>