Xen Security Advisory 530 v2 (CVE-2026-102706,CVE-2026-102707,CVE-2026-102708,CVE-2026-119779,CVE-2026-119780) - XAPI: Multiple RBAC issues (update)

index 5f1bdae2fd79..ef46341cd143 100644
@@ -2579,8 +2579,8 @@ let match_protected_key objname fieldname =
    lower-privileged sessions must be able to manipulate a subset of entries
    (those not protected by a more privileged role).
 *)
-let set_map_with_rbac ~__context ~self ~value ~get_fn ~set_fn ~match_protected
-    ~object_name ~field_name =
+let set_map_with_rbac' ~session_id ~__context ~self ~value ~get_fn ~set_fn
+    ~match_protected ~object_name ~field_name =
   let match_protected = match_protected field_name in
   let module S = Set.Make (String) in
   let create_lookup kvs =

@@ -2647,7 +2647,6 @@ let set_map_with_rbac ~__context ~self ~value ~get_fn ~set_fn ~match_protected
   in
   (* Find the first disallowed permission, indicating that we cannot
      perform the action. *)
-  let session_id = Context.get_session_id __context in
   match
     Rbac.find_first_disallowed_permission ~__context ~session_id ~permissions
   with

@@ -2663,3 +2662,76 @@ let set_map_with_rbac ~__context ~self ~value ~get_fn ~set_fn ~match_protected
         (Rbac.disallowed_permission_exn ~extra_dmsg ~extra_msg ~__context
            ~permission:disallowed ~action
         )
+
+let set_map_with_rbac ~__context ~self ~value ~get_fn ~set_fn ~match_protected
+    ~object_name ~field_name =
+  set_map_with_rbac'
+    ~session_id:(Context.get_session_id __context)
+    ~__context ~self ~value ~get_fn ~set_fn ~match_protected ~object_name
+    ~field_name
+
+(* Try to recover the original session from the task as it is still useful even
+   after passing the RBAC check in API message entry. For example, RBAC check
+   against some keys is only possible when the VM's metadata is received from an
+   input stream while the call has passed RBAC check in API message entry on
+   coordinator and has been forwarded to a supporter host. *)
+let original_session ~__context =
+  match Context.get_session_id __context with
+  | exception Failure _ ->
+      (* No session, check is not needed. *)
+      Ok None
+  | session_id ->
+      let is_forwarding_session =
+        (* Quick check on local session. *)
+        (not (Session_check.is_local_session __context session_id))
+        (* Quick check on forwarded_task as well. *)
+        && Context.forwarded_task __context
+        &&
+        let session = Db.Session.get_record ~__context ~self:session_id in
+        session.API.session_pool && session.API.session_is_local_superuser
+      in
+      if is_forwarding_session then (
+        let ( let* ) = Result.bind in
+        let* task_id =
+          if Context.task_in_database __context then
+            Ok (Context.get_task_id __context)
+          else (
+            warn "%s: no task" __FUNCTION__ ;
+            Error ()
+          )
+        in
+        match Db.Task.get_session ~__context ~self:task_id with
+        | session_id when session_id <> Ref.null ->
+            Ok (Some session_id)
+        | _ ->
+            warn "%s: no session" __FUNCTION__ ;
+            Error ()
+        | exception e ->
+            warn "%s: failed to get originating session: %s" __FUNCTION__
+              (Printexc.to_string e) ;
+            Error ()
+      ) else
+        Ok (Some session_id)
+
+(* With similar purpose of set_map_with_rbac, this is a pure checking function
+   to ensure the session is privileged enough to set values for the protected
+   keys if they present in the map [value]. *)
+let assert_map_with_rbac ~__context ~match_protected ~object_name ~field_name
+    ~value =
+  match original_session ~__context with
+  | Ok None ->
+      ()
+  | Ok (Some session_id) ->
+      set_map_with_rbac' ~session_id ~__context ~self:Ref.null ~value
+        ~get_fn:(fun ~__context:_ ~self:_ -> [])
+        ~set_fn:(fun ~__context:_ ~self:_ ~value:_ -> ())
+        ~match_protected ~object_name ~field_name
+  | Error () ->
+      let action = Printf.sprintf "%s.set_%s" object_name field_name in
+      raise
+        Api_errors.(
+          Server_error
+            ( rbac_permission_denied
+            , [action; "cannot determine the originating session"]
+            )
+        )

