Re: [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

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;