From f01ed4ec2510b0af68ebe87a8d097128fb6a5d28 Mon Sep 17 00:00:00 2001
From: ospab <gvoprgrg@gmail.com>
Date: Sat, 18 Jul 2026 17:38:09 +0300
Subject: [PATCH] fix(server): constant-time comparison for Management API
 secrets

check_token() and handle_login() compared bearer tokens, session tokens,
and the password hash with plain ==, which short-circuits on the first
differing byte - a textbook remote timing side-channel against exactly
the long-lived secrets these gates exist to protect. Added subtle (already
in the dependency tree transitively via chacha20poly1305) as a direct
dependency and route every secret comparison through a small secure_eq()
wrapper over ConstantTimeEq. Username comparison in handle_login is left
as-is: it isn't treated as a secret in this threat model (one fixed admin
username), matching standard practice of only constant-timing the
password/token side of an auth check.

Added tests for secure_eq() itself (equal, different, different-length,
empty) alongside the existing check_token coverage.
---
 Cargo.lock             |  1 +
 ostp-server/Cargo.toml |  1 +
 ostp-server/src/api.rs | 30 +++++++++++++++++++++++++-----
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index bf35549..a9d8fc4 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1496,6 +1496,7 @@ dependencies = [
  "sha2",
  "simple-dns",
  "socket2",
+ "subtle",
  "tokio",
  "tower-http",
  "tracing",
diff --git a/ostp-server/Cargo.toml b/ostp-server/Cargo.toml
index 2cff111..3fe0ff0 100644
--- a/ostp-server/Cargo.toml
+++ b/ostp-server/Cargo.toml
@@ -31,3 +31,4 @@ hex = "0.4.3"
 chacha20poly1305.workspace = true
 x25519-dalek = { version = "2.0.1", features = ["static_secrets"] }
 chrono = "0.4.44"
+subtle = "2.6"
diff --git a/ostp-server/src/api.rs b/ostp-server/src/api.rs
index 13a2996..c64c1d0 100644
--- a/ostp-server/src/api.rs
+++ b/ostp-server/src/api.rs
@@ -318,6 +318,18 @@ pub async fn start_api_server(
 
 // ── Middleware: token check ──────────────────────────────────────────────────
 
+/// Constant-time string equality for secrets (tokens, password hashes).
+/// Plain `==` short-circuits on the first differing byte, which leaks how
+/// many leading bytes an attacker's guess got right through response
+/// timing - a classic remote timing side-channel against exactly the kind
+/// of long-lived bearer/session secrets compared here. `subtle` is already
+/// pulled in transitively (chacha20poly1305 etc.); pinning it as a direct
+/// dependency here makes that guarantee explicit for this call site.
+fn secure_eq(a: &str, b: &str) -> bool {
+    use subtle::ConstantTimeEq;
+    a.as_bytes().ct_eq(b.as_bytes()).into()
+}
+
 fn check_token(state: &ApiState, headers: &axum::http::HeaderMap) -> bool {
     // Both session token (for web UI) and static API token (for relays) are checked
     let mut allowed = false;
@@ -332,19 +344,19 @@ fn check_token(state: &ApiState, headers: &axum::http::HeaderMap) -> bool {
             if let Some(token) = val.strip_prefix("Bearer ") {
                 let current_session = state.session_token.read().unwrap_or_else(|e| e.into_inner()).clone();
                 if let Some(session) = current_session {
-                    if token == session {
+                    if secure_eq(token, &session) {
                         allowed = true;
                     }
                 }
-                
+
                 if let Some(ref api_tok) = state.api_token {
-                    if token == api_tok {
+                    if secure_eq(token, api_tok) {
                         allowed = true;
                     }
                 }
             } else {
                 if let Some(ref api_tok) = state.api_token {
-                    if val == api_tok {
+                    if secure_eq(val, api_tok) {
                         allowed = true;
                     }
                 }
@@ -371,7 +383,7 @@ async fn handle_login(
     let hash = sha2::Sha256::digest(password.as_bytes());
     let hash_hex = format!("{:x}", hash);
 
-    if hash_hex == state.password_hash {
+    if secure_eq(&hash_hex, &state.password_hash) {
         let token = uuid::Uuid::new_v4().to_string();
         *state.session_token.write().unwrap_or_else(|e| e.into_inner()) = Some(token.clone());
         (StatusCode::OK, ApiResponse::success(LoginResponse { token }))
@@ -882,6 +894,14 @@ mod tests {
         let _router = create_api_router(state);
     }
 
+    #[test]
+    fn test_secure_eq_matches_and_rejects() {
+        assert!(secure_eq("same-secret", "same-secret"));
+        assert!(!secure_eq("same-secret", "different"));
+        assert!(!secure_eq("short", "much-longer-value"));
+        assert!(secure_eq("", ""));
+    }
+
     fn headers_with_bearer(token: &str) -> axum::http::HeaderMap {
         let mut h = axum::http::HeaderMap::new();
         h.insert("authorization", format!("Bearer {token}").parse().unwrap());