From bcb45bf8d08624ae2eb6122770725562bf375a37 Mon Sep 17 00:00:00 2001 From: Trezy Date: Wed, 8 Jul 2026 17:55:32 -0500 Subject: [PATCH] fix: secret comparisons now use constant time Signed-off-by: Trezy --- Cargo.lock | 1 + Cargo.toml | 1 + src/constant_time.rs | 49 ++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/oauth/client_auth.rs | 4 ++-- src/rate_limit.rs | 2 +- 6 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 src/constant_time.rs diff --git a/Cargo.lock b/Cargo.lock index 0005bff..a0027c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1844,6 +1844,7 @@ dependencies = [ "sha2 0.11.0", "sqlparser", "sqlx", + "subtle", "thiserror 2.0.18", "tokio", "tokio-rustls", diff --git a/Cargo.toml b/Cargo.toml index 127d634..7980575 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,6 +62,7 @@ blake3 = "1" hkdf = "0.13" hmac = "0.13" sqlparser = "0.62.0" +subtle = "2" [[bin]] name = "migrate-lua-sql" diff --git a/src/constant_time.rs b/src/constant_time.rs new file mode 100644 index 0000000..eb0e13a --- /dev/null +++ b/src/constant_time.rs @@ -0,0 +1,49 @@ +//! Constant-time comparison helpers for secrets and their hashes. +//! +//! Comparing secret material (or its digest) with `==` can leak how many +//! leading bytes matched via early-exit timing. These helpers compare in time +//! independent of the *content* of equal-length inputs. Input length is not +//! treated as secret — the values compared here are fixed-length hashes or +//! attacker-known challenges — so an early length-mismatch return is fine. + +/// Constant-time equality over two byte slices. `subtle`'s slice comparison +/// short-circuits only on a length mismatch (length is not secret here); for +/// equal-length inputs it compares every byte regardless of where they differ. +pub fn ct_eq(a: &[u8], b: &[u8]) -> bool { + use subtle::ConstantTimeEq; + a.ct_eq(b).into() +} + +/// Constant-time equality over two strings (compares their UTF-8 bytes). +pub fn ct_eq_str(a: &str, b: &str) -> bool { + ct_eq(a.as_bytes(), b.as_bytes()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn equal_values_match() { + assert!(ct_eq_str("a1b2c3", "a1b2c3")); + assert!(ct_eq(b"\x00\x01\x02", b"\x00\x01\x02")); + } + + #[test] + fn different_same_length_do_not_match() { + assert!(!ct_eq_str("a1b2c3", "a1b2c4")); + // Differing only in the first byte must also be rejected. + assert!(!ct_eq_str("X1b2c3", "a1b2c3")); + } + + #[test] + fn different_length_does_not_match() { + assert!(!ct_eq_str("abc", "abcd")); + assert!(!ct_eq_str("abcd", "abc")); + } + + #[test] + fn empty_values_match() { + assert!(ct_eq_str("", "")); + } +} diff --git a/src/lib.rs b/src/lib.rs index 51711f1..6386fa4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ pub mod admin; pub mod auth; pub mod config; +pub mod constant_time; pub mod db; pub mod delegation; pub mod dev_happyview; diff --git a/src/oauth/client_auth.rs b/src/oauth/client_auth.rs index 36841c1..728dbc6 100644 --- a/src/oauth/client_auth.rs +++ b/src/oauth/client_auth.rs @@ -36,7 +36,7 @@ pub async fn authenticate_confidential( let (id, key, client_type, scopes, origins_json, stored_hash) = row.ok_or_else(|| AppError::Auth("invalid client credentials".into()))?; - if stored_hash != secret_hash { + if !crate::constant_time::ct_eq_str(&stored_hash, &secret_hash) { return Err(AppError::Auth("invalid client credentials".into())); } @@ -290,7 +290,7 @@ pub fn verify_pkce(challenge: &str, verifier: &str) -> bool { use base64::engine::general_purpose::URL_SAFE_NO_PAD; let hash = Sha256::digest(verifier.as_bytes()); let computed = URL_SAFE_NO_PAD.encode(hash); - computed == challenge + crate::constant_time::ct_eq_str(&computed, challenge) } #[cfg(test)] diff --git a/src/rate_limit.rs b/src/rate_limit.rs index af571c2..ccc3476 100644 --- a/src/rate_limit.rs +++ b/src/rate_limit.rs @@ -198,7 +198,7 @@ impl RateLimiter { use sha2::{Digest, Sha256}; if let Some(identity) = self.client_identities.get(client_key) { let hash = hex::encode(Sha256::digest(secret.as_bytes())); - hash == identity.secret_hash + crate::constant_time::ct_eq_str(&hash, &identity.secret_hash) } else { false } -- 2.51.2