From a9d075bc191fd2c66106234c27191400c772097a Mon Sep 17 00:00:00 2001 From: hzhang Date: Sun, 17 May 2026 22:07:43 +0100 Subject: [PATCH] fix(security): block OIDC-binding privilege escalation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The oidc-binding PUT/DELETE endpoints allowed any account.create holder (non-admin role 'account-manager') to bind an attacker-controlled OIDC identity to the admin account (or unbind admin, reopening the OIDC-only bootstrap window) — full admin takeover. Non-admin callers may now only manage bindings of non-privileged accounts: requests targeting an is_admin user, the built-in acc-mgr/deleted-user, or any holder of account.create / user.reset-apikey are rejected with 403. Global admins remain unrestricted, so the intended "account-manager binds normal users" capability is preserved. Found by post-feature security audit. Verified locally. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/api/routers/users.py | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/app/api/routers/users.py b/app/api/routers/users.py index b0a84db..fee741c 100644 --- a/app/api/routers/users.py +++ b/app/api/routers/users.py @@ -438,17 +438,38 @@ class OidcBindingResponse(BaseModel): oidc_subject: str | None = None +def _assert_can_manage_oidc_binding(db: Session, caller: models.User, target: models.User) -> None: + """Global admins may (un)bind anyone. Non-admin account managers may + only operate on non-privileged accounts — never on an admin or another + privileged account — otherwise binding an attacker-controlled OIDC + identity to an admin would be a privilege-escalation primitive.""" + if getattr(caller, "is_admin", False): + return + privileged = ( + getattr(target, "is_admin", False) + or target.username in ("acc-mgr", "deleted-user") + or _has_global_permission(db, target, "account.create") + or _has_global_permission(db, target, "user.reset-apikey") + ) + if privileged: + raise HTTPException( + status_code=403, + detail="Only a global admin may manage the OIDC binding of a privileged account", + ) + + @router.put("/{identifier}/oidc-binding", response_model=OidcBindingResponse) def bind_user_oidc( identifier: str, payload: OidcBindingRequest, db: Session = Depends(get_db), - _: models.User = Depends(require_account_creator), + caller: models.User = Depends(require_account_creator), ): """Bind an hf user to an external OIDC identity (issuer + subject). - Admin or account-manager only (JWT or API key). One OIDC identity maps - to at most one user.""" + Admin or account-manager (JWT or API key). Account managers may not + target privileged/admin accounts. One OIDC identity maps to at most + one user.""" issuer = (payload.issuer or "").strip() subject = (payload.subject or "").strip() if not issuer or not subject: @@ -456,6 +477,7 @@ def bind_user_oidc( user = _find_user_by_id_or_username(db, identifier) if not user: raise HTTPException(status_code=404, detail="User not found") + _assert_can_manage_oidc_binding(db, caller, user) clash = db.query(models.User).filter( models.User.oidc_issuer == issuer, models.User.oidc_subject == subject, @@ -475,12 +497,14 @@ def bind_user_oidc( def unbind_user_oidc( identifier: str, db: Session = Depends(get_db), - _: models.User = Depends(require_account_creator), + caller: models.User = Depends(require_account_creator), ): - """Remove a user's OIDC binding. Admin or account-manager only.""" + """Remove a user's OIDC binding. Admin or account-manager; account + managers may not target privileged/admin accounts.""" user = _find_user_by_id_or_username(db, identifier) if not user: raise HTTPException(status_code=404, detail="User not found") + _assert_can_manage_oidc_binding(db, caller, user) user.oidc_issuer = None user.oidc_subject = None db.commit()