[libvirt] [PATCH v2] conf: virDomainDefValidateInternal prohibit some characters in shmem name

[John Ferlan]

On 08/01/2018 06:24 AM, skobyda at redhat.com wrote:
> On Fri, 2018-07-27 at 13:19 -0400, John Ferlan wrote:
>> You shouldn't drop libvir-list when you reply...
> Sorry, I dropped it by mistake.
>>
>> On 07/27/2018 06:35 AM, skobyda at redhat.com wrote:
>>> On Mon, 2018-07-23 at 17:32 -0400, John Ferlan wrote:
>>>>
>>>> On 07/12/2018 09:10 AM, Simon Kobyda wrote:
>>>>> XML shmem name will not include character '/', and will not be
>>>>> equal to strings
>>>>> "." or "..", as shmem name is used in a path.
>>>>
>>>> Validate that the provided XML shmem name is not directory
>>>> specific
>>>> "."
>>>> or ".." names as well as ensuring that there is no path separator
>>>> '/'
>>>> in
>>>> the name.
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1192400
>>>>> ---
>>>>>
>>>>> Changes in V2 
>>>>> 	- Added error reports
>>>>> 	- Error situation will happen only if shmem name is
>>>>> equal to
>>>>> 	  "." or "..", however their occurence in a name
>>>>> compromised of
>>>>> more
>>>>>           characters is allowed.
>>>>>
>>>>>  src/conf/domain_conf.c | 22 ++++++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>
>>>> I believe this actually belongs in
>>>> virDomainDeviceDefValidateInternal
>>>> for case VIR_DOMAIN_DEVICE_SHMEM.
>>>> Also, should the docs/schemas/domaincommon.rng be modified?
>>>> Currently
>>>> it
>>>> has:
>>>>
>>>>   <define name="shmem">
>>>>     <element name="shmem">
>>>>       <attribute name="name">
>>>>         <data type="string">
>>>>           <param name="pattern">[^/]*</param>
>>>>         </data>
>>>>
>>>
>>> Is it a good idea? To create such regular expression, I believe it
>>> would make it unreadable for another person, ergo useless. I don't
>>> want
>>> it to end up as IPv6. I've benn told that actual parser can be, and
>>> sometimes is stricter than scheme.
>>>
>>
>> Well this is what I was hoping others would be able to chime in on.
>> Hence why you ask on list. I don't disagree that being stricter in
>> Parse
>> is fine.  Doing regexes is like reading a foreign language to me at
>> times. Some just don't make sense.
> 
> Yeah, that's why I don't want to try to insert complicated regex in
> docs/schemas/domaincommon.rng.
>>
>> I think that regex is indicating - any character except '/', but I'm
>> not
>> sure. If it is, it's ironic that we have to check for '/'
>> specifically
>> since it shouldn't be passing the xml validation test - OK at least
>> if
>> one was editing via virsh.
>>
>> John
>>
> You are right about what that regex does. But from what I heard, that
> functionality is rather new to libvirt, so users have option to bypass
> that verification. So I would like to also leave it in code in case
> users try to bypass that regex verification.
> 
> Simon.
> 

See commit id '1c06d0fa' for the shmem validation previously added.
Ironically a higher numbered bz than referenced your commit.

And yes it's quite easy to tell virsh edit to ignore the xml validation
error. I'm fine with leaving the RNG as is. I had a different bz in my
pile related to resource names, so the topic was somewhat fresh in mind
related to looking at what various RNG "patterns" had, see:

https://www.redhat.com/archives/libvir-list/2018-July/msg02046.html

in that case the problem is that it's possible to name something " " or
"	" and you don't know whether a space or tab or any combination was
used for the entirety of the name.

I think if you move the code from virDomainDefValidateInternal to a
SHMEM specific case in virDomainDeviceDefValidateInternal, then that'll
be fine. I would think that the qemuProcessStartValidateShmem call from
commit 1c06d0fa would not be possible then.


John

>>>> Consider how other names are limited in their scope. The
>>>> basictypes.rng
>>>> has a number of examples.
>>>>
>>>> Naturally, the problem with changing it is that someone somewhere
>>>> will
>>>> complain, but libvirt used to accept this other format. Right now
>>>> I
>>>> would think the scope a bit too broad.
>>>
>>> But then we cannot fix most of the bugs, since there could always
>>> be
>>> some user which relies on bug behavior.
>>>>
>>>> If we are to limit the name we should also document in
>>>> docs/formatdomain.html.in that the shmem name is "limited" in
>>>> name to
>>>> avoid the '/' character, ".", and "..".
>>>>
>>>> BTW: My regex isn't that good, but it would seem '/' is an
>>>> invalid
>>>> character by XML standards even though the code never checked for
>>>> it.
>>>> Using virt-xml-validate <file> <schema> would "validate" whether
>>>> someone
>>>> provides valid XML.
>>>>
>>>>
>>>> John
>>>>
>>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>>> index 7ab2953d83..6b34c17de4 100644
>>>>> --- a/src/conf/domain_conf.c
>>>>> +++ b/src/conf/domain_conf.c
>>>>> @@ -6107,6 +6107,8 @@ virDomainDefLifecycleActionValidate(const
>>>>> virDomainDef *def)
>>>>>  static int
>>>>>  virDomainDefValidateInternal(const virDomainDef *def)
>>>>>  {
>>>>> +    size_t i;
>>>>> +
>>>>>      if (virDomainDefCheckDuplicateDiskInfo(def) < 0)
>>>>>          return -1;
>>>>>  
>>>>> @@ -6136,6 +6138,26 @@ virDomainDefValidateInternal(const
>>>>> virDomainDef *def)
>>>>>          return -1;
>>>>>      }
>>>>>  
>>>>> +    for (i = 0; i < def->nshmems; i++) {
>>>>> +        if (strchr(def->shmems[i]->name, '/')) {
>>>>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>>>>> +                           _("shmem name cannot include '/'
>>>>> character"));
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        if (STREQ(def->shmems[i]->name, ".")) {
>>>>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>>>>> +                           _("shmem name cannot be equal to
>>>>> '.'"));
>>>>> +            return -1;
>>>>> +        }
>>>>> +
>>>>> +        if (STREQ(def->shmems[i]->name, "..")) {
>>>>> +            virReportError(VIR_ERR_XML_ERROR, "%s",
>>>>> +                           _("shmem name cannot be equal to
>>>>> '..'"));
>>>>> +            return -1;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>>      if (virDomainDefLifecycleActionValidate(def) < 0)
>>>>>          return -1;
>>>>>  
>>>>>