[libvirt] [PATCH] qemu: Label uniqDir when probing capabilities

Martin Kletzander posted 1 patch 4 years, 11 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/0e68f22354d6d4d6ce607485da4ee370d10021e0.1555075941.git.mkletzan@redhat.com
src/qemu/qemu_process.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
[libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Martin Kletzander 4 years, 11 months ago
This does not cause a problem in usual scenarios thanks to us allowing
CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
an issue because the directory is created with mkdtemp(3) which explicitly
creates that with 0700 permissions and qemu running as non-root cannot access
that.

The scenarios include:
 - Builds without CAPNG
 - Running libvirtd in a container [1]
 - and possibly others.

[1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/qemu/qemu_process.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 47d8ca2ff163..2e2c4812fef7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
 }
 
 
+static int
+qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {
+    /* We cannot use the security driver here, but we should not need to. */
+    if (chown(proc->uniqDir, proc->runUid, -1) < 0) {
+        virReportSystemError(errno,
+                             "Cannot chown uniq path: %s", proc->uniqDir);
+        return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuProcessQMPInit(qemuProcessQMPPtr proc)
 {
@@ -8466,6 +8479,9 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
         goto cleanup;
     }
 
+    if (qemuProcessQEMULabelUniqPath(proc) < 0)
+        goto cleanup;
+
     if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir,
                     "qmp.monitor") < 0)
         goto cleanup;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Daniel P. Berrangé 4 years, 11 months ago
On Fri, Apr 12, 2019 at 03:32:21PM +0200, Martin Kletzander wrote:
> This does not cause a problem in usual scenarios thanks to us allowing
> CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
> an issue because the directory is created with mkdtemp(3) which explicitly
> creates that with 0700 permissions and qemu running as non-root cannot access
> that.
> 
> The scenarios include:
>  - Builds without CAPNG

This makes sense, since we're unable to give QEMU CAP_DAC_OVERRIDE

>  - Running libvirtd in a container [1]

I'm rather puzzelled why this is a problem unless libvirtd itself
lacks permissions to give CAP_DAC_OVERRIDE, but if that was the
case I would have expected an error from capng when trying to
grant it/.

>  - and possibly others.
> 
> [1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/qemu/qemu_process.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 47d8ca2ff163..2e2c4812fef7 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
>  }
>  
>  
> +static int
> +qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {
> +    /* We cannot use the security driver here, but we should not need to. */
> +    if (chown(proc->uniqDir, proc->runUid, -1) < 0) {
> +        virReportSystemError(errno,
> +                             "Cannot chown uniq path: %s", proc->uniqDir);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuProcessQMPInit(qemuProcessQMPPtr proc)
>  {
> @@ -8466,6 +8479,9 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
>          goto cleanup;
>      }
>  
> +    if (qemuProcessQEMULabelUniqPath(proc) < 0)
> +        goto cleanup;
> +
>      if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir,
>                      "qmp.monitor") < 0)
>          goto cleanup;
> -- 
> 2.21.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Martin Kletzander 4 years, 11 months ago
On Fri, Apr 12, 2019 at 02:45:32PM +0100, Daniel P. Berrangé wrote:
>On Fri, Apr 12, 2019 at 03:32:21PM +0200, Martin Kletzander wrote:
>> This does not cause a problem in usual scenarios thanks to us allowing
>> CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
>> an issue because the directory is created with mkdtemp(3) which explicitly
>> creates that with 0700 permissions and qemu running as non-root cannot access
>> that.
>>
>> The scenarios include:
>>  - Builds without CAPNG
>
>This makes sense, since we're unable to give QEMU CAP_DAC_OVERRIDE
>
>>  - Running libvirtd in a container [1]
>
>I'm rather puzzelled why this is a problem unless libvirtd itself
>lacks permissions to give CAP_DAC_OVERRIDE, but if that was the
>case I would have expected an error from capng when trying to
>grant it/.
>

That would make sense, but it clearly did not happen.  I'm not sure how that is
handled, maybe some other bug somewhere, maybe an obfuscating a cap detection, I
don't know.

>>  - and possibly others.
>>
>> [1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_process.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 47d8ca2ff163..2e2c4812fef7 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
>>  }
>>
>>
>> +static int
>> +qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {
>> +    /* We cannot use the security driver here, but we should not need to. */
>> +    if (chown(proc->uniqDir, proc->runUid, -1) < 0) {
>> +        virReportSystemError(errno,
>> +                             "Cannot chown uniq path: %s", proc->uniqDir);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static int
>>  qemuProcessQMPInit(qemuProcessQMPPtr proc)
>>  {
>> @@ -8466,6 +8479,9 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
>>          goto cleanup;
>>      }
>>
>> +    if (qemuProcessQEMULabelUniqPath(proc) < 0)
>> +        goto cleanup;
>> +
>>      if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir,
>>                      "qmp.monitor") < 0)
>>          goto cleanup;
>> --
>> 2.21.0
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Martin Kletzander 4 years, 11 months ago
On Fri, Apr 12, 2019 at 02:45:32PM +0100, Daniel P. Berrangé wrote:
>On Fri, Apr 12, 2019 at 03:32:21PM +0200, Martin Kletzander wrote:
>> This does not cause a problem in usual scenarios thanks to us allowing
>> CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
>> an issue because the directory is created with mkdtemp(3) which explicitly
>> creates that with 0700 permissions and qemu running as non-root cannot access
>> that.
>>
>> The scenarios include:
>>  - Builds without CAPNG
>
>This makes sense, since we're unable to give QEMU CAP_DAC_OVERRIDE
>
>>  - Running libvirtd in a container [1]
>
>I'm rather puzzelled why this is a problem unless libvirtd itself
>lacks permissions to give CAP_DAC_OVERRIDE, but if that was the
>case I would have expected an error from capng when trying to
>grant it/.
>
>>  - and possibly others.
>>
>> [1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_process.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 47d8ca2ff163..2e2c4812fef7 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
>>  }
>>
>>
>> +static int
>> +qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {

The bracket spacing is fixed locally in my branch and will be pushed as part of
this commit if this gets reviewed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Daniel P. Berrangé 4 years, 11 months ago
On Fri, Apr 12, 2019 at 03:32:21PM +0200, Martin Kletzander wrote:
> This does not cause a problem in usual scenarios thanks to us allowing
> CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
> an issue because the directory is created with mkdtemp(3) which explicitly
> creates that with 0700 permissions and qemu running as non-root cannot access
> that.
> 
> The scenarios include:
>  - Builds without CAPNG
>  - Running libvirtd in a container [1]

s/in a container/in certain container configurations/  since I'm sceptical
this is todo with containers in general, as opposed to some configuration
choice of the container used by kubevirt.

>  - and possibly others.
> 
> [1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/qemu/qemu_process.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 47d8ca2ff163..2e2c4812fef7 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
>  }
>  
>  
> +static int
> +qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {
> +    /* We cannot use the security driver here, but we should not need to. */
> +    if (chown(proc->uniqDir, proc->runUid, -1) < 0) {
> +        virReportSystemError(errno,
> +                             "Cannot chown uniq path: %s", proc->uniqDir);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuProcessQMPInit(qemuProcessQMPPtr proc)
>  {
> @@ -8466,6 +8479,9 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
>          goto cleanup;
>      }
>  
> +    if (qemuProcessQEMULabelUniqPath(proc) < 0)
> +        goto cleanup;
> +
>      if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir,
>                      "qmp.monitor") < 0)
>          goto cleanup;
> -- 
> 2.21.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Label uniqDir when probing capabilities
Posted by Martin Kletzander 4 years, 11 months ago
On Fri, Apr 12, 2019 at 03:54:26PM +0100, Daniel P. Berrangé wrote:
>On Fri, Apr 12, 2019 at 03:32:21PM +0200, Martin Kletzander wrote:
>> This does not cause a problem in usual scenarios thanks to us allowing
>> CAP_DAC_OVERRIDE for the qemu process, however in some scenarios this might be
>> an issue because the directory is created with mkdtemp(3) which explicitly
>> creates that with 0700 permissions and qemu running as non-root cannot access
>> that.
>>
>> The scenarios include:
>>  - Builds without CAPNG
>>  - Running libvirtd in a container [1]
>
>s/in a container/in certain container configurations/  since I'm sceptical
>this is todo with containers in general, as opposed to some configuration
>choice of the container used by kubevirt.
>

Oh, yes, much better wording as the container itself might not be related to
capabilities at all.  I'll fix that.  And those syntax-check failures as well.

>>  - and possibly others.
>>
>> [1] https://github.com/kubevirt/kubevirt/pull/2181#issuecomment-481840304
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/qemu/qemu_process.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>
>Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>

Thanks-by: Me

>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 47d8ca2ff163..2e2c4812fef7 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -8447,6 +8447,19 @@ qemuProcessQMPNew(const char *binary,
>>  }
>>
>>
>> +static int
>> +qemuProcessQEMULabelUniqPath(qemuProcessQMPPtr proc) {
>> +    /* We cannot use the security driver here, but we should not need to. */
>> +    if (chown(proc->uniqDir, proc->runUid, -1) < 0) {
>> +        virReportSystemError(errno,
>> +                             "Cannot chown uniq path: %s", proc->uniqDir);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static int
>>  qemuProcessQMPInit(qemuProcessQMPPtr proc)
>>  {
>> @@ -8466,6 +8479,9 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
>>          goto cleanup;
>>      }
>>
>> +    if (qemuProcessQEMULabelUniqPath(proc) < 0)
>> +        goto cleanup;
>> +
>>      if (virAsprintf(&proc->monpath, "%s/%s", proc->uniqDir,
>>                      "qmp.monitor") < 0)
>>          goto cleanup;
>> --
>> 2.21.0
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list