The existing QEMU URI path validation considers that a
privileged user can use a "/session" URI as well. Let's update
virConnectValidateURIPath() to consider this usage, allowing us
to use the function inside qemu_driver.c.

Although the existing callers of virConnectValidateURIPath()
didn't consider that a privileged user could connect with
"/session", it is sensible to consider that this should be
the case. thus no serious harm is done.

Suggested-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/driver.c           |  3 ++-
 src/qemu/qemu_driver.c | 20 ++++----------------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/src/driver.c b/src/driver.c
index e627b0c1d7..f9c41383dc 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -276,7 +276,8 @@ virConnectValidateURIPath(const char *uriPath,
                           bool privileged)
 {
     if (privileged) {
-        if (STRNEQ(uriPath, "/system")) {
+        if (STRNEQ(uriPath, "/system") &&
+            STRNEQ(uriPath, "/session")) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("unexpected %s URI path '%s', try %s:///system"),
                            entityName, uriPath, entityName);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0753904472..05025e7251 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1295,22 +1295,10 @@ static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn,
         return VIR_DRV_OPEN_ERROR;
     }
 
-    if (virQEMUDriverIsPrivileged(qemu_driver)) {
-        if (STRNEQ(conn->uri->path, "/system") &&
-            STRNEQ(conn->uri->path, "/session")) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unexpected QEMU URI path '%s', try qemu:///system"),
-                           conn->uri->path);
-            return VIR_DRV_OPEN_ERROR;
-        }
-    } else {
-        if (STRNEQ(conn->uri->path, "/session")) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unexpected QEMU URI path '%s', try qemu:///session"),
-                           conn->uri->path);
-            return VIR_DRV_OPEN_ERROR;
-        }
-    }
+    if (!virConnectValidateURIPath(conn->uri->path,
+                                   "QEMU",
+                                   virQEMUDriverIsPrivileged(qemu_driver)))
+        return VIR_DRV_OPEN_ERROR;
 
     if (virConnectOpenEnsureACL(conn) < 0)
         return VIR_DRV_OPEN_ERROR;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 23, 2019 at 11:03:41AM -0300, Daniel Henrique Barboza wrote:
> The existing QEMU URI path validation considers that a
> privileged user can use a "/session" URI as well. Let's update
> virConnectValidateURIPath() to consider this usage, allowing us
> to use the function inside qemu_driver.c.
> 
> Although the existing callers of virConnectValidateURIPath()
> didn't consider that a privileged user could connect with
> "/session", it is sensible to consider that this should be
> the case. thus no serious harm is done.

That we allowed use of '/session' while connecting as root is a bug
IMHO. Ideally we're kill that off entirely, so there's potential for
us to allow a genuine session instance daemon as root at a later date.

I'm just concerned about whether there might be apps relying on this
bug.

I certainly don't want to see the bug spread across all drivers
though.

> 
> Suggested-by: Cole Robinson <crobinso@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  src/driver.c           |  3 ++-
>  src/qemu/qemu_driver.c | 20 ++++----------------
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/src/driver.c b/src/driver.c
> index e627b0c1d7..f9c41383dc 100644
> --- a/src/driver.c
> +++ b/src/driver.c
> @@ -276,7 +276,8 @@ virConnectValidateURIPath(const char *uriPath,
>                            bool privileged)
>  {
>      if (privileged) {
> -        if (STRNEQ(uriPath, "/system")) {
> +        if (STRNEQ(uriPath, "/system") &&
> +            STRNEQ(uriPath, "/session")) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unexpected %s URI path '%s', try %s:///system"),
>                             entityName, uriPath, entityName);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0753904472..05025e7251 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1295,22 +1295,10 @@ static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn,
>          return VIR_DRV_OPEN_ERROR;
>      }
>  
> -    if (virQEMUDriverIsPrivileged(qemu_driver)) {
> -        if (STRNEQ(conn->uri->path, "/system") &&
> -            STRNEQ(conn->uri->path, "/session")) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unexpected QEMU URI path '%s', try qemu:///system"),
> -                           conn->uri->path);
> -            return VIR_DRV_OPEN_ERROR;
> -        }
> -    } else {
> -        if (STRNEQ(conn->uri->path, "/session")) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unexpected QEMU URI path '%s', try qemu:///session"),
> -                           conn->uri->path);
> -            return VIR_DRV_OPEN_ERROR;
> -        }
> -    }
> +    if (!virConnectValidateURIPath(conn->uri->path,
> +                                   "QEMU",
> +                                   virQEMUDriverIsPrivileged(qemu_driver)))
> +        return VIR_DRV_OPEN_ERROR;
>  
>      if (virConnectOpenEnsureACL(conn) < 0)
>          return VIR_DRV_OPEN_ERROR;
> -- 
> 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

