[libvirt] [PATCH] qemu: Forbid slashes in shmem name

[John Ferlan]

On 02/02/2017 08:14 AM, Martin Kletzander wrote:
> With that users could access files outside /dev/shm.  That itself
> isn't a security problem, but might cause some errors we want to
> avoid.  So let's forbid slashes as we do with domain and volume names
> and also mention that in the schema.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1395496
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  docs/schemas/domaincommon.rng |  6 +++++-
>  src/qemu/qemu_process.c       | 23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 

This was really familiar... hmm.. oh yeah...

Can/should virXMLCheckIllegalChars be used?

See commits ae381879f, dc40dd60, and e1b81968

Likewise, makes me wonder if the *.rng for all those would need some
sort of updating to remove chance that a '\n' exists like you've done
here for the '/' character.

Secondary of course is should the failure be in Parse rather than
checking at startup time?

I agree in principal with what's be done, just the "where" it should be
done.

John

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index cc6e0d0c0d65..00cdc93bca59 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3598,7 +3598,11 @@
> 
>    <define name="shmem">
>      <element name="shmem">
> -      <attribute name="name"/>
> +      <attribute name="name">
> +        <data type="string">
> +          <param name="pattern">[^/]*</param>
> +        </data>
> +      </attribute>
>        <interleave>
>          <optional>
>            <element name="model">
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 184440dc1af6..0f63668100a6 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4586,6 +4586,26 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
> 
> 
>  static int
> +qemuProcessStartValidateShmem(virDomainObjPtr vm)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < vm->def->nshmems; i++) {
> +        virDomainShmemDefPtr shmem = vm->def->shmems[i];
> +
> +        if (strchr(shmem->name, '/')) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("shmem name '%s' must not contain '/'"),
> +                           shmem->name);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int
>  qemuProcessStartValidateXML(virQEMUDriverPtr driver,
>                              virDomainObjPtr vm,
>                              virQEMUCapsPtr qemuCaps,
> @@ -4661,6 +4681,9 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
>      if (qemuProcessStartValidateVideo(vm, qemuCaps) < 0)
>          return -1;
> 
> +    if (qemuProcessStartValidateShmem(vm) < 0)
> +        return -1;
> +
>      VIR_DEBUG("Checking for any possible (non-fatal) issues");
> 
>      qemuProcessStartWarnShmem(vm);
>