diff --git a/backend/tests/wm_token_superadmin_guard.rs b/backend/tests/wm_token_superadmin_guard.rs index d30eb3be72..8823cb8707 100644 --- a/backend/tests/wm_token_superadmin_guard.rs +++ b/backend/tests/wm_token_superadmin_guard.rs @@ -759,6 +759,32 @@ async fn test_wm_token_cannot_destroy_its_on_behalf_account( resp.text().await? ); + // Ejecting the identity from a workspace is the same primitive, on both routes + // that expose it (one keyed by username, one by email). + for route in [ + "w/test-workspace/users/leave", + "w/test-workspace/workspaces/leave", + ] { + let resp = authed( + client().post(format!("http://localhost:{port}/api/{route}")), + &user_wm, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "WM_TOKEN must not leave a workspace as {route}: {}", + resp.text().await? + ); + } + let membership: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM usr WHERE workspace_id = 'test-workspace' AND email = 'test2@windmill.dev'", + ) + .fetch_one(&db) + .await?; + assert_eq!(membership, 1, "the workspace membership must survive"); + // The account and its credentials are untouched, not merely the response refused. let account_rows: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM password WHERE email = 'test2@windmill.dev'") @@ -773,3 +799,59 @@ async fn test_wm_token_cannot_destroy_its_on_behalf_account( Ok(()) } + +/// `load_workspace_authed` grants an admin claim in a workspace the caller may have +/// no relationship with, and carries `job_id` into the result — so deriving it from +/// the on-behalf email would hand a WM_TOKEN admin over every workspace on the +/// instance, and with it the cross-workspace diff (GHSA-hfh4-cx4h-3fcr). +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_wm_token_gets_no_admin_claim_in_a_foreign_workspace( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + set_jwt_secret().await; + + // A workspace the superadmin identity is not a member of. + sqlx::query( + "INSERT INTO workspace (id, name, owner) VALUES ('other-workspace', 'Other', 'test-user')", + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api"); + + let sa_wm = wm_token("test@windmill.dev", true).await; + let resp = authed( + client().get(format!( + "{base}/w/test-workspace/workspaces/compare/other-workspace" + )), + &sa_wm, + ) + .send() + .await?; + assert_ne!( + resp.status(), + 200, + "superadmin WM_TOKEN must not diff a workspace it does not belong to" + ); + + // No false positive: a real superadmin token still holds the claim. + let resp = authed( + client().get(format!( + "{base}/w/test-workspace/workspaces/compare/other-workspace" + )), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a real superadmin token must still diff across workspaces: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 1cdd9f496c..a30b62d032 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -3296,6 +3296,7 @@ async fn leave_workspace( Path(w_id): Path, authed: ApiAuthed, ) -> Result { + forbid_job_token_account_destruction(&authed)?; let mut tx = db.begin().await?; sqlx::query!( "DELETE FROM usr WHERE workspace_id = $1 AND username = $2", diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index a44989d9d1..475c8db285 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -7822,9 +7822,7 @@ async fn archive_workspace( .fetch_optional(&db) .await? .unwrap_or(false); - if !is_prod_admin - && !windmill_api_auth::is_super_admin_authed(&db, &authed).await? - { + if !is_prod_admin && !windmill_api_auth::is_super_admin_authed(&db, &authed).await? { return Err(Error::PermissionDenied(format!( "Archiving dev workspace '{w_id}' requires being an admin of its parent prod workspace '{prod}' (or a superadmin)" ))); @@ -7897,6 +7895,7 @@ async fn leave_workspace( Path(w_id): Path, authed: ApiAuthed, ) -> Result { + windmill_api_auth::forbid_job_token_account_destruction(&authed)?; let mut tx = db.begin().await?; sqlx::query!( "DELETE FROM usr WHERE workspace_id = $1 AND email = $2", @@ -10107,8 +10106,11 @@ async fn load_workspace_authed( .await .map_err(|e| Error::internal_err(e.to_string()))?; - let is_super_admin = - windmill_common::auth::is_super_admin_email(db, &base_authed.email).await?; + // Job-aware: this grants an admin claim in a workspace the caller may have no + // relationship with, and `job_id` is carried into the result — so a `WM_TOKEN` + // whose on-behalf identity is a superadmin would hold admin everywhere + // (GHSA-hfh4-cx4h-3fcr). It then falls through to its real membership below. + let is_super_admin = windmill_api_auth::is_super_admin_authed(db, base_authed).await?; let user_row = sqlx::query!( "SELECT username, is_admin, operator FROM usr diff --git a/docs/followup-onbehalf-execution-privilege-hardening.md b/docs/followup-onbehalf-execution-privilege-hardening.md index 77c336fbc7..a2efe1a72f 100644 --- a/docs/followup-onbehalf-execution-privilege-hardening.md +++ b/docs/followup-onbehalf-execution-privilege-hardening.md @@ -65,6 +65,69 @@ job's *preserved on-behalf email* rather than from a request `ApiAuthed`, so the `is_super_admin`/`is_job_token` bool from callers. Only independently authenticated superadmins should get the exemption. +### 3. Two credential mints still reachable by a job token + +Both hand back a database-backed token, which authenticates with `job_id: None` and so +sheds the cap. Neither is an *elevation* on its own, which is why they are here rather +than in #10124, but both let a job token trade itself for a credential it should not have. + +- `impersonate_service_account` (`windmill-api-users/src/users_ee.rs`) gates only on the + raw `require_admin(authed.is_admin, ..)` boolean, which a `WM_TOKEN` satisfies — it is + capped *at* workspace admin. It mints a 24h, **unscoped**, `workspace_id`-NULL token for + the service account and returns it in `Set-Cookie`. Unscoped means the route-scope + middleware does not confine it, so it holds whatever the service account's `usr` row + grants, including a workspace-admin claim with no job provenance. +- `new_webhook_token` (`windmill-native-triggers/src/handler.rs`) reaches + `create_token_internal` from trigger create/update and from the rename path with no job + guard. `create_token_internal` copies `super_admin` from the caller's `password` row, and + `webhook_token_expiration()` is `None` for GitHub and Nextcloud, so the result is a + permanent superadmin-flagged token — embedded in the webhook URL handed to the external + service, and readable by whoever controls that repo. Its sibling `rotate_webhook_token` + (`windmill-native-triggers/src/lib.rs`) copies `email`/`super_admin`/`owner` verbatim from + the old row, so guarding only the fresh-mint path leaves the update route open. + The `jobs:run::` scopes do bound this one, and scopes are enforced globally at + auth extraction — unlike the impersonation token above. + +Fix both with `forbid_elevated_job_token`, as the other mints do. + +### 4. Destructive self-service against the borrowed identity + +A different threat model from the GHSA: not escalation, but a `wm_deployers` member +damaging an arbitrary victim through a runnable that preserves their identity. #10124 +closed `leave_instance`, `tokens/delete`, `users/leave`, and `workspaces/leave`; these +remain, ranked by damage: + +- `update_draft` (`windmill-api/src/drafts.rs`) deliberately **skips** + `require_can_write_path` when the request is a self-discard, and honours `force`, so a + job token can destroy the victim's unsaved script/flow/app work at any path. Draft paths + are enumerable first through `drafts/list` with the same token. +- `decline_invite` (`windmill-api-users/src/users.rs`) takes `workspace_id` from the request + **body**, not the path, escaping the token's workspace binding: it can delete the victim's + pending invite to any workspace on the instance, including admin invites. +- `accept_invite` consumes an invite (irreversible, same body-supplied workspace) and, when + `AUTOMATE_USERNAME_CREATION` is off, lets the caller choose the victim's username in that + workspace. +- `delete_input` / `update_input` (`windmill-api-inputs/src/lib.rs`) delete or rewrite the + victim's saved inputs; flipping `is_public` to true leaks their saved arguments. +- `unstar` (`windmill-api/src/favorite.rs`) — cosmetic. + +`forbid_job_token_account_destruction` is the right guard for the first three. Decide +deliberately where the line sits for the tail: a job legitimately acts *as* the identity, +so not every self-service write should be refused. + +Note `leave_workspace` (both routes) did a *bare* `DELETE FROM usr`, unlike +`delete_workspace_user_internal`, which also strips `extra_perms`, folder owners, drafts, +favorites, inputs and captures. Even now that a job token cannot reach it, a user leaving a +workspace still leaves dangling `u/` ACLs behind — worth fixing separately. + +### 5. Missing workspace binding on `get_github_app_token` + +`git_sync_ee.rs` gates on `require_admin(authed.is_admin, ..)` but never checks that +`authed`'s workspace matches the `workspace_id` claim inside the supplied job JWT — the JWT +alone drives the installation lookup. Exploiting it requires already holding a git-sync job +JWT from the other workspace, so it is not an escalation path on its own, but the binding +should be explicit. + ## Guiding principle `on_behalf_of` is attacker-influenced (a `wm_deployers` member sets it). It must never