On 9/23/19 11:51 AM, Daniel P. Berrangé wrote:
> On Mon, Sep 23, 2019 at 11:03:41AM -0300, Daniel Henrique Barboza wrote:
>> The existing QEMU URI path validation considers that a
>> privileged user can use a "/session" URI as well. Let's update
>> virConnectValidateURIPath() to consider this usage, allowing us
>> to use the function inside qemu_driver.c.
>>
>> Although the existing callers of virConnectValidateURIPath()
>> didn't consider that a privileged user could connect with
>> "/session", it is sensible to consider that this should be
>> the case. thus no serious harm is done.
> That we allowed use of '/session' while connecting as root is a bug
> IMHO. Ideally we're kill that off entirely, so there's potential for
> us to allow a genuine session instance daemon as root at a later date.
>
> I'm just concerned about whether there might be apps relying on this
> bug.

In QEMU I believe this would be considered a sort of "API break",
then it would be put in a deprecation cycle and, after the cycle (last
time I checked it was about 2 years, but I might be wrong) it would be
permanently changed.

Perhaps a similar approach can be taken here in Libvirt. I mean, in
theory we could simply change it, claiming that it is a bug fix and every
other driver is forbidding root with 'session' already. In reality,
people will get mad about it ...

>
> I certainly don't want to see the bug spread across all drivers
> though.


I'll resubmit the patch series, creating a new function to emulate
the current behavior of qemu_driver, vbox_common and vbox_driver,
without changing the behavior of everyone else. I'll put a comment
in it to document the problem you mentioned above. At least
we have this issue documented in the code (perhaps a TODO/FIXME
tag is warranted?) and it's centralized in one place.


Thanks,


DHB


>
>> Suggested-by: Cole Robinson <crobinso@redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   src/driver.c           |  3 ++-
>>   src/qemu/qemu_driver.c | 20 ++++----------------
>>   2 files changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/driver.c b/src/driver.c
>> index e627b0c1d7..f9c41383dc 100644
>> --- a/src/driver.c
>> +++ b/src/driver.c
>> @@ -276,7 +276,8 @@ virConnectValidateURIPath(const char *uriPath,
>>                             bool privileged)
>>   {
>>       if (privileged) {
>> -        if (STRNEQ(uriPath, "/system")) {
>> +        if (STRNEQ(uriPath, "/system") &&
>> +            STRNEQ(uriPath, "/session")) {
>>               virReportError(VIR_ERR_INTERNAL_ERROR,
>>                              _("unexpected %s URI path '%s', try %s:///system"),
>>                              entityName, uriPath, entityName);
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 0753904472..05025e7251 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1295,22 +1295,10 @@ static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn,
>>           return VIR_DRV_OPEN_ERROR;
>>       }
>>   
>> -    if (virQEMUDriverIsPrivileged(qemu_driver)) {
>> -        if (STRNEQ(conn->uri->path, "/system") &&
>> -            STRNEQ(conn->uri->path, "/session")) {
>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           _("unexpected QEMU URI path '%s', try qemu:///system"),
>> -                           conn->uri->path);
>> -            return VIR_DRV_OPEN_ERROR;
>> -        }
>> -    } else {
>> -        if (STRNEQ(conn->uri->path, "/session")) {
>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           _("unexpected QEMU URI path '%s', try qemu:///session"),
>> -                           conn->uri->path);
>> -            return VIR_DRV_OPEN_ERROR;
>> -        }
>> -    }
>> +    if (!virConnectValidateURIPath(conn->uri->path,
>> +                                   "QEMU",
>> +                                   virQEMUDriverIsPrivileged(qemu_driver)))
>> +        return VIR_DRV_OPEN_ERROR;
>>   
>>       if (virConnectOpenEnsureACL(conn) < 0)
>>           return VIR_DRV_OPEN_ERROR;
>> -- 
>> 2.21.0
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> Regards,
> Daniel

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list