index 067108cd8f81..b71fb7c2dbd0 100644
@@ -517,6 +517,10 @@ module VM : HandlerTools = struct
         in
         match import_action with
         | Replace (_, vm_record) | Clean_import vm_record ->
+            Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+              ~field_name:"other_config" ~value:vm_record.API.vM_other_config ;
+            Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+              ~field_name:"platform" ~value:vm_record.API.vM_platform ;
             if is_live config then assert_can_live_import __context vm_record ;
             ( if needs_cpu_check config vm_record then
                 let vmm_record =

index f474ca306cb6..2573cb8af024 100644
@@ -591,6 +591,10 @@ let create ~__context ~name_label ~name_description ~power_state ~user_version
     else
       other_config
   in
+  Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+    ~field_name:"other_config" ~value:other_config ;
+  Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+    ~field_name:"platform" ~value:platform ;
   (* NB apart from the above, parameter validation is delayed until VM.start *)
   let uuid = Uuidx.make () in
   let vm_ref = Ref.make () in

index 25a30eac2785..ea76589db52b 100644
@@ -419,10 +419,23 @@ let copy_vm_record ?snapshot_info_record ~__context ~vm ~disk_op ~new_name
 let make_driver_params () =
   [(Constants._sm_epoch_hint, Uuidx.to_string (Uuidx.make ()))]
 
+let assert_protected_map_keys_allowed ~__context ~vm disk_op =
+  match disk_op with
+  | Disk_op_clone | Disk_op_copy _ ->
+      Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+        ~field_name:"other_config"
+        ~value:(Db.VM.get_other_config ~__context ~self:vm) ;
+      Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
+        ~field_name:"platform"
+        ~value:(Db.VM.get_platform ~__context ~self:vm)
+  | Disk_op_snapshot | Disk_op_checkpoint ->
+      ()
+
 (* NB this function may be called when the VM is suspended for copy/clone
    operations. Snapshot can be done in live. *)
 let clone ?snapshot_info_record ?(ignore_vdis = []) disk_op ~__context ~vm
     ~new_name =
+  assert_protected_map_keys_allowed ~__context ~vm disk_op ;
   Helpers.call_api_functions ~__context (fun rpc session_id ->
       let task_id = Ref.string_of (Context.get_task_id __context) in
       let vbds = Db.VM.get_VBDs ~__context ~self:vm in

index f71ab3fd3b36..bcb5e8875cb7 100644
@@ -1738,3 +1738,8 @@ let check_secureboot_certificates_state ~__context ~self :
         vm_uuid (Printexc.to_string e) ;
       `ok
   )
+
+let assert_protected_map_keys_allowed ~__context ~field_name ~value =
+  let match_protected = Helpers.match_protected_key "VM" in
+  Helpers.assert_map_with_rbac ~__context ~match_protected ~object_name:"vm"
+    ~field_name ~value

index caedfc564108..8a1b43bcf7dd 100644
@@ -73,4 +73,5 @@ let () =
     @ Test_rfb_client_msgtype_parser.tests
     @ Test_tracked_user_agents.tests
     @ Test_cli_args.tests
+    @ Test_vm_protected_keys.tests
     )

404 lines not on this page · raw message

index 05843952fca7..16e13a7a9c5d 100644
@@ -62,3 +62,7 @@ val clone :
   -> vm:[`VM] API.Ref.t
   -> new_name:string
   -> [`VM] Ref.t
+
+(* For test purpose only *)
+val assert_protected_map_keys_allowed :
+  __context:Context.t -> vm:[`VM] API.Ref.t -> disk_op_t -> unit

index ef46341cd143..c4cd232b1256 100644
@@ -2735,3 +2735,28 @@ let assert_map_with_rbac ~__context ~match_protected ~object_name ~field_name
             , [action; "cannot determine the originating session"]
             )
         )
+
+(* With similar purpose of assert_map_with_rbac, check if setting a non-default
+   value on a field of an object is allowed. *)
+let assert_field_value_with_rbac ~__context ~object_name ~field_name ~is_default
+    =
+  if not is_default then
+    let action = Printf.sprintf "%s.set_%s" object_name field_name in
+    let permission = String.lowercase_ascii action in
+    match original_session ~__context with
+    | Ok None ->
+        ()
+    | Ok (Some session_id) ->
+        if not (Rbac.is_access_allowed ~__context ~session_id ~permission) then
+          raise
+            (Rbac.disallowed_permission_exn ~extra_dmsg:"" ~extra_msg:""
+               ~__context ~permission ~action
+            )
+    | Error () ->
+        raise
+          Api_errors.(
+            Server_error
+              ( rbac_permission_denied
+              , [action; "cannot determine the originating session"]
+              )
+          )

