From d88017e65c4eb814f7a522b11f3f952664c57c70 Mon Sep 17 00:00:00 2001 From: Pau Ruiz Safont Date: Mon, 22 Jul 2024 10:42:25 +0100 Subject: [PATCH 1/2] IH-507: xapi_xenops: raise an error when the kernel isn't allowed Previously the path was replaced by an empty string, when trying to start he vm. The only feedback was on the logs as a debug message, but not all users that start VMs have access to the logs. Signed-off-by: Pau Ruiz Safont --- ocaml/xapi/xapi_xenops.ml | 110 +++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 56 deletions(-) diff --git a/ocaml/xapi/xapi_xenops.ml b/ocaml/xapi/xapi_xenops.ml index 50aa2c6c53d..1da207c74f8 100644 --- a/ocaml/xapi/xapi_xenops.ml +++ b/ocaml/xapi/xapi_xenops.ml @@ -337,19 +337,43 @@ let rtc_timeoffset_of_vm ~__context (vm, vm_t) vbds = ) ) -(* /boot/ contains potentially sensitive files like xen-initrd, so we will only*) -(* allow directly booting guests from the subfolder /boot/guest/ *) +(* /boot/ contains potentially sensitive files like xen-initrd, only allow + directly booting guests from the subfolder /boot/guest/ *) let allowed_dom0_directories_for_boot_files = - ["/boot/guest/"; "/var/lib/xcp/guest"] - -let is_boot_file_whitelisted filename = - let safe_str str = not (String.has_substr str "..") in - (* make sure the script prefix is the allowed dom0 directory *) - List.exists - (fun allowed -> String.starts_with ~prefix:allowed filename) - allowed_dom0_directories_for_boot_files - (* avoid ..-style attacks and other weird things *) - && safe_str filename + ["/boot/guest/"; "/var/lib/xcp/guest/"] + +let kernel_path filename = + let ( let* ) = Result.bind in + let* real_path = + try Ok (Unix.realpath filename) with + | Unix.(Unix_error (ENOENT, _, _)) -> + let reason = "File does not exist" in + Error (filename, reason) + | exn -> + let reason = Printexc.to_string exn in + Error (filename, reason) + in + let* () = + match Unix.stat real_path with + | {st_kind= Unix.S_REG; _} -> + Ok () + | _ -> + let reason = "Is not a regular file" in + Error (filename, reason) + in + let allowed = + List.exists + (fun allowed -> String.starts_with ~prefix:allowed real_path) + allowed_dom0_directories_for_boot_files + in + if not allowed then + let reason = + Printf.sprintf "Is not in any of the allowed kernel directories: [%s]" + (String.concat "; " allowed_dom0_directories_for_boot_files) + in + Error (filename, reason) + else + Ok real_path let builder_of_vm ~__context (vmref, vm) timeoffset pci_passthrough vgpu = let open Vm in @@ -372,19 +396,12 @@ let builder_of_vm ~__context (vmref, vm) timeoffset pci_passthrough vgpu = Cirrus in let pci_emulations = - let s = - try Some (List.assoc "mtc_pci_emulations" vm.API.vM_other_config) - with _ -> None - in + let s = List.assoc_opt "mtc_pci_emulations" vm.API.vM_other_config in match s with | None -> [] - | Some x -> ( - try - let l = String.split ',' x in - List.map (String.strip String.isspace) l - with _ -> [] - ) + | Some x -> + String.split_on_char ',' x |> List.map String.trim in let make_hvmloader_boot_record () = if bool vm.API.vM_platform false "qemu_stubdom" then @@ -427,15 +444,10 @@ let builder_of_vm ~__context (vmref, vm) timeoffset pci_passthrough vgpu = ; acpi= bool vm.API.vM_platform true "acpi" ; serial= ((* The platform value should override the other_config value. If - * neither are set, use pty. *) + neither are set, use pty. *) let key = "hvm_serial" in - let other_config_value = - try Some (List.assoc key vm.API.vM_other_config) - with Not_found -> None - in - let platform_value = - try Some (List.assoc key vm.API.vM_platform) with Not_found -> None - in + let other_config_value = List.assoc_opt key vm.API.vM_other_config in + let platform_value = List.assoc_opt key vm.API.vM_platform in match (other_config_value, platform_value) with | None, None -> Some "pty" @@ -444,10 +456,7 @@ let builder_of_vm ~__context (vmref, vm) timeoffset pci_passthrough vgpu = | Some value, None -> Some value ) - ; keymap= - ( try Some (List.assoc "keymap" vm.API.vM_platform) - with Not_found -> None - ) + ; keymap= List.assoc_opt "keymap" vm.API.vM_platform ; vnc_ip= None (*None PR-1255*) ; pci_emulations ; pci_passthrough @@ -464,30 +473,19 @@ let builder_of_vm ~__context (vmref, vm) timeoffset pci_passthrough vgpu = ; tpm= tpm_of_vm () } in - let make_direct_boot_record - {Helpers.kernel= k; kernel_args= ka; ramdisk= initrd} = - let k = - if is_boot_file_whitelisted k then - k - else ( - debug "kernel %s is not in the whitelist: ignoring" k ; - "" - ) - in - let initrd = - Option.map - (fun x -> - if is_boot_file_whitelisted x then - x - else ( - debug "initrd %s is not in the whitelist: ignoring" k ; - "" - ) - ) - initrd + let make_direct_boot_record {Helpers.kernel; kernel_args= ka; ramdisk} = + let resolve name ~path = + match kernel_path path with + | Ok k -> + k + | Error (file, msg) -> + info {|%s: refusing to load %s "%s": %s|} __FUNCTION__ name file msg ; + raise Api_errors.(Server_error (invalid_value, [name; file; msg])) in + let kernel = resolve "kernel" ~path:kernel in + let ramdisk = Option.map (fun k -> resolve "ramdisk" ~path:k) ramdisk in { - boot= Direct {kernel= k; cmdline= ka; ramdisk= initrd} + boot= Direct {kernel; cmdline= ka; ramdisk} ; framebuffer= bool vm.API.vM_platform false "pvfb" ; framebuffer_ip= None (* None PR-1255 *) ; vncterm= not (List.mem_assoc "disable_pv_vnc" vm.API.vM_other_config) From 5dc2900f1f808c2b49a6ecbda3720b0b8d70a917 Mon Sep 17 00:00:00 2001 From: Pau Ruiz Safont Date: Tue, 23 Jul 2024 11:23:07 +0100 Subject: [PATCH 2/2] IH-507: Do not allow guest kernels in /boot/ This location is for dom0's boot chain exclusively Signed-off-by: Pau Ruiz Safont --- ocaml/xapi/xapi_xenops.ml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ocaml/xapi/xapi_xenops.ml b/ocaml/xapi/xapi_xenops.ml index 1da207c74f8..cb1932aab0a 100644 --- a/ocaml/xapi/xapi_xenops.ml +++ b/ocaml/xapi/xapi_xenops.ml @@ -337,10 +337,7 @@ let rtc_timeoffset_of_vm ~__context (vm, vm_t) vbds = ) ) -(* /boot/ contains potentially sensitive files like xen-initrd, only allow - directly booting guests from the subfolder /boot/guest/ *) -let allowed_dom0_directories_for_boot_files = - ["/boot/guest/"; "/var/lib/xcp/guest/"] +let allowed_dom0_directories_for_boot_files = ["/var/lib/xcp/guest/"] let kernel_path filename = let ( let* ) = Result.bind in