From 6ca02e37d72a81e7e32d4d3eef24d8a0abe3deb2 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Tue, 22 Mar 2022 13:53:41 +0000
Subject: [PATCH] lib: Improve security of in/out sockets when running virt-v2v
 as root
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(cherry picked from commit 4e7f206843735ba24e2034f694a214ef057ee139)
---
 lib/nbdkit.ml  |  3 ++-
 lib/qemuNBD.ml |  3 ++-
 lib/utils.ml   | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 lib/utils.mli  | 11 +++++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/lib/nbdkit.ml b/lib/nbdkit.ml
index 85621775..9ee6f39c 100644
--- a/lib/nbdkit.ml
+++ b/lib/nbdkit.ml
@@ -205,6 +205,7 @@ If the messages above are not sufficient to diagnose the problem then add the 
   (* Set the regular Unix permissions, in case nbdkit is
    * running as another user.
    *)
-  chmod socket 0o777;
+  chown_for_libvirt_rhbz_1045069 socket;
+  chmod socket 0o700;
 
   socket, pid
diff --git a/lib/qemuNBD.ml b/lib/qemuNBD.ml
index 54139ce0..2c999b9f 100644
--- a/lib/qemuNBD.ml
+++ b/lib/qemuNBD.ml
@@ -150,7 +150,8 @@ 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;
+  chmod socket 0o700;
 
   (* We don't need the PID file any longer. *)
   unlink pidfile;
diff --git a/lib/utils.ml b/lib/utils.ml
index 876a44c6..7116a4f9 100644
--- a/lib/utils.ml
+++ b/lib/utils.ml
@@ -147,6 +147,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)
+  )
+
+(* 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
+ *)
+and libvirt_qemu_user =
+  let user =
+    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
+  fun () -> Lazy.force user
+
 (* 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.
@@ -159,8 +203,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 c571cca5..d431e21f 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.31.1