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

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(a)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.

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