fix(auth): compare sensitive tokens in constant time (#5371)

* fix(auth): compare sensitive tokens in constant time

* test(auth): scope constant-time source guard

* test(auth): ignore test source in comparison guard

* chore(deps): use constant-time s3s authentication
This commit is contained in:
cxymds
2026-07-28 17:56:22 +08:00
committed by GitHub
parent 7f146fc5de
commit eb755e2b97
5 changed files with 57 additions and 9 deletions
Generated
+1 -1
View File
@@ -10355,7 +10355,7 @@ checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f"
[[package]]
name = "s3s"
version = "0.14.1"
source = "git+https://github.com/cxymds/s3s.git?rev=afa1796ec64cd1e1c78dfee46c4b4b72bbc7b39d#afa1796ec64cd1e1c78dfee46c4b4b72bbc7b39d"
source = "git+https://github.com/cxymds/s3s.git?rev=fe3941d91fa1c69956f209a9145995c9f0235bff#fe3941d91fa1c69956f209a9145995c9f0235bff"
dependencies = [
"arc-swap",
"arrayvec",
+1 -1
View File
@@ -288,7 +288,7 @@ redis = { version = "1.4.1" }
rustix = { version = "1.1.4" }
rust-embed = { version = "8.12.0" }
rustc-hash = { version = "2.1.3" }
s3s = { git = "https://github.com/cxymds/s3s.git", rev = "afa1796ec64cd1e1c78dfee46c4b4b72bbc7b39d" }
s3s = { git = "https://github.com/cxymds/s3s.git", rev = "fe3941d91fa1c69956f209a9145995c9f0235bff" }
serial_test = "4.0.1"
shadow-rs = { default-features = false, version = "2.0.0" }
siphasher = "1.0.3"
+1 -1
View File
@@ -40,7 +40,7 @@ allow-git = [
# SigV4 payload-checksum fix until it is available in a crates.io release.
# owner: marshawcoco review: 2026-10
"https://github.com/s3s-project/s3s.git",
# Presigned URL expiry validation pending upstream submission.
# Presigned expiry and constant-time authentication fixes pending upstream merge.
# owner: cxymds review: 2026-10
"https://github.com/cxymds/s3s.git",
"https://github.com/apache/datafusion.git",
+37 -2
View File
@@ -33,7 +33,7 @@ use crate::admin::runtime_sources::{
};
use crate::admin::storage_api::access::{ReqInfo, authorize_request, spawn_traced};
use crate::admin::storage_api::contract::bucket::{BucketOperations, BucketOptions};
use crate::auth::{check_key_valid, get_session_token};
use crate::auth::{check_key_valid, constant_time_eq, get_session_token};
use crate::error::ApiError;
use crate::license::license_check;
use crate::server::{
@@ -905,7 +905,9 @@ fn validate_object_lambda_response_auth_headers(headers: &HeaderMap, output_rout
.and_then(|value| value.to_str().ok())
.map(str::trim);
if route == Some(output_route) && token == Some(output_token) {
if route.is_some_and(|route| constant_time_eq(route, output_route))
&& token.is_some_and(|token| constant_time_eq(token, output_token))
{
return Ok(());
}
@@ -4351,6 +4353,39 @@ mod tests {
let err = validate_object_lambda_response_auth_headers(&mismatched, "route-123", "token-456")
.expect_err("mismatched auth headers should fail");
assert_eq!(err.code(), &S3ErrorCode::InvalidRequest);
for (route, token) in [
("Route-123", "token-456"),
("route-124", "token-456"),
("route-1234", "token-456"),
("route-123", "Token-456"),
("route-123", "token-457"),
("route-123", "token-4567"),
] {
let mut headers = HeaderMap::new();
headers.insert(
"x-amz-request-route",
HeaderValue::try_from(route).expect("test route must be a valid header"),
);
headers.insert(
"x-amz-request-token",
HeaderValue::try_from(token).expect("test token must be a valid header"),
);
assert!(
validate_object_lambda_response_auth_headers(&headers, "route-123", "token-456").is_err(),
"first-byte, last-byte, and length mismatches must fail: {route}/{token}"
);
}
}
#[test]
fn object_lambda_auth_headers_use_constant_time_helper() {
let source = include_str!("router.rs");
let production = source.split_once("#[cfg(test)]").map_or(source, |(production, _)| production);
assert!(!production.contains("route == Some(output_route)"));
assert!(!production.contains("token == Some(output_token)"));
assert!(production.contains("constant_time_eq(route, output_route)"));
assert!(production.contains("constant_time_eq(token, output_token)"));
}
#[test]
+17 -4
View File
@@ -452,7 +452,7 @@ pub fn check_claims_from_token(token: &str, cred: &Credentials) -> S3Result<Hash
return Err(s3_error!(InvalidRequest, "invalid token2"));
}
if !cred.is_service_account() && cred.is_temp() && token != cred.session_token {
if !cred.is_service_account() && cred.is_temp() && !constant_time_eq(token, &cred.session_token) {
return Err(s3_error!(InvalidRequest, "invalid token3"));
}
@@ -1801,9 +1801,10 @@ mod tests {
#[test]
fn test_constant_time_eq() {
assert!(constant_time_eq("test", "test"));
assert!(!constant_time_eq("test", "Test"));
assert!(!constant_time_eq("test", "test1"));
assert!(!constant_time_eq("test1", "test"));
assert!(!constant_time_eq("Test", "test"), "first-byte mismatch must fail");
assert!(!constant_time_eq("tesu", "test"), "last-byte mismatch must fail");
assert!(!constant_time_eq("test", "test1"), "longer candidate must fail");
assert!(!constant_time_eq("test1", "test"), "shorter candidate must fail");
assert!(!constant_time_eq("", "test"));
assert!(constant_time_eq("", ""));
@@ -1815,6 +1816,18 @@ mod tests {
assert!(!constant_time_eq(key1, key3));
}
#[test]
fn session_token_comparison_uses_constant_time_helper() {
let source = include_str!("auth.rs");
let production = source.split_once("#[cfg(test)]").map_or(source, |(production, _)| production);
let ordinary_comparison = ["token ", "!=", " cred.session_token"].concat();
assert!(
!production.contains(&ordinary_comparison),
"temporary session tokens must not use ordinary string comparison"
);
assert!(production.contains("!constant_time_eq(token, &cred.session_token)"));
}
#[test]
fn test_get_condition_values_source_ip() {
let mut headers = HeaderMap::new();