fix: base workspace-app CORS on resolved owner instead of URL username#26086
fix: base workspace-app CORS on resolved owner instead of URL username#26086geokat wants to merge 1 commit into
Conversation
d3bd31b to
75e1b53
Compare
Docs preview📖 View docs preview for |
75e1b53 to
302a2ba
Compare
07f7dcc to
510ba49
Compare
4a77e06 to
b45e9e5
Compare
The workspace-app CORS check trusted the username parsed from the origin's subdomain hostname, comparing it against the target app's username. Because that value is attacker-controllable, a same-owner origin could be spoofed and credentialed cross-origin reads allowed. Compare the target app's owner, taken from the issued signed token, against the origin app's owner, resolved from the database as the owner of the origin's workspace. Allow the request only when the two owner IDs match. wsproxy resolves the origin owner through a new coderd endpoint since it has no database access. Fail closed when the owner cannot be resolved. Rename the wsproxy TokenProvider to AppsProvider to reflect its expanded responsibility. Resolving the origin owner from its workspace rather than the URL username keeps this check sound even if the resolution-time guard that rejects owner/username mismatches is ever removed, giving a second line of defense against the spoof. Refs: https://linear.app/codercom/issue/PLAT-260
510ba49 to
c773cce
Compare
b45e9e5 to
725749e
Compare
|
|
||
| //nolint:gocritic // The caller may not have permission to read the | ||
| // workspace, but CORS needs authoritative app identity. | ||
| dbReq, err := req.getDatabase(dbauthz.AsSystemRestricted(ctx), p.Database) |
There was a problem hiding this comment.
Heads up, not blocking: this resolves the origin owner from the DB on every CORS request in simple mode (AllowOriginFunc calls ResolveAppOwnerID for each cross-origin hit, not just preflights), and on the wsproxy path it's an HTTP round-trip to coderd per request via /resolve-app-owner. Previously this was an in-memory string compare. For app-heavy workspaces behind a proxy that's a real per-request cost.
Worth caching the normalized origin ApplicationURL -> owner ID with a short TTL, both here and in enterprise/wsproxy/AppsProvider.ResolveAppOwnerID. TTL should stay short since this gates a credentialed (AllowCredentials) cross-origin read and ownership can change (transfer/delete).
Planning to file a follow-up issue for the cache, unless you think it's in-scope?
There was a problem hiding this comment.
I think we probably do need to cache this result, we don't want to be issuing round-trips from proxies to the primary on every request.
|
Note on the rename churn (happy to revert): this PR renames the These are cosmetic — they reflect the provider now also resolving owners, not just issuing tokens. |
deansheather
left a comment
There was a problem hiding this comment.
Sorry it took so long for the review.
Do we need to implement this defense-in-depth measure? While it's true that attackers could change their username, they can't change their username to the same as another user. I'd like to see a PoC validated before making a significant change like this to the wsproxy-facing API and adding extra complexity/latency to wsproxy requests.
|
|
||
| //nolint:gocritic // The caller may not have permission to read the | ||
| // workspace, but CORS needs authoritative app identity. | ||
| dbReq, err := req.getDatabase(dbauthz.AsSystemRestricted(ctx), p.Database) |
There was a problem hiding this comment.
I think we probably do need to cache this result, we don't want to be issuing round-trips from proxies to the primary on every request.
|
@deansheather
The only way I can produce a PoC is to disable the owner/username check from #26085, and that regression is already guarded by the WorkspaceUUIDOwnerMismatch tests added in that PR. You're right, this PR (plus an owner-lookup cache) is too much complexity to defend against a regression that tests already catch. I'll close this for now unless there are objections. |
They are now the sole line of defense since we decided not to merge #26086 (defense-in-depth)
Stacked on #26085 (the minimum fix).
The workspace-app CORS check compared the username parsed from the origin's subdomain hostname against the target app's username. That value is attacker-controllable, so a same-owner origin could be spoofed and credentialed cross-origin reads allowed.
Compare resolved owner IDs instead: the target app's owner comes from the issued signed token, and the origin app's owner is resolved from the database as the owner of the origin's workspace. Permit the credentialed cross-origin read (emit the CORS allow-origin header) only when the two match. wsproxy has no database access, so it resolves the origin owner through a new coderd endpoint. The check fails closed when the owner cannot be resolved.
Because the origin owner is read from the resolved workspace rather than the URL username, this check denies the spoofed cross-origin read on its own, even if #26085 were absent or its resolution-time guard regressed. The check runs on every CORS request, but while #26085 is in place no origin an attacker can serve will exercise that independence: the only distinguishing input is the spoofed-username origin #26085 prevents from resolving. So the standalone behavior isn't separately tested.
Refs: https://linear.app/codercom/issue/PLAT-260
This PR is part of a stack that merges into
main:Created with stakk