From 72930b1e30007ccca746063b7d68bd8ddc3fd98d Mon Sep 17 00:00:00 2001 From: Hunter Wu Date: Sat, 6 Dec 2025 15:13:27 +0800 Subject: [PATCH] security: Fix timing attack vulnerability in credential comparison (#1014) Co-authored-by: Copilot AI --- .github/workflows/helm-package.yml | 7 ++- Cargo.toml | 1 + rustfs/Cargo.toml | 1 + rustfs/src/admin/handlers/group.rs | 4 +- rustfs/src/admin/handlers/service_account.rs | 6 +-- rustfs/src/admin/handlers/user.rs | 6 +-- rustfs/src/auth.rs | 52 ++++++++++++++++++-- 7 files changed, 65 insertions(+), 12 deletions(-) diff --git a/.github/workflows/helm-package.yml b/.github/workflows/helm-package.yml index 0dca2db7..9c7b46ee 100644 --- a/.github/workflows/helm-package.yml +++ b/.github/workflows/helm-package.yml @@ -6,11 +6,16 @@ on: types: [completed] env: - new_version: ${{ github.ref_name }} + new_version: ${{ github.event.workflow_run.head_branch }} jobs: build-helm-package: runs-on: ubuntu-latest + # Only run on successful builds triggered by tag pushes (version format: x.y.z or x.y.z-suffix) + if: | + github.event.workflow_run.conclusion == 'success' && + github.event.workflow_run.event == 'push' && + contains(github.event.workflow_run.head_branch, '.') steps: - name: Checkout helm chart repo diff --git a/Cargo.toml b/Cargo.toml index c369f677..f4608aef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -152,6 +152,7 @@ rustls-pemfile = "2.2.0" rustls-pki-types = "1.13.1" sha1 = "0.11.0-rc.3" sha2 = "0.11.0-rc.3" +subtle = "2.6" zeroize = { version = "1.8.2", features = ["derive"] } # Time and Date diff --git a/rustfs/Cargo.toml b/rustfs/Cargo.toml index 4552bf79..2563d4c7 100644 --- a/rustfs/Cargo.toml +++ b/rustfs/Cargo.toml @@ -92,6 +92,7 @@ serde_urlencoded = { workspace = true } # Cryptography and Security rustls = { workspace = true } +subtle = { workspace = true } # Time and Date chrono = { workspace = true } diff --git a/rustfs/src/admin/handlers/group.rs b/rustfs/src/admin/handlers/group.rs index 40b1149a..953f3105 100644 --- a/rustfs/src/admin/handlers/group.rs +++ b/rustfs/src/admin/handlers/group.rs @@ -29,7 +29,7 @@ use tracing::warn; use crate::{ admin::{auth::validate_admin_request, router::Operation, utils::has_space_be}, - auth::{check_key_valid, get_session_token}, + auth::{check_key_valid, constant_time_eq, get_session_token}, }; #[derive(Debug, Deserialize, Default)] @@ -240,7 +240,7 @@ impl Operation for UpdateGroupMembers { get_global_action_cred() .map(|cred| { - if cred.access_key == *member { + if constant_time_eq(&cred.access_key, member) { return Err(S3Error::with_message( S3ErrorCode::MethodNotAllowed, format!("can't add root {member}"), diff --git a/rustfs/src/admin/handlers/service_account.rs b/rustfs/src/admin/handlers/service_account.rs index 4295de90..935abcc0 100644 --- a/rustfs/src/admin/handlers/service_account.rs +++ b/rustfs/src/admin/handlers/service_account.rs @@ -13,7 +13,7 @@ // limitations under the License. use crate::admin::utils::has_space_be; -use crate::auth::{get_condition_values, get_session_token}; +use crate::auth::{constant_time_eq, get_condition_values, get_session_token}; use crate::{admin::router::Operation, auth::check_key_valid}; use http::HeaderMap; use hyper::StatusCode; @@ -83,7 +83,7 @@ impl Operation for AddServiceAccount { return Err(s3_error!(InvalidRequest, "get sys cred failed")); }; - if sys_cred.access_key == create_req.access_key { + if constant_time_eq(&sys_cred.access_key, &create_req.access_key) { return Err(s3_error!(InvalidArgument, "can't create user with system access key")); } @@ -107,7 +107,7 @@ impl Operation for AddServiceAccount { return Err(s3_error!(InvalidRequest, "iam not init")); }; - let deny_only = cred.access_key == target_user || cred.parent_user == target_user; + let deny_only = constant_time_eq(&cred.access_key, &target_user) || constant_time_eq(&cred.parent_user, &target_user); if !iam_store .is_allowed(&Args { diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 986c01cf..be20eda0 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -14,7 +14,7 @@ use crate::{ admin::{auth::validate_admin_request, router::Operation, utils::has_space_be}, - auth::{check_key_valid, get_session_token}, + auth::{check_key_valid, constant_time_eq, get_session_token}, }; use http::{HeaderMap, StatusCode}; use matchit::Params; @@ -95,7 +95,7 @@ impl Operation for AddUser { } if let Some(sys_cred) = get_global_action_cred() { - if sys_cred.access_key == ak { + if constant_time_eq(&sys_cred.access_key, ak) { return Err(s3_error!(InvalidArgument, "can't create user with system access key")); } } @@ -162,7 +162,7 @@ impl Operation for SetUserStatus { return Err(s3_error!(InvalidRequest, "get cred failed")); }; - if input_cred.access_key == ak { + if constant_time_eq(&input_cred.access_key, ak) { return Err(s3_error!(InvalidArgument, "can't change status of self")); } diff --git a/rustfs/src/auth.rs b/rustfs/src/auth.rs index e53bf525..cc2d24c2 100644 --- a/rustfs/src/auth.rs +++ b/rustfs/src/auth.rs @@ -29,9 +29,37 @@ use s3s::auth::SimpleAuth; use s3s::s3_error; use serde_json::Value; use std::collections::HashMap; +use subtle::ConstantTimeEq; use time::OffsetDateTime; use time::format_description::well_known::Rfc3339; +/// Performs constant-time string comparison to prevent timing attacks. +/// +/// This function should be used when comparing sensitive values like passwords, +/// API keys, or authentication tokens. It ensures the comparison time is +/// independent of the position where strings differ and handles length differences +/// securely. +/// +/// # Security Note +/// This implementation uses the `subtle` crate to provide cryptographically +/// sound constant-time guarantees. The function is resistant to timing side-channel +/// attacks and suitable for security-critical comparisons. +/// +/// # Example +/// ``` +/// use rustfs::auth::constant_time_eq; +/// +/// let secret1 = "my-secret-key"; +/// let secret2 = "my-secret-key"; +/// let secret3 = "wrong-secret"; +/// +/// assert!(constant_time_eq(secret1, secret2)); +/// assert!(!constant_time_eq(secret1, secret3)); +/// ``` +pub fn constant_time_eq(a: &str, b: &str) -> bool { + a.as_bytes().ct_eq(b.as_bytes()).into() +} + // Authentication type constants const JWT_ALGORITHM: &str = "Bearer "; const SIGN_V2_ALGORITHM: &str = "AWS "; @@ -111,7 +139,7 @@ pub async fn check_key_valid(session_token: &str, access_key: &str) -> S3Result< let sys_cred = cred.clone(); - if cred.access_key != access_key { + if !constant_time_eq(&cred.access_key, access_key) { let Ok(iam_store) = rustfs_iam::get() else { return Err(S3Error::with_message( S3ErrorCode::InternalError, @@ -146,7 +174,8 @@ pub async fn check_key_valid(session_token: &str, access_key: &str) -> S3Result< cred.claims = if !claims.is_empty() { Some(claims) } else { None }; - let mut owner = sys_cred.access_key == cred.access_key || cred.parent_user == sys_cred.access_key; + let mut owner = + constant_time_eq(&sys_cred.access_key, &cred.access_key) || constant_time_eq(&cred.parent_user, &sys_cred.access_key); // permitRootAccess if let Some(claims) = &cred.claims { @@ -225,7 +254,7 @@ pub fn get_condition_values( let principal_type = if !username.is_empty() { if claims.is_some() { "AssumedRole" - } else if sys_cred.access_key == username { + } else if constant_time_eq(&sys_cred.access_key, &username) { "Account" } else { "User" @@ -1102,4 +1131,21 @@ mod tests { assert_eq!(auth_type, AuthType::Unknown); } + + #[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")); + assert!(constant_time_eq("", "")); + + // Test with credentials-like strings + let key1 = "AKIAIOSFODNN7EXAMPLE"; + let key2 = "AKIAIOSFODNN7EXAMPLE"; + let key3 = "AKIAIOSFODNN7EXAMPLF"; + assert!(constant_time_eq(key1, key2)); + assert!(!constant_time_eq(key1, key3)); + } }