thr3ads.net - Libguestfs - [Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root [Mar 2022]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2022-Mar-22 21:21 UTC

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

When using the libvirt backend and running as root, libvirt will run
qemu as a non-root user (eg. qemu:qemu).  The v2v directory stores NBD
endpoints that qemu must be able to open and so we set the directory
to mode 0711.  Unfortunately this permits any non-root user to open
the sockets (since, by design, they have predictable names within the
directory).

Additionally we were setting the sockets themselves to 0777 mode.

Instead of using directory permissions, change the owner of the
directory and sockets to precisely give access to the qemu user and no
one else.

Reported-by: Xiaodai Wang
Thanks: Dr David Gilbert, Daniel Berrang?, Laszlo Ersek
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
---
 lib/nbdkit.ml  |  4 ++--
 lib/qemuNBD.ml |  2 +-
 lib/utils.ml   | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 lib/utils.mli  | 11 +++++++++++
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/lib/nbdkit.ml b/lib/nbdkit.ml
index 6787fbb083..9bd7963d0f 100644
--- a/lib/nbdkit.ml
+++ b/lib/nbdkit.ml
@@ -202,9 +202,9 @@ If the messages above are not sufficient to diagnose the
problem then add the
                          socket]);
   );
 
-  (* Set the regular Unix permissions, in case qemu is
+  (* Set the regular Unix permissions, in case nbdkit is
    * running as another user.
    *)
-  chmod socket 0o777;
+  chown_for_libvirt_rhbz_1045069 socket;
 
   socket, pid
diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
index 54139ce0b4..a5d3f03ba1 100644
--- a/lib/qemuNBD.ml
+++ b/lib/qemuNBD.ml
@@ -150,7 +150,7 @@ If the messages above are not sufficient to diagnose the
problem then add the
   (* Set the regular Unix permissions, in case qemu is
    * running as another user.
    *)
-  chmod socket 0o777;
+  chown_for_libvirt_rhbz_1045069 socket;
 
   (* We don't need the PID file any longer. *)
   unlink pidfile;
diff --git a/lib/utils.ml b/lib/utils.ml
index 757bc73c8e..40aa1545bf 100644
--- a/lib/utils.ml
+++ b/lib/utils.ml
@@ -146,6 +146,50 @@ let backend_is_libvirt ()    let backend = fst
(String.split ":" backend) in
   backend = "libvirt"
 
+let rec chown_for_libvirt_rhbz_1045069 file +  let running_as_root =
Unix.geteuid () = 0 in
+  if running_as_root && backend_is_libvirt () then (
+    try
+      let user = Option.default "qemu" (libvirt_qemu_user ()) in
+      let uid +        if String.is_prefix user "+" then
+          int_of_string (String.sub user 1 (String.length user - 1))
+        else
+          (Unix.getpwnam user).pw_uid in
+      debug "setting owner of %s to %d:root" file uid;
+      Unix.chown file uid 0
+    with
+    | exn -> (* Print exception, but continue. *)
+       debug "could not set owner of %s: %s"
+         file (Printexc.to_string exn)
+  )
+
+and libvirt_qemu_user () +(* Get the local user that libvirt uses to run qemu
when we are
+ * running as root.  This is returned as an optional string
+ * containing the username.  The username might be "+NNN"
+ * meaning a numeric UID.
+ * https://listman.redhat.com/archives/libguestfs/2022-March/028450.html
+ *)
+  let uid +    lazy (
+      let conn = Libvirt.Connect.connect_readonly () in
+      let xml = Libvirt.Connect.get_capabilities conn in
+      let doc = Xml.parse_memory xml in
+      let xpathctx = Xml.xpath_new_context doc in
+      let expr +       
"//secmodel[./model=\"dac\"]/baselabel[@type=\"kvm\"]/text()"
in
+      let uid_gid = Xpath_helpers.xpath_string xpathctx expr in
+      match uid_gid with
+      | None -> None
+      | Some uid_gid ->
+         (* The string will be something like "+107:+107", return the
+          * UID part.
+          *)
+         Some (fst (String.split ":" uid_gid))
+    ) in
+  Lazy.force uid
+
 (* When using the SSH driver in qemu (currently) this requires
  * ssh-agent authentication.  Give a clear error if this hasn't been
  * set up (RHBZ#1139973).  This might improve if we switch to libssh1.
@@ -158,8 +202,7 @@ let error_if_no_ssh_agent ()  (* Create the directory
containing inX and outX sockets. *)
 let create_v2v_directory ()    let d = Mkdtemp.temp_dir "v2v." in
-  let running_as_root = Unix.geteuid () = 0 in
-  if running_as_root then Unix.chmod d 0o711;
+  chown_for_libvirt_rhbz_1045069 d;
   On_exit.rmdir d;
   d
 
diff --git a/lib/utils.mli b/lib/utils.mli
index c571cca5db..d431e21f5c 100644
--- a/lib/utils.mli
+++ b/lib/utils.mli
@@ -61,6 +61,17 @@ val qemu_img_supports_offset_and_size : unit -> bool
 val backend_is_libvirt : unit -> bool
 (** Return true iff the current backend is libvirt. *)
 
+val chown_for_libvirt_rhbz_1045069 : string -> unit
+(** If running and root, and if the backend is libvirt, libvirt
+    will run qemu as a non-root user.  This prevents access
+    to root-owned files and directories.  To fix this, provide
+    a function to chown things we might need to qemu:root so
+    qemu can access them.  Note that root normally ignores
+    permissions so can still access the resource.
+
+    This is best-effort.  If something fails then we carry
+    on and hope for the best. *)
+
 val error_if_no_ssh_agent : unit -> unit
 
 val create_v2v_directory : unit -> string
-- 
2.35.1

Richard W.M. Jones

2022-Mar-22 21:32 UTC

head link

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

On Tue, Mar 22, 2022 at 09:21:26PM +0000, Richard W.M. Jones
wrote:> +and libvirt_qemu_user () > +  let uid > +    lazy (
...> +    ) in
> +  Lazy.force uid
Bleah, this but without the obviously bogus use of lazy().  I've fixed
this in my local copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Laszlo Ersek

2022-Mar-23 10:32 UTC

head link

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

On 03/22/22 22:21, Richard W.M. Jones wrote:> When using the libvirt backend and running as root, libvirt will run
> qemu as a non-root user (eg. qemu:qemu).  The v2v directory stores NBD
> endpoints that qemu must be able to open and so we set the directory
> to mode 0711.  Unfortunately this permits any non-root user to open
> the sockets (since, by design, they have predictable names within the
> directory).
>
> Additionally we were setting the sockets themselves to 0777 mode.
>
> Instead of using directory permissions, change the owner of the
> directory and sockets to precisely give access to the qemu user and no
> one else.
>
> Reported-by: Xiaodai Wang
> Thanks: Dr David Gilbert, Daniel Berrang?, Laszlo Ersek
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
> ---
>  lib/nbdkit.ml  |  4 ++--
>  lib/qemuNBD.ml |  2 +-
>  lib/utils.ml   | 47 +++++++++++++++++++++++++++++++++++++++++++++--
>  lib/utils.mli  | 11 +++++++++++
>  4 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/lib/nbdkit.ml b/lib/nbdkit.ml
> index 6787fbb083..9bd7963d0f 100644
> --- a/lib/nbdkit.ml
> +++ b/lib/nbdkit.ml
> @@ -202,9 +202,9 @@ If the messages above are not sufficient to diagnose
the problem then add the
>                           socket]);
>    );
>
> -  (* Set the regular Unix permissions, in case qemu is
> +  (* Set the regular Unix permissions, in case nbdkit is
>     * running as another user.
>     *)
> -  chmod socket 0o777;
> +  chown_for_libvirt_rhbz_1045069 socket;
>
>    socket, pid
> diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
> index 54139ce0b4..a5d3f03ba1 100644
> --- a/lib/qemuNBD.ml
> +++ b/lib/qemuNBD.ml
> @@ -150,7 +150,7 @@ If the messages above are not sufficient to diagnose
the problem then add the
>    (* Set the regular Unix permissions, in case qemu is
>     * running as another user.
>     *)
> -  chmod socket 0o777;
> +  chown_for_libvirt_rhbz_1045069 socket;
>
>    (* We don't need the PID file any longer. *)
>    unlink pidfile;
> diff --git a/lib/utils.ml b/lib/utils.ml
> index 757bc73c8e..40aa1545bf 100644
> --- a/lib/utils.ml
> +++ b/lib/utils.ml
> @@ -146,6 +146,50 @@ let backend_is_libvirt () >    let backend = fst
(String.split ":" backend) in
>    backend = "libvirt"
>
> +let rec chown_for_libvirt_rhbz_1045069 file > +  let running_as_root =
Unix.geteuid () = 0 in
> +  if running_as_root && backend_is_libvirt () then (
> +    try
> +      let user = Option.default "qemu" (libvirt_qemu_user ()) in
> +      let uid > +        if String.is_prefix user "+" then
> +          int_of_string (String.sub user 1 (String.length user - 1))
> +        else
> +          (Unix.getpwnam user).pw_uid in
> +      debug "setting owner of %s to %d:root" file uid;
> +      Unix.chown file uid 0
> +    with
> +    | exn -> (* Print exception, but continue. *)
> +       debug "could not set owner of %s: %s"
> +         file (Printexc.to_string exn)
> +  )
> +
> +and libvirt_qemu_user () > +(* Get the local user that libvirt uses to
run qemu when we are
> + * running as root.  This is returned as an optional string
> + * containing the username.  The username might be "+NNN"
> + * meaning a numeric UID.
> + * https://listman.redhat.com/archives/libguestfs/2022-March/028450.html
> + *)
> +  let uid > +    lazy (
> +      let conn = Libvirt.Connect.connect_readonly () in
> +      let xml = Libvirt.Connect.get_capabilities conn in
> +      let doc = Xml.parse_memory xml in
> +      let xpathctx = Xml.xpath_new_context doc in
> +      let expr > +       
"//secmodel[./model=\"dac\"]/baselabel[@type=\"kvm\"]/text()"
in
> +      let uid_gid = Xpath_helpers.xpath_string xpathctx expr in
> +      match uid_gid with
> +      | None -> None
> +      | Some uid_gid ->
> +         (* The string will be something like "+107:+107",
return the
> +          * UID part.
> +          *)
> +         Some (fst (String.split ":" uid_gid))
> +    ) in
> +  Lazy.force uid
> +
>  (* When using the SSH driver in qemu (currently) this requires
>   * ssh-agent authentication.  Give a clear error if this hasn't been
>   * set up (RHBZ#1139973).  This might improve if we switch to libssh1.
> @@ -158,8 +202,7 @@ let error_if_no_ssh_agent () >  (* Create the
directory containing inX and outX sockets. *)
>  let create_v2v_directory () >    let d = Mkdtemp.temp_dir
"v2v." in
> -  let running_as_root = Unix.geteuid () = 0 in
> -  if running_as_root then Unix.chmod d 0o711;
> +  chown_for_libvirt_rhbz_1045069 d;
>    On_exit.rmdir d;
>    d
>
> diff --git a/lib/utils.mli b/lib/utils.mli
> index c571cca5db..d431e21f5c 100644
> --- a/lib/utils.mli
> +++ b/lib/utils.mli
> @@ -61,6 +61,17 @@ val qemu_img_supports_offset_and_size : unit -> bool
>  val backend_is_libvirt : unit -> bool
>  (** Return true iff the current backend is libvirt. *)
>
> +val chown_for_libvirt_rhbz_1045069 : string -> unit
> +(** If running and root, and if the backend is libvirt, libvirt
> +    will run qemu as a non-root user.  This prevents access
> +    to root-owned files and directories.  To fix this, provide
> +    a function to chown things we might need to qemu:root so
> +    qemu can access them.  Note that root normally ignores
> +    permissions so can still access the resource.
> +
> +    This is best-effort.  If something fails then we carry
> +    on and hope for the best. *)
> +
>  val error_if_no_ssh_agent : unit -> unit
>
>  val create_v2v_directory : unit -> string
>
(1) The containing directory is created with Mkdtemp.temp_dir ->
guestfs_int_mllib_mkdtemp() -> mkdtemp(), and the latter is documented
to create the directory with file mode bits 0700. Thus, non-QEMU
processes will not have access. OK.

(2) With the chmods on the sockets removed, the unix domain sockets'
file mode bits will depend on the NBD server's umask. qemu-nbd does not
contain a umask call, so it inherits virt-v2v's; nbdkit does contain a
umask(0022) call. I think this is slightly inconsistent, so I'd suggest
squashing:
> diff --git a/lib/nbdkit.ml b/lib/nbdkit.ml
> index 9bd7963d0f38..9ee6f39ce335 100644
> --- a/lib/nbdkit.ml
> +++ b/lib/nbdkit.ml
> @@ -206,5 +206,6 @@ If the messages above are not sufficient to diagnose
the problem then add the
>     * running as another user.
>     *)
>    chown_for_libvirt_rhbz_1045069 socket;
> +  chmod socket 0o700;
>
>    socket, pid
> diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
> index a5d3f03ba1e2..2c999b9f8f8b 100644
> --- a/lib/qemuNBD.ml
> +++ b/lib/qemuNBD.ml
> @@ -151,6 +151,7 @@ If the messages above are not sufficient to diagnose
the problem then add the
>     * running as another user.
>     *)
>    chown_for_libvirt_rhbz_1045069 socket;
> +  chmod socket 0o700;
>
>    (* We don't need the PID file any longer. *)
>    unlink pidfile;
With or without this addition:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

(3) Looking at the code gave me one further idea, I'll post that
separately.

Thanks,
Laszlo

Libguestfs - Mar 2022 - [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root

[Libguestfs] [PATCH v2v v3] lib: Improve security of in/out sockets when running virt-v2v as root