Sign the OIDC login flow-state cookie (login CSRF / session fixation)#1312
Sign the OIDC login flow-state cookie (login CSRF / session fixation)#1312lovasoa wants to merge 1 commit into
Conversation
…on fixation The temporary OIDC login-state cookie (sqlpage_oidc_state_<state>) was stored as unsigned JSON and trusted on the callback purely by its name matching the callback's state parameter. An attacker able to plant a cookie for the SQLPage origin could pre-seed flow state the server never issued and, together with an authorization code for an identity they control, log a victim's browser into that attacker-controlled identity. Sign the cookie value with HMAC-SHA256 keyed by the OIDC client secret (same key material as compute_logout_signature) and verify it on the callback, rejecting any login-state cookie this server did not produce.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15d16a0357
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let signature = compute_login_flow_state_signature(&payload, client_secret); | ||
| let cookie_value = format!("{payload}.{signature}"); |
There was a problem hiding this comment.
Signing the payload does not prevent the documented login-CSRF scenario because an attacker can initiate a legitimate login flow in their own browser, copy the resulting server-signed flow-state cookie, complete authorization for their identity, and then plant that exact cookie in the victim's browser before directing it to the callback. Since the HMAC only proves that the server issued the payload and is not bound to the initiating browser or stored as one-time server-side state, verification succeeds and the victim is still logged into the attacker's identity.
Useful? React with 👍 / 👎.
|
Closing as out of scope under SQLPage's threat model. This login-CSRF/session-fixation requires the attacker to plant a cookie on the SQLPage origin, which is the precondition the original report stated. The HMAC-signing approach in this PR does not actually close it: an attacker can start their own legitimate flow, copy the validly server-signed state cookie, and plant it. A server-side one-time state store does not close it either, since the attacker's state is validly issued and the victim consumes it once (that only prevents replay). The effective mitigation would be cookie-injection hardening (e.g. a |
1 participant
Sign the OIDC login flow-state cookie (login CSRF / session fixation)
The built-in OIDC login flow stored its temporary state in an unsigned JSON cookie named after the
stateparameter (sqlpage_oidc_state_<state>), and the callback trusted whatever cookie matched the callback'sstate(oidc.rs:1107 stores unsigned JSON, oidc.rs:703 selects it bystate, oidc.rs:716 setssqlpage_auth).An attacker who can plant a cookie for the SQLPage origin (or a parent domain) primes a
state+ matchingsqlpage_oidc_state_<state>cookie plus an authorization code for an identity they control. A victim hitting/sqlpage/oidc_callback?code=<attacker_code>&state=<attacker_state>then getsset-cookie: sqlpage_auth=<JWT for the attacker identity>: login CSRF / session fixation, logging the victim into the attacker's account. (Not token forgery; precondition is cookie-planting.)Fix
HMAC-SHA256-sign the cookie value with the OIDC client secret (the same key material as the existing
compute_logout_signature) and verify it on the callback. A login-state cookie the server never produced is rejected.Proof
New integration test
test_oidc_forged_state_cookie_is_rejecteddrives the callback with an attacker-planted (unsigned) state cookie and asserts nosqlpage_authis set.Before the fix it fails: the forged cookie produces a login.
After the fix, the full OIDC suite passes, including the existing happy-path login:
cargo clippy --all-targets --all-features -- -D warningsis clean. A focused unit test (login_flow_state_cookie_round_trips_and_rejects_forgery) also covers sign/verify, forged-signature rejection, and wrong-secret rejection.In-flight logins started before the upgrade simply restart the OIDC flow; no config change needed.