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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux