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

Martin Kletzander posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/2ee47f7dc23c1d4b1ba2bea41211fff518bbfa85.1486041254.git.mkletzan@redhat.com
docs/schemas/domaincommon.rng |  6 +++++-
src/qemu/qemu_process.c       | 23 +++++++++++++++++++++++
2 files changed, 28 insertions(+), 1 deletion(-)
[libvirt] [PATCH] qemu: Forbid slashes in shmem name
Posted by Martin Kletzander 7 years, 1 month ago
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@redhat.com>
---
 docs/schemas/domaincommon.rng |  6 +++++-
 src/qemu/qemu_process.c       | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid slashes in shmem name
Posted by John Ferlan 7 years, 1 month ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid slashes in shmem name
Posted by Daniel P. Berrange 7 years, 1 month ago
On Fri, Feb 10, 2017 at 09:07:36AM -0500, John Ferlan wrote:
> 
> 
> 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@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?

The fact that we need to forbid '/' due to it being interpreted as
a path, is an artifact of the QEMU implementation. Other drivers
might not map the names into file paths. So checking in QEMU
driver code is correct.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Forbid slashes in shmem name
Posted by Martin Kletzander 7 years ago
On Fri, Feb 10, 2017 at 02:10:17PM +0000, Daniel P. Berrange wrote:
>On Fri, Feb 10, 2017 at 09:07:36AM -0500, John Ferlan wrote:
>>
>>
>> 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@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?
>
>The fact that we need to forbid '/' due to it being interpreted as
>a path, is an artifact of the QEMU implementation. Other drivers
>might not map the names into file paths. So checking in QEMU
>driver code is correct.
>

Ping, does this mean ACK?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list