From 6177f5e5551c2e06a551852b3d17364fbd1197a6 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 16 Mar 2026 12:36:21 +0530 Subject: [PATCH 1/2] Use constant-time comparison for token and credential verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace standard == comparisons with subtle::ConstantTimeEq in the three places that verify secrets: tstoken signature in proxy.rs, clear-URL signature in http_util.rs, and Basic Auth credentials in auth.rs. The auth fix also removes the && short-circuit that created a username- existence oracle — both username and password are now always evaluated using bitwise & on subtle::Choice values. Adds log::warn on auth failure (path only, no credentials) and five targeted tests covering tampered tokens, wrong-username-right-password, and empty tokens. Closes #410 --- Cargo.lock | 1 + Cargo.toml | 1 + crates/common/Cargo.toml | 1 + crates/common/src/auth.rs | 39 +++++++++++++++++++++++++++++++++- crates/common/src/http_util.rs | 31 ++++++++++++++++++++++++++- crates/common/src/proxy.rs | 27 ++++++++++++++++++++++- 6 files changed, 97 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 459fbb9b..b9310e78 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2369,6 +2369,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", + "subtle", "temp-env", "tokio", "tokio-test", diff --git a/Cargo.toml b/Cargo.toml index 3ce93495..fbf036dd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ tokio-test = "0.4" toml = "0.9.8" url = "2.5.8" urlencoding = "2.1" +subtle = "2" uuid = { version = "1.18", features = ["v4"] } validator = { version = "0.20", features = ["derive"] } which = "8" diff --git a/crates/common/Cargo.toml b/crates/common/Cargo.toml index c360474e..cb7dedc1 100644 --- a/crates/common/Cargo.toml +++ b/crates/common/Cargo.toml @@ -40,6 +40,7 @@ regex = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } sha2 = { workspace = true } +subtle = { workspace = true } tokio = { workspace = true } toml = { workspace = true } trusted-server-js = { path = "../js" } diff --git a/crates/common/src/auth.rs b/crates/common/src/auth.rs index ba669e6a..ac332e67 100644 --- a/crates/common/src/auth.rs +++ b/crates/common/src/auth.rs @@ -1,6 +1,7 @@ use base64::{engine::general_purpose::STANDARD, Engine as _}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; +use subtle::ConstantTimeEq; use crate::settings::Settings; @@ -14,9 +15,14 @@ pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option return Some(unauthorized_response()), }; - if username == handler.username && password == handler.password { + // Use bitwise & (not &&) so both sides always evaluate — eliminates the + // username-existence oracle that short-circuit evaluation would create. + let username_ok = username.as_bytes().ct_eq(handler.username.as_bytes()); + let password_ok = password.as_bytes().ct_eq(handler.password.as_bytes()); + if (username_ok & password_ok).into() { None } else { + log::warn!("Basic auth failed for path: {}", req.get_path()); Some(unauthorized_response()) } } @@ -109,6 +115,37 @@ mod tests { assert_eq!(response.get_status(), StatusCode::UNAUTHORIZED); } + #[test] + fn challenge_when_username_wrong_password_correct() { + // Validates that both fields are always evaluated — no short-circuit username oracle. + let settings = settings_with_handlers(); + let mut req = Request::new(Method::GET, "https://example.com/secure/data"); + let token = STANDARD.encode("wrong-user:pass"); + req.set_header(header::AUTHORIZATION, format!("Basic {token}")); + + let response = enforce_basic_auth(&settings, &req).expect("should challenge"); + assert_eq!( + response.get_status(), + StatusCode::UNAUTHORIZED, + "should reject wrong username even with correct password" + ); + } + + #[test] + fn challenge_when_username_correct_password_wrong() { + let settings = settings_with_handlers(); + let mut req = Request::new(Method::GET, "https://example.com/secure/data"); + let token = STANDARD.encode("user:wrong-pass"); + req.set_header(header::AUTHORIZATION, format!("Basic {token}")); + + let response = enforce_basic_auth(&settings, &req).expect("should challenge"); + assert_eq!( + response.get_status(), + StatusCode::UNAUTHORIZED, + "should reject correct username with wrong password" + ); + } + #[test] fn challenge_when_scheme_is_not_basic() { let settings = settings_with_handlers(); diff --git a/crates/common/src/http_util.rs b/crates/common/src/http_util.rs index 931c894f..a4c88a92 100644 --- a/crates/common/src/http_util.rs +++ b/crates/common/src/http_util.rs @@ -3,6 +3,7 @@ use chacha20poly1305::{aead::Aead, aead::KeyInit, XChaCha20Poly1305, XNonce}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; use sha2::{Digest, Sha256}; +use subtle::ConstantTimeEq; use crate::constants::INTERNAL_HEADERS; use crate::settings::Settings; @@ -287,7 +288,8 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { /// Verify a `tstoken` for the given clear-text URL. #[must_use] pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { - sign_clear_url(settings, clear_url) == token + let expected = sign_clear_url(settings, clear_url); + expected.as_bytes().ct_eq(token.as_bytes()).into() } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). @@ -354,6 +356,33 @@ mod tests { )); } + #[test] + fn verify_clear_url_rejects_tampered_token() { + let settings = crate::test_support::tests::create_test_settings(); + let url = "https://cdn.example/a.png?x=1"; + let valid_token = sign_clear_url(&settings, url); + + // Flip one bit in the first byte — same URL, same length, wrong bytes + let mut tampered = valid_token.into_bytes(); + tampered[0] ^= 0x01; + let tampered = + String::from_utf8(tampered).expect("should be valid utf8 after single-bit flip"); + + assert!( + !verify_clear_url_signature(&settings, url, &tampered), + "should reject token with tampered bytes" + ); + } + + #[test] + fn verify_clear_url_rejects_empty_token() { + let settings = crate::test_support::tests::create_test_settings(); + assert!( + !verify_clear_url_signature(&settings, "https://cdn.example/a.png", ""), + "should reject empty token" + ); + } + // RequestInfo tests #[test] diff --git a/crates/common/src/proxy.rs b/crates/common/src/proxy.rs index 99db6328..d46f161d 100644 --- a/crates/common/src/proxy.rs +++ b/crates/common/src/proxy.rs @@ -4,6 +4,7 @@ use fastly::http::{header, HeaderValue, Method, StatusCode}; use fastly::{Request, Response}; use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use subtle::ConstantTimeEq; use crate::constants::{ HEADER_ACCEPT, HEADER_ACCEPT_ENCODING, HEADER_ACCEPT_LANGUAGE, HEADER_REFERER, @@ -1052,7 +1053,8 @@ fn reconstruct_and_validate_signed_target( }; let expected = compute_encrypted_sha256_token(settings, &full_for_token); - if expected != sig { + let valid: bool = expected.as_bytes().ct_eq(sig.as_bytes()).into(); + if !valid { return Err(Report::new(TrustedServerError::Proxy { message: "invalid tstoken".to_string(), })); @@ -1238,6 +1240,29 @@ mod tests { assert_eq!(err.current_context().status_code(), StatusCode::BAD_GATEWAY); } + #[tokio::test] + async fn reconstruct_rejects_tampered_tstoken() { + let settings = create_test_settings(); + let tsurl = "https://cdn.example/asset.js"; + let tsurl_encoded = + url::form_urlencoded::byte_serialize(tsurl.as_bytes()).collect::(); + // Syntactically valid base64url token of the right length, but not the correct signature + let bad_token = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let url = format!( + "https://edge.example/first-party/proxy?tsurl={}&tstoken={}", + tsurl_encoded, bad_token + ); + + let err: Report = + reconstruct_and_validate_signed_target(&settings, &url) + .expect_err("should reject tampered token"); + assert_eq!( + err.current_context().status_code(), + StatusCode::BAD_GATEWAY, + "should return 502 for invalid tstoken" + ); + } + #[tokio::test] async fn click_missing_params_returns_400() { let settings = create_test_settings(); From 0d5376ef69964bb2503b13830d9e2bbb1e677d8e Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 17 Mar 2026 13:30:41 +0530 Subject: [PATCH 2/2] Address PR review feedback on constant-time comparison hardening --- Cargo.toml | 2 +- crates/common/src/auth.rs | 9 ++++++++- crates/common/src/http_util.rs | 8 ++++++-- crates/common/src/proxy.rs | 8 ++++++-- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fbf036dd..6a7c8570 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,13 +70,13 @@ regex = "1.12.3" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.149" sha2 = "0.10.9" +subtle = "2" temp-env = "0.3.6" tokio = { version = "1.49", features = ["sync", "macros", "io-util", "rt", "time"] } tokio-test = "0.4" toml = "0.9.8" url = "2.5.8" urlencoding = "2.1" -subtle = "2" uuid = { version = "1.18", features = ["v4"] } validator = { version = "0.20", features = ["derive"] } which = "8" diff --git a/crates/common/src/auth.rs b/crates/common/src/auth.rs index ac332e67..18876706 100644 --- a/crates/common/src/auth.rs +++ b/crates/common/src/auth.rs @@ -1,12 +1,19 @@ use base64::{engine::general_purpose::STANDARD, Engine as _}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; +/// Enforce HTTP Basic Authentication for paths that require credentials. +/// +/// Returns `None` if the request is authorized (or the path has no handler), +/// `Some(401 response)` otherwise. Uses constant-time comparison for both +/// username and password, and evaluates both regardless of individual match +/// results to prevent timing oracles. +#[must_use] pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option { let handler = settings.handler_for_path(req.get_path())?; diff --git a/crates/common/src/http_util.rs b/crates/common/src/http_util.rs index a4c88a92..3f81aef3 100644 --- a/crates/common/src/http_util.rs +++ b/crates/common/src/http_util.rs @@ -3,7 +3,7 @@ use chacha20poly1305::{aead::Aead, aead::KeyInit, XChaCha20Poly1305, XNonce}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; use sha2::{Digest, Sha256}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::constants::INTERNAL_HEADERS; use crate::settings::Settings; @@ -286,10 +286,14 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { } /// Verify a `tstoken` for the given clear-text URL. +/// +/// Uses constant-time comparison to prevent timing side-channel attacks. +/// Length is not secret (always 43 bytes for base64url-encoded SHA-256), +/// but we check explicitly to document the invariant. #[must_use] pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { let expected = sign_clear_url(settings, clear_url); - expected.as_bytes().ct_eq(token.as_bytes()).into() + expected.len() == token.len() && bool::from(expected.as_bytes().ct_eq(token.as_bytes())) } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). diff --git a/crates/common/src/proxy.rs b/crates/common/src/proxy.rs index d46f161d..b6500b02 100644 --- a/crates/common/src/proxy.rs +++ b/crates/common/src/proxy.rs @@ -4,7 +4,7 @@ use fastly::http::{header, HeaderValue, Method, StatusCode}; use fastly::{Request, Response}; use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::constants::{ HEADER_ACCEPT, HEADER_ACCEPT_ENCODING, HEADER_ACCEPT_LANGUAGE, HEADER_REFERER, @@ -1053,7 +1053,11 @@ fn reconstruct_and_validate_signed_target( }; let expected = compute_encrypted_sha256_token(settings, &full_for_token); - let valid: bool = expected.as_bytes().ct_eq(sig.as_bytes()).into(); + // Constant-time comparison to prevent timing side-channel attacks on the token. + // Length is not secret (always 43 bytes for base64url-encoded SHA-256), + // but we check explicitly to document the invariant. + let valid = + expected.len() == sig.len() && bool::from(expected.as_bytes().ct_eq(sig.as_bytes())); if !valid { return Err(Report::new(TrustedServerError::Proxy { message: "invalid tstoken".to_string(),