fix(security): block OIDC-binding privilege escalation
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) <noreply@anthropic.com>
This commit is contained in:
@@ -438,17 +438,38 @@ class OidcBindingResponse(BaseModel):
|
|||||||
oidc_subject: str | None = None
|
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)
|
@router.put("/{identifier}/oidc-binding", response_model=OidcBindingResponse)
|
||||||
def bind_user_oidc(
|
def bind_user_oidc(
|
||||||
identifier: str,
|
identifier: str,
|
||||||
payload: OidcBindingRequest,
|
payload: OidcBindingRequest,
|
||||||
db: Session = Depends(get_db),
|
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).
|
"""Bind an hf user to an external OIDC identity (issuer + subject).
|
||||||
|
|
||||||
Admin or account-manager only (JWT or API key). One OIDC identity maps
|
Admin or account-manager (JWT or API key). Account managers may not
|
||||||
to at most one user."""
|
target privileged/admin accounts. One OIDC identity maps to at most
|
||||||
|
one user."""
|
||||||
issuer = (payload.issuer or "").strip()
|
issuer = (payload.issuer or "").strip()
|
||||||
subject = (payload.subject or "").strip()
|
subject = (payload.subject or "").strip()
|
||||||
if not issuer or not subject:
|
if not issuer or not subject:
|
||||||
@@ -456,6 +477,7 @@ def bind_user_oidc(
|
|||||||
user = _find_user_by_id_or_username(db, identifier)
|
user = _find_user_by_id_or_username(db, identifier)
|
||||||
if not user:
|
if not user:
|
||||||
raise HTTPException(status_code=404, detail="User not found")
|
raise HTTPException(status_code=404, detail="User not found")
|
||||||
|
_assert_can_manage_oidc_binding(db, caller, user)
|
||||||
clash = db.query(models.User).filter(
|
clash = db.query(models.User).filter(
|
||||||
models.User.oidc_issuer == issuer,
|
models.User.oidc_issuer == issuer,
|
||||||
models.User.oidc_subject == subject,
|
models.User.oidc_subject == subject,
|
||||||
@@ -475,12 +497,14 @@ def bind_user_oidc(
|
|||||||
def unbind_user_oidc(
|
def unbind_user_oidc(
|
||||||
identifier: str,
|
identifier: str,
|
||||||
db: Session = Depends(get_db),
|
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)
|
user = _find_user_by_id_or_username(db, identifier)
|
||||||
if not user:
|
if not user:
|
||||||
raise HTTPException(status_code=404, detail="User not found")
|
raise HTTPException(status_code=404, detail="User not found")
|
||||||
|
_assert_can_manage_oidc_binding(db, caller, user)
|
||||||
user.oidc_issuer = None
|
user.oidc_issuer = None
|
||||||
user.oidc_subject = None
|
user.oidc_subject = None
|
||||||
db.commit()
|
db.commit()
|
||||||
|
|||||||
Reference in New Issue
Block a user