From 3af7bb3c9c35b051d23bf32be848a8e84dea1ebc Mon Sep 17 00:00:00 2001 From: 81reap Date: Tue, 1 Sep 2026 21:58:06 -0400 Subject: [PATCH] fix(oidc) :: compare logout in constant time Before verification compared the HMACs with an slice comparison. `Mac::verify_slice` removes the base64-encoding and decoding. --- CHANGELOG.md | 1 + src/webserver/oidc.rs | 80 ++++++++++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80a70444..5b3954ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ SQLPage now keeps the variable value, producing `https://api.example.com/john.doe` as expected. - Errors from a failed migration now point at the file and the SQL that failed. Before a reversible migration could be reported with the contents of its `.down.sql` half, and any migration with a multi-word name was reported as `0001_add new users.sql` rather than `0001_add_new_users.sql`. - A request that resolves to a directory now returns a 404 page. Directory names that contain a dot are routed to the static file handler, which used to fail with a server error instead. +- The signature on an OIDC logout URL is now compared in constant time. - A `content_security_policy` that does not contain `'nonce-{NONCE}'` is now sent as written, instead of being silently dropped and leaving the response with no `Content-Security-Policy` header at all. Setting the option to the empty string still disables the header, as documented. ## v0.46 diff --git a/src/webserver/oidc.rs b/src/webserver/oidc.rs index 19f2ccf1..64c4c747 100644 --- a/src/webserver/oidc.rs +++ b/src/webserver/oidc.rs @@ -182,13 +182,18 @@ impl OidcConfig { /// `None`/empty if the caller is not authenticated. #[must_use] pub fn create_logout_url(&self, redirect_uri: &str, session_token: Option<&str>) -> String { + use base64::Engine as _; + use hmac::Mac as _; + let timestamp = chrono::Utc::now().timestamp(); - let signature = compute_logout_signature( + let mac = logout_mac( redirect_uri, timestamp, session_token.unwrap_or_default(), &self.client_secret, ); + let signature = + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(mac.finalize().into_bytes()); let query = form_urlencoded::Serializer::new(String::new()) .append_pair("redirect_uri", redirect_uri) .append_pair("timestamp", ×tamp.to_string()) @@ -673,18 +678,18 @@ fn process_oidc_logout( Ok(response) } -fn compute_logout_signature( +type LogoutMac = hmac::Hmac; + +fn logout_mac( redirect_uri: &str, timestamp: i64, session_token: &str, client_secret: &str, -) -> String { - use base64::Engine; - use hmac::{Hmac, KeyInit, Mac}; - use sha2::Sha256; +) -> LogoutMac { + use hmac::{KeyInit, Mac}; - let mut mac = Hmac::::new_from_slice(client_secret.as_bytes()) - .expect("HMAC accepts any key size"); + let mut mac = + LogoutMac::new_from_slice(client_secret.as_bytes()).expect("HMAC accepts any key size"); mac.update(redirect_uri.as_bytes()); mac.update(×tamp.to_be_bytes()); // Bind the signature to the session so a logout URL can only log out the @@ -692,8 +697,7 @@ fn compute_logout_signature( // the redirect_uri and session_token fields. mac.update(&(session_token.len() as u64).to_be_bytes()); mac.update(session_token.as_bytes()); - let signature = mac.finalize().into_bytes(); - base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(signature) + mac } fn verify_logout_params( @@ -702,25 +706,20 @@ fn verify_logout_params( client_secret: &str, ) -> anyhow::Result<()> { use base64::Engine; - - let expected_signature = compute_logout_signature( - ¶ms.redirect_uri, - params.timestamp, - session_token, - client_secret, - ); + use hmac::Mac; let provided_signature = base64::engine::general_purpose::URL_SAFE_NO_PAD .decode(¶ms.signature) .with_context(|| "Invalid logout signature encoding")?; - let expected_signature_bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD - .decode(&expected_signature) - .with_context(|| "Failed to decode expected signature")?; - - if expected_signature_bytes[..] != provided_signature[..] { - anyhow::bail!("Invalid logout signature"); - } + logout_mac( + ¶ms.redirect_uri, + params.timestamp, + session_token, + client_secret, + ) + .verify_slice(&provided_signature) + .map_err(|_| anyhow::anyhow!("Invalid logout signature"))?; let now = chrono::Utc::now().timestamp(); if now - params.timestamp > LOGOUT_TOKEN_VALIDITY_SECONDS { @@ -1391,6 +1390,39 @@ mod tests { .expect("generated URL should validate for the session it was issued for"); } + fn logout_params_stamped(timestamp: i64) -> LogoutParams { + use base64::Engine as _; + use hmac::Mac as _; + + let mac = logout_mac("/after", timestamp, "session-token", "secret"); + LogoutParams { + redirect_uri: "/after".to_string(), + timestamp, + signature: base64::engine::general_purpose::URL_SAFE_NO_PAD + .encode(mac.finalize().into_bytes()), + } + } + + #[test] + fn a_correctly_signed_logout_token_is_still_rejected_outside_its_time_window() { + let now = chrono::Utc::now().timestamp(); + verify_logout_params(&logout_params_stamped(now), "session-token", "secret") + .expect("a freshly issued logout token is accepted"); + + for (label, timestamp) in [ + ("expired", now - LOGOUT_TOKEN_VALIDITY_SECONDS * 2), + ("in the future", now + 3600), + ] { + let err = + verify_logout_params(&logout_params_stamped(timestamp), "session-token", "secret") + .expect_err(&format!("a logout token {label} must be rejected")); + assert!( + !err.to_string().contains("signature"), + "{label} token was rejected for the wrong reason: {err}" + ); + } + } + /// A logout URL is bound to the session it was issued for: presenting it /// from a different browser (a different `sqlpage_auth` cookie), or with /// no session at all, must NOT validate. This is what prevents a forced