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 <[email protected]> (3) Looking at the code gave me one further idea, I'll post that separately. Thanks, Laszlo _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
