security: Fix timing attack vulnerability in credential comparison #1014
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix a critical timing attack vulnerability in credential comparison by replacing standard == comparisons with a custom constant_time_eq() function across authentication-related code. However, the implementation has critical security flaws that prevent it from achieving its stated security goals.
Purpose: Prevent timing side-channel attacks on credential validation by implementing constant-time string comparison.
Key Changes:
- Added a custom
constant_time_eq()function usingAtomicU8for comparisons - Replaced 8 credential comparison operations across 4 files
- Added unit tests for the new function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
rustfs/src/auth.rs |
Introduced constant_time_eq() function and replaced 3 credential comparisons in check_key_valid() and get_condition_values() functions |
rustfs/src/admin/handlers/user.rs |
Replaced 2 credential comparisons in user management operations (AddUser and SetUserStatus) |
rustfs/src/admin/handlers/service_account.rs |
Replaced 2 credential comparisons in service account creation logic |
rustfs/src/admin/handlers/group.rs |
Replaced 1 credential comparison when validating group member additions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rustfs/src/auth.rs
Outdated
| pub fn constant_time_eq(a: &str, b: &str) -> bool { | ||
| use std::sync::atomic::{AtomicU8, Ordering}; | ||
| if a.len() != b.len() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Missing documentation for this security-critical function. Add documentation that:
- Explains the function's purpose (preventing timing attacks)
- Documents the security properties it provides
- Clarifies when it should be used
- Notes any limitations
Example:
/// 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.
///
/// # Security Note
/// This implementation uses the `subtle` crate to provide cryptographically
/// sound constant-time guarantees.
pub fn constant_time_eq(a: &str, b: &str) -> bool {| pub fn constant_time_eq(a: &str, b: &str) -> bool { | |
| use std::sync::atomic::{AtomicU8, Ordering}; | |
| if a.len() != b.len() { | |
| return false; | |
| } |
rustfs/src/auth.rs
Outdated
| pub fn constant_time_eq(a: &str, b: &str) -> bool { | ||
| use std::sync::atomic::{AtomicU8, Ordering}; | ||
| if a.len() != b.len() { | ||
| return false; | ||
| } | ||
| let result = AtomicU8::new(0); | ||
| for (byte_a, byte_b) in a.bytes().zip(b.bytes()) { | ||
| result.fetch_or(byte_a ^ byte_b, Ordering::Relaxed); | ||
| } | ||
| result.load(Ordering::Relaxed) == 0 |
There was a problem hiding this comment.
Critical Security Issue: This constant-time comparison implementation is not actually constant-time and does not prevent timing attacks.
Problems with the current implementation:
-
Early length check leak (lines 34-36): Returning early when lengths differ leaks timing information. Attackers can determine the correct credential length by measuring response times.
-
Not constant-time loop: Using
AtomicU8doesn't make the loop constant-time. The loop still takes time proportional to the string length, and modern CPUs with branch prediction, cache effects, and out-of-order execution can still leak timing information. -
No compiler optimization barriers: The implementation doesn't use proper compiler fences or volatile operations to prevent the optimizer from short-circuiting the comparison.
Recommended Solution:
Use the industry-standard subtle crate which provides cryptographically-verified constant-time comparisons:
use subtle::ConstantTimeEq;
pub fn constant_time_eq(a: &str, b: &str) -> bool {
a.as_bytes().ct_eq(b.as_bytes()).into()
}This handles length differences securely and is audited for constant-time behavior.
Add to Cargo.toml:
subtle = "2.6"The subtle crate is specifically designed for this purpose and is widely used in cryptographic applications.
| pub fn constant_time_eq(a: &str, b: &str) -> bool { | |
| use std::sync::atomic::{AtomicU8, Ordering}; | |
| if a.len() != b.len() { | |
| return false; | |
| } | |
| let result = AtomicU8::new(0); | |
| for (byte_a, byte_b) in a.bytes().zip(b.bytes()) { | |
| result.fetch_or(byte_a ^ byte_b, Ordering::Relaxed); | |
| } | |
| result.load(Ordering::Relaxed) == 0 | |
| use subtle::ConstantTimeEq; | |
| pub fn constant_time_eq(a: &str, b: &str) -> bool { | |
| a.as_bytes().ct_eq(b.as_bytes()).into() |
Response to Copilot AI ReviewThank you for the thorough security review! You're absolutely right about the critical flaws in my initial implementation. Issues Addressed✅ Fixed timing leaks: Removed early length check that leaked timing information Changes Made
Why
|
Implemented constant-time string comparison for access_key validation to prevent timing attacks that could leak credential information. Changes: - Added constant_time_eq() function using atomic operations - Replaced 8 unsafe == comparisons with constant-time version - Applied fix in auth.rs and admin handlers (user, service_account, group) - Added unit tests for constant_time_eq() Security Impact: - Prevents timing side-channel attacks on authentication - Protects admin and service account credentials - Follows security best practices for sensitive data comparison Fixes: Critical timing attack vulnerability in authentication system
…comparison Address Copilot AI review feedback: 1. Replace custom implementation with industry-standard 'subtle' crate 2. Add comprehensive documentation explaining security properties 3. Fix timing leaks from early length check 4. Ensure proper compiler optimization barriers The subtle crate provides cryptographically-verified constant-time comparisons that are resistant to: - Branch prediction timing leaks - Cache timing attacks - CPU out-of-order execution leaks - Compiler optimization short-circuits Co-authored-by: Copilot AI <[email protected]>
loverustfs
force-pushed
the
fix/timing-attack-vulnerability
branch
from
2584894 to
930f681
Compare
|
Hey @wmh , Using Lint to check, there may still be some code compatibility bugs. Thank you for your contribution. |
3 participants
🔒 Security Fix: Timing Attack Vulnerability
Problem
The authentication system was using standard
==comparisons for sensitive credential validation (access_key), which is vulnerable to timing attacks. Attackers could potentially infer correct credentials by measuring comparison execution time.Solution
Implemented a constant-time string comparison function using atomic operations to ensure comparison time is independent of string content differences.
Changes
constant_time_eq()function usingAtomicU8for constant-time comparison==comparisons across authentication codeauth.rs,user.rs,service_account.rs,group.rsFiles Modified
rustfs/src/auth.rs- Core auth logic (3 fixes + new function)rustfs/src/admin/handlers/user.rs- User management (2 fixes)rustfs/src/admin/handlers/service_account.rs- Service accounts (2 fixes)rustfs/src/admin/handlers/group.rs- Group management (1 fix)Security Impact
Testing
constant_time_eq()Risk Level
CRITICAL - Timing attacks can be exploited remotely to brute-force credentials
cc: @rustfs/security