index b71fb7c2dbd0..16ebb4c9264c 100644
@@ -521,6 +521,10 @@ module VM : HandlerTools = struct
               ~field_name:"other_config" ~value:vm_record.API.vM_other_config ;
             Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
               ~field_name:"platform" ~value:vm_record.API.vM_platform ;
+            (* The appliance is cleared on import, so it isn't checked. *)
+            Xapi_vm_helpers.assert_protected_field_allowed ~__context
+              ~field_name:"ha_restart_priority"
+              ~is_default:(vm_record.API.vM_ha_restart_priority = "") ;
             if is_live config then assert_can_live_import __context vm_record ;
             ( if needs_cpu_check config vm_record then
                 let vmm_record =

index 2573cb8af024..da87a18752c4 100644
@@ -595,6 +595,10 @@ let create ~__context ~name_label ~name_description ~power_state ~user_version
     ~field_name:"other_config" ~value:other_config ;
   Xapi_vm_helpers.assert_protected_map_keys_allowed ~__context
     ~field_name:"platform" ~value:platform ;
+  Xapi_vm_helpers.assert_protected_field_allowed ~__context
+    ~field_name:"ha_restart_priority" ~is_default:(ha_restart_priority = "") ;
+  Xapi_vm_helpers.assert_protected_field_allowed ~__context
+    ~field_name:"appliance" ~is_default:(appliance = Ref.null) ;
   (* NB apart from the above, parameter validation is delayed until VM.start *)
   let uuid = Uuidx.make () in
   let vm_ref = Ref.make () in

index ea76589db52b..3d62851e5209 100644
@@ -431,11 +431,22 @@ let assert_protected_map_keys_allowed ~__context ~vm disk_op =
   | Disk_op_snapshot | Disk_op_checkpoint ->
       ()
 
+let assert_protected_fields_allowed ~__context ~vm disk_op =
+  match disk_op with
+  | Disk_op_clone | Disk_op_copy _ ->
+      (* The appliance of the new VM is reset, so it isn't checked. *)
+      Xapi_vm_helpers.assert_protected_field_allowed ~__context
+        ~field_name:"ha_restart_priority"
+        ~is_default:(Db.VM.get_ha_restart_priority ~__context ~self:vm = "")
+  | Disk_op_snapshot | Disk_op_checkpoint ->
+      ()
+
 (* NB this function may be called when the VM is suspended for copy/clone
    operations. Snapshot can be done in live. *)
 let clone ?snapshot_info_record ?(ignore_vdis = []) disk_op ~__context ~vm
     ~new_name =
   assert_protected_map_keys_allowed ~__context ~vm disk_op ;
+  assert_protected_fields_allowed ~__context ~vm disk_op ;
   Helpers.call_api_functions ~__context (fun rpc session_id ->
       let task_id = Ref.string_of (Context.get_task_id __context) in
       let vbds = Db.VM.get_VBDs ~__context ~self:vm in

index bcb5e8875cb7..c6270a6a8245 100644
@@ -1743,3 +1743,7 @@ let assert_protected_map_keys_allowed ~__context ~field_name ~value =
   let match_protected = Helpers.match_protected_key "VM" in
   Helpers.assert_map_with_rbac ~__context ~match_protected ~object_name:"vm"
     ~field_name ~value
+
+let assert_protected_field_allowed ~__context ~field_name ~is_default =
+  Helpers.assert_field_value_with_rbac ~__context ~object_name:"vm" ~field_name
+    ~is_default

index 8a1b43bcf7dd..3e13d3fbccad 100644
@@ -74,4 +74,5 @@ let () =
     @ Test_tracked_user_agents.tests
     @ Test_cli_args.tests
     @ Test_vm_protected_keys.tests
+    @ Test_vm_protected_fields.tests
     )

300 lines not on this page · raw message

index 16e13a7a9c5d..b0936d9301fd 100644
@@ -66,3 +66,7 @@ val clone :
 (* For test purpose only *)
 val assert_protected_map_keys_allowed :
   __context:Context.t -> vm:[`VM] API.Ref.t -> disk_op_t -> unit
+
+(* For test purpose only *)
+val assert_protected_fields_allowed :
+  __context:Context.t -> vm:[`VM] API.Ref.t -> disk_op_t -> unit

index 861e48852207..97e26278ccc3 100644
@@ -1493,6 +1493,11 @@ module Network = struct
                   ("folder", _R_VM_OP)
                 ; ("XenCenter.CustomFields.*", _R_VM_OP)
                 ; ("XenCenterCreateInProgress", _R_VM_OP)
+                ; (* Host Internal Management Network markers. Redundant for
+                     the writer_roles, but they generate the per-key rules that
+                     Network.create checks, its default role being VM admin. *)
+                  ("is_guest_installer_network", _R_POOL_OP)
+                ; ("is_host_internal_management_network", _R_POOL_OP)
                 ]
               ~published:rel_rio
           ; field

index 16ebb4c9264c..080582b47309 100644
@@ -1273,6 +1273,17 @@ module Net : HandlerTools = struct
     | Found_net _ ->
         handle_dry_run __context config rpc session_id state x precheck_result
     | Create net_record ->
+        (* The [session_id] passed to Client.Network.create_from_record
+           (and hence to Network.create) may have been created by
+           Helpers.call_api_functions with a higher privilege level than the
+           requesting client session. This is intentional, but it means the
+           per-key RBAC check inside Network.create would evaluate that
+           elevated session and let protected keys through. The import
+           metadata is caller-controlled, so enforce the check here instead,
+           where Helpers.assert_map_with_rbac can still resolve the
+           originating session. *)
+        Xapi_network_helpers.assert_protected_map_keys_allowed ~__context
+          ~field_name:"other_config" ~value:net_record.API.network_other_config ;
         let net =
           log_reraise
             ("failed to create Network with name_label "

index dfa15d0a18ab..d35de92696a5 100644
@@ -235,6 +235,8 @@ let rec choose_bridge_name bridges =
 
 let create ~__context ~name_label ~name_description ~mTU ~other_config ~bridge
     ~managed ~tags =
+  Xapi_network_helpers.assert_protected_map_keys_allowed ~__context
+    ~field_name:"other_config" ~value:other_config ;
   with_lock mutex (fun () ->
       let networks = Db.Network.get_all ~__context in
       let bridges =

index 0a6bcd47c0e2..0e1932abfda6 100644
@@ -123,3 +123,8 @@ let assert_network_compatible_with_sriov ~__context ~pif ~network =
                 )
             )
   )
+
+let assert_protected_map_keys_allowed ~__context ~field_name ~value =
+  let match_protected = Helpers.match_protected_key "network" in
+  Helpers.assert_map_with_rbac ~__context ~match_protected
+    ~object_name:"network" ~field_name ~value

index 080582b47309..9a2254e13998 100644
@@ -1284,6 +1284,12 @@ module Net : HandlerTools = struct
            originating session. *)
         Xapi_network_helpers.assert_protected_map_keys_allowed ~__context
           ~field_name:"other_config" ~value:net_record.API.network_other_config ;
+        (* The bridge from the import is meaningless to this pool.
+           Network.create will choose the bridge. A network created during
+           import must be a managed one. *)
+        let net_record =
+          {net_record with API.network_bridge= ""; network_managed= true}
+        in
         let net =
           log_reraise
             ("failed to create Network with name_label "

index 3e13d3fbccad..184a56c92a7e 100644
@@ -75,4 +75,5 @@ let () =
     @ Test_cli_args.tests
     @ Test_vm_protected_keys.tests
     @ Test_vm_protected_fields.tests
+    @ Test_network_protected_keys.tests
     )

264 lines not on this page · raw message

92 lines not on this page · raw message

index 04671057c5e5..72bfd8ae785a 100644
@@ -54,6 +54,7 @@
     xapi_database
     xapi_internal
     xml-light2
+    xmlm
   )
   (deps
     (source_tree test_data)

index 184a56c92a7e..25c2130caabe 100644
@@ -46,6 +46,7 @@ let () =
      ; ("Test_bios_strings", Test_bios_strings.test)
      ; ("Test_certificates", Test_certificates.test)
      ; ("Test_caller_limit", Test_caller_limit.test)
+     ; ("Test_message", Test_message.test)
      ]
     @ Test_guest_agent.tests
     @ Test_nm.tests

215 lines not on this page · raw message

index 326e47ee463b..eedf1b163427 100644
@@ -126,6 +126,18 @@ module Helper = struct
   end)
 end
 
+let can_own_task ?session_id task_id =
+  try
+    Context.with_context ~quiet:true ?session_id "can_own_task"
+      (fun __context -> TaskHelper.assert_op_valid ~__context task_id
+    ) ;
+    true
+  with e ->
+    warn "Task %s provided in the request can't be handled by the caller: %s"
+      (Ref.string_of task_id)
+      (ExnHelper.string_of_exn e) ;
+    false
+
 let do_dispatch ?session_id ?forward_op ?self:_ supports_async called_fn_name
     op_fn marshaller fd http_req label sync_ty generate_task_for =
   (* if the call has been forwarded to us, then they are responsible for completing the task, so we don't need to complete it *)

@@ -140,6 +152,20 @@ let do_dispatch ?session_id ?forward_op ?self:_ supports_async called_fn_name
       Context.of_http_req ?session_id ~internal_async_subtask ~generate_task_for
         ~supports_async ~label ~http_req ~fd ()
     in
+    if
+      Context.task_in_database __context
+      && not (can_own_task ?session_id (Context.get_task_id __context))
+    then
+      raise
+        Api_errors.(
+          Server_error
+            ( rbac_permission_denied
+            , [
+                String.lowercase_ascii called_fn_name
+              ; "The task provided in the request belongs to another subject."
+              ]
+            )
+        ) ;
 
     Constants.when_tgroups_enabled (fun () ->
         let identity =

index 9155469e26ba..6944db32ea86 100644
@@ -39,6 +39,13 @@ val exec_with_subtask :
   -> (__context:Context.t -> 'b)
   -> 'b
 
+val can_own_task : ?session_id:API.ref_session -> API.ref_task -> bool
+(** [can_own_task ?session_id task] is true when [session_id] can handle
+    [task]. A task provided by a request is caller-controlled, so it has to be
+    checked before anything is done with it. Only call this for a task
+    that is really in the database: the check cannot resolve a dummy ref and
+    would refuse every unprivileged caller. *)
+
 (* used by auto-generated code in server.ml *)
 val my_assoc : string -> (string * 'a) list -> 'a
 

index 1990ac2ce7ad..56a42d9a88c9 100644
@@ -62,13 +62,19 @@ let rbac_assert_permission_fn = ref None
 
 let are_auth_user_ids_of_sessions_equal ~__context s1 s2 =
   Context.with_tracing ~__context __FUNCTION__ @@ fun __context ->
-  let s1_auth_user_sid =
-    Db_actions.DB_Action.Session.get_auth_user_sid ~__context ~self:s1
-  in
-  let s2_auth_user_sid =
-    Db_actions.DB_Action.Session.get_auth_user_sid ~__context ~self:s2
-  in
-  s1_auth_user_sid = s2_auth_user_sid
+  let open Db_actions.DB_Action.Session in
+  let s1_auth_user_sid = get_auth_user_sid ~__context ~self:s1 in
+  let s2_auth_user_sid = get_auth_user_sid ~__context ~self:s2 in
+  match s1_auth_user_sid = s2_auth_user_sid with
+  | false ->
+      false
+  | true when s1_auth_user_sid <> "" ->
+      true
+  | _ ->
+      (* Only local superuser and client-cert sessions have no sid, and a
+         session is exactly one of them. *)
+      let f = get_is_local_superuser ~__context in
+      f ~self:s1 = f ~self:s2
 
 let assert_op_valid ?(ok_if_no_session_in_context = false) ~__context task_id =
   let@ __context = Context.with_tracing ~__context __FUNCTION__ in

index 50d4767140aa..bf28e4965b6d 100644
@@ -100,8 +100,8 @@ let create_session_for_client_cert req s =
 
 let assert_credentials_ok realm ?(http_action = realm) ?(fn = Rbac.nofn)
     (req : Request.t) ic =
+  let task_id = ref_param_of_req req "task_id" in
   let rbac_raise permission msg exc =
-    let task_id = ref_param_of_req req "task_id" in
     ( match task_id with
     | None ->
         ()

@@ -115,6 +115,16 @@ let assert_credentials_ok realm ?(http_action = realm) ?(fn = Rbac.nofn)
     raise exc
   in
   let rbac_check session_id =
+    ( match task_id with
+    | None ->
+        ()
+    | Some task_id ->
+        if not (Server_helpers.can_own_task ~session_id task_id) then (
+          error "Refusing the request: task %s is not owned by the caller"
+            (Ref.string_of task_id) ;
+          raise Http.Forbidden
+        )
+    ) ;
     let rbac_task_desc = "handler" in
     let http_permission = Datamodel.rbac_http_permission_prefix ^ http_action in
     try

@@ -263,6 +273,8 @@ let with_context ?(dummy = false) label (req : Request.t) (s : Unix.file_descr)
             Xapi_session.consider_touching_session inet_rpc session_id () ;
           f __context
         in
+        (* The caller-supplied task is the one [f] runs under and reports into.
+           It must therefore belong to the caller. *)
         match task_id with
         | None ->
             Server_helpers.exec_with_new_task ?subtask_of ~session_id

@@ -270,9 +282,15 @@ let with_context ?(dummy = false) label (req : Request.t) (s : Unix.file_descr)
               ~origin:(Context.Http (req, s))
               label login_perform_logout
         | Some task_id ->
-            Server_helpers.exec_with_forwarded_task ~session_id
-              ~origin:(Context.Http (req, s))
-              task_id login_perform_logout
+            if Server_helpers.can_own_task ~session_id task_id then
+              Server_helpers.exec_with_forwarded_task ~session_id
+                ~origin:(Context.Http (req, s))
+                task_id login_perform_logout
+            else (
+              warn "Refusing the request: task %s is not owned by the caller"
+                (Ref.string_of task_id) ;
+              raise Http.Forbidden
+            )
       )
       (fun () ->
         if must_logout then

@@ -281,20 +299,9 @@ let with_context ?(dummy = false) label (req : Request.t) (s : Unix.file_descr)
             session_id
       )
   with Http.Unauthorised _ as e ->
-    let fail __context =
-      TaskHelper.failed ~__context
-        (Api_errors.Server_error (Api_errors.session_authentication_failed, []))
-    in
     debug
       "No authentication provided to http handler: returning 401 unauthorised" ;
-    (* Fail the task *)
-    ( match task_id with
-    | None ->
-        Server_helpers.exec_with_new_task ~task_in_database:(not dummy) label
-          fail
-    | Some task_id ->
-        Server_helpers.exec_with_forwarded_task task_id fail
-    ) ;
+    (* Do nothing here as authentication failed already. *)
     req.Request.close <- true ;
     raise e
 

index 25c2130caabe..f4c66b188748 100644
@@ -77,4 +77,5 @@ let () =
     @ Test_vm_protected_keys.tests
     @ Test_vm_protected_fields.tests
     @ Test_network_protected_keys.tests
+    @ Test_task_ownership.tests
     )

419 lines not on this page · raw message

index be816dd5bc71..c81be0b50e04 100644
@@ -24,6 +24,10 @@ let make_session ~__context ?(rbac_permissions = []) ~auth_user_sid () =
   Test_common.make_session ~__context ~is_local_superuser:false ~auth_user_sid
     ~rbac_permissions ()
 
+let make_client_cert_session ~__context =
+  Test_common.make_session ~__context ~is_local_superuser:false
+    ~auth_user_sid:"" ~client_certificate:true ()
+
 let make_task ~__context ~session_id label =
   let self = Ref.make () in
   Db.Task.create ~__context ~ref:self ~uuid:(Test_common.make_uuid ())

@@ -247,6 +251,14 @@ let test_not_own_task_of_other_user () =
   let session_id = make_session ~__context ~auth_user_sid:"S-1-attacker" () in
   check_can_own_task ~session_id ~expected:false ~task "task of another user"
 
+let test_client_cert_does_not_own_root_task () =
+  let __context = Test_common.make_test_database () in
+  let root = Test_common.make_session ~__context ~is_local_superuser:true () in
+  let task = make_task ~__context ~session_id:root "task of the root" in
+  let session_id = make_client_cert_session ~__context in
+  check_can_own_task ~session_id ~expected:false ~task
+    "client-cert session and root task"
+
 let test_not_own_task_without_session () =
   (* The Http.Unauthorised path of Xapi_http.with_context has no session at all. *)
   let __context = Test_common.make_test_database () in

@@ -343,6 +355,17 @@ let test_dispatch_allows_without_task_header () =
   check_dispatch_ran ~expected:true ~session_id ~supports_async:false
     "no task-id header"
 
+let test_dispatch_denies_client_cert_with_root_task () =
+  let __context = Test_common.make_test_database () in
+  let root = Test_common.make_session ~__context ~is_local_superuser:true () in
+  let task = make_task ~__context ~session_id:root "task of the root" in
+  let session_id = make_client_cert_session ~__context in
+  expect_rbac_denied
+    "task-id header from a client-cert session naming a root's task" (fun () ->
+      ignore (dispatch ~task ~session_id ())
+  ) ;
+  check_task_untouched ~__context ~task "task-id header, client-cert session"
+
 let tests =
   [
     ( "Xapi_http.assert_credentials_ok: RBAC-denied task"

@@ -394,6 +417,10 @@ let tests =
         , `Quick
         , test_own_task_of_destroy_any_holder
         )
+      ; ( "does not own a root task from a client-cert session"
+        , `Quick
+        , test_client_cert_does_not_own_root_task
+        )
       ]
     )
   ; ( "Server_helpers.do_dispatch: the task-id header"

@@ -414,6 +441,10 @@ let tests =
         , `Quick
         , test_dispatch_allows_without_task_header
         )
+      ; ( "refuses a root task named by a client-cert session"
+        , `Quick
+        , test_dispatch_denies_client_cert_with_root_task
+        )
       ]
     )
   ]

103 lines not on this page · raw message

index 6c655de59fa9..cdd060404b08 100644
@@ -140,11 +140,17 @@ let get_proxy_params ~__context =
   match (url, username, password_ref) with
   | url', username', password_ref'
     when url' <> "" && username' <> "" && password_ref' <> Ref.null ->
+      let password = Db.Secret.get_value ~__context ~self:password_ref' in
+      (* The secret value may change after being configured. Re-check it again. *)
+      if has_control_character password then (
+        error
+          "The password of the repository proxy contains control character(s)" ;
+        raise Api_errors.(Server_error (invalid_repository_proxy_credential, []))
+      ) ;
       [
         Printf.sprintf "proxy=%s" url'
       ; Printf.sprintf "proxy_username=%s" username'
-      ; Printf.sprintf "proxy_password=%s"
-          (Db.Secret.get_value ~__context ~self:password_ref')
+      ; Printf.sprintf "proxy_password=%s" password
       ]
   | url', "", _ when url' <> "" ->
       [Printf.sprintf "proxy=%s" url']

index 13d0eb94770f..5d6bc65558c8 100644
@@ -164,7 +164,21 @@ module DomainNameIncludeIP = struct
     )
 end
 
+let has_control_character s =
+  let is_control_character c =
+    let code = Char.code c in
+    code < 0x20 || code = 0x7f
+  in
+  String.exists is_control_character s
+
+let assert_no_control_chars ~url ~url_error =
+  if has_control_character url then (
+    error "Invalid url: it contains control character(s)" ;
+    raise Api_errors.(Server_error (url_error, [url]))
+  )
+
 let assert_url_is_valid ~url =
+  assert_no_control_chars ~url ~url_error:Api_errors.invalid_base_url ;
   try
     let uri = Uri.of_string url in
     match Uri.(scheme uri, host uri) with

@@ -265,6 +279,7 @@ let get_remote_pool_coordinator_ip url =
       raise Api_errors.(Server_error (invalid_base_url, [url]))
 
 let assert_remote_pool_url_is_valid ~url =
+  assert_no_control_chars ~url ~url_error:Api_errors.invalid_base_url ;
   ignore (get_remote_pool_coordinator_ip url : string)
 
 let with_pool_repositories f =

index 63f208b98c41..de1959246996 100644
@@ -4317,13 +4317,18 @@ let configure_repository_proxy ~__context ~self ~url ~username ~password =
       error "missing password of the repository proxy with a username specified" ;
       raise Api_errors.(Server_error (invalid_repository_proxy_credential, []))
   | u, p when u <> "" && p <> "" ->
-      if String.contains u '\n' || String.contains p '\n' then (
+      if
+        Repository_helpers.has_control_character u
+        || Repository_helpers.has_control_character p
+      then (
         error "getting invalid username/password of the repository proxy" ;
         raise Api_errors.(Server_error (invalid_repository_proxy_credential, []))
       )
   | _ ->
       ()
   ) ;
+  Repository_helpers.assert_no_control_chars ~url
+    ~url_error:Api_errors.invalid_repository_proxy_url ;
   let old_secret_ref = Db.Pool.get_repository_proxy_password ~__context ~self in
   let secret_ref =
     if password = "" then