From: Ming Lu Subject: CA-430077: Low-privilege user can force-fail arbitrary tasks via task_id parameter A low-privilege user could use [task_id] parameter in some APIs to set other users' tasks failed. So now setting a task failed only if the task is owned by the user (caller). For a call or request, the check can be placed in the following 3 places alternatively: 1. Xapi_http.assert_credentials_ok if rbac_check is true in HTTP actions; 2. Server_helpers.do_dispatch if the call is a regular XenAPI call; 3. others by calling Xapi_http.with_context. This is a best-to-have place in case a call or request doesn't go to the 2 places above. This is CVE-2026-119779, part of XSA-530. Signed-off-by: Ming Lu Reviewed-by: Andrii Sultanov diff --git a/ocaml/xapi/server_helpers.ml b/ocaml/xapi/server_helpers.ml index 326e47ee463b..eedf1b163427 100644 --- a/ocaml/xapi/server_helpers.ml +++ b/ocaml/xapi/server_helpers.ml @@ -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 = diff --git a/ocaml/xapi/server_helpers.mli b/ocaml/xapi/server_helpers.mli index 9155469e26ba..6944db32ea86 100644 --- a/ocaml/xapi/server_helpers.mli +++ b/ocaml/xapi/server_helpers.mli @@ -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 diff --git a/ocaml/xapi/taskHelper.ml b/ocaml/xapi/taskHelper.ml index 1990ac2ce7ad..56a42d9a88c9 100644 --- a/ocaml/xapi/taskHelper.ml +++ b/ocaml/xapi/taskHelper.ml @@ -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 diff --git a/ocaml/xapi/xapi_http.ml b/ocaml/xapi/xapi_http.ml index 50d4767140aa..bf28e4965b6d 100644 --- a/ocaml/xapi/xapi_http.ml +++ b/ocaml/xapi/xapi_http.ml @@ -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