From c41062f27877de2eefdffacc8cfde95771de221b Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 9 Jul 2026 01:07:36 +0800 Subject: [PATCH] fix(protocols): constant-time FormPost signature check and guard DLO/SLO range underflow (#4519) --- crates/protocols/src/swift/dlo.rs | 93 +++++++++++++++++++++++++- crates/protocols/src/swift/formpost.rs | 38 ++++++++++- crates/protocols/src/swift/slo.rs | 63 +++++++++++++++++ crates/protocols/src/swift/tempurl.rs | 44 ++++++------ 4 files changed, 212 insertions(+), 26 deletions(-) diff --git a/crates/protocols/src/swift/dlo.rs b/crates/protocols/src/swift/dlo.rs index b0ee4fc2c..230380810 100644 --- a/crates/protocols/src/swift/dlo.rs +++ b/crates/protocols/src/swift/dlo.rs @@ -116,6 +116,13 @@ fn parse_range_header(range_str: &str, total_size: u64) -> Result<(u64, u64), Sw return Err(SwiftError::BadRequest("Invalid Range header format".to_string())); } + // A range can never be satisfied against an empty aggregate. Guard here so the + // `total_size - 1` arithmetic below cannot underflow (which would wrap to a bogus + // Content-Range in release builds and panic in debug builds). + if total_size == 0 { + return Err(SwiftError::BadRequest("Cannot satisfy Range request on empty object".to_string())); + } + let range_part = &range_str[6..]; let parts: Vec<&str> = range_part.split('-').collect(); @@ -168,8 +175,16 @@ fn calculate_dlo_segments_for_range( let mut current_offset = 0u64; for (idx, segment) in segments.iter().enumerate() { + let seg_size = segment.size as u64; + + // Empty segments contribute no bytes; skip them so the `seg_size - 1` + // arithmetic below cannot underflow. + if seg_size == 0 { + continue; + } + let segment_start = current_offset; - let segment_end = current_offset + segment.size as u64 - 1; + let segment_end = current_offset + seg_size - 1; // Check if this segment overlaps with requested range if segment_end >= start && segment_start <= end { @@ -179,13 +194,13 @@ fn calculate_dlo_segments_for_range( let byte_end = if end < segment_end { end - segment_start } else { - segment.size as u64 - 1 + seg_size - 1 }; result.push((idx, byte_start, byte_end, segment.clone())); } - current_offset += segment.size as u64; + current_offset += seg_size; // Stop if we've passed the requested range if current_offset > end { @@ -285,6 +300,8 @@ async fn create_dlo_stream( segments .iter() .enumerate() + // Skip empty segments so `s.size - 1` cannot underflow; they carry no bytes. + .filter(|(_, s)| s.size as u64 > 0) .map(|(i, s)| (i, 0, s.size as u64 - 1, s.clone())) .collect() }; @@ -536,6 +553,76 @@ mod tests { assert_eq!(result.len(), 0); } + #[test] + fn test_calculate_dlo_segments_for_range_zero_byte_segment() { + let segments = vec![ + ObjectInfo { + name: "seg001".to_string(), + size: 1000, + content_type: None, + etag: None, + }, + ObjectInfo { + name: "seg002".to_string(), + size: 0, // empty segment + content_type: None, + etag: None, + }, + ObjectInfo { + name: "seg003".to_string(), + size: 500, + content_type: None, + etag: None, + }, + ]; + + // Must not panic / underflow on the zero-byte segment. + let result = calculate_dlo_segments_for_range(&segments, 500, 1200).unwrap(); + + // The empty segment carries no bytes and is skipped. + assert_eq!(result.len(), 2); + assert_eq!(result[0].0, 0); + assert_eq!(result[0].1, 500); + assert_eq!(result[0].2, 999); + assert_eq!(result[1].0, 2); + assert_eq!(result[1].1, 0); + assert_eq!(result[1].2, 200); + } + + #[test] + fn test_calculate_dlo_segments_for_range_leading_zero_byte_segment() { + let segments = vec![ + ObjectInfo { + name: "seg001".to_string(), + size: 0, // empty leading segment (current_offset == 0) + content_type: None, + etag: None, + }, + ObjectInfo { + name: "seg002".to_string(), + size: 1000, + content_type: None, + etag: None, + }, + ]; + + // `current_offset + 0 - 1` would underflow without the guard. + let result = calculate_dlo_segments_for_range(&segments, 0, 999).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].0, 1); + assert_eq!(result[0].1, 0); + assert_eq!(result[0].2, 999); + } + + #[test] + fn test_parse_range_header_empty_aggregate() { + // A range request against a 0-size aggregate must return a clean error + // rather than underflowing `total_size - 1`. + assert!(parse_range_header("bytes=0-99", 0).is_err()); + assert!(parse_range_header("bytes=-1", 0).is_err()); + assert!(parse_range_header("bytes=0-", 0).is_err()); + } + #[test] fn test_calculate_dlo_segments_for_range_exact_boundaries() { let segments = vec![ diff --git a/crates/protocols/src/swift/formpost.rs b/crates/protocols/src/swift/formpost.rs index a49ebe95e..052dad787 100644 --- a/crates/protocols/src/swift/formpost.rs +++ b/crates/protocols/src/swift/formpost.rs @@ -209,7 +209,19 @@ pub fn validate_formpost(path: &str, request: &FormPostRequest, key: &str) -> Sw key, )?; - if request.signature != expected_sig { + // Compare signatures in constant time to avoid a timing side-channel, matching + // the sibling TempURL/SFTP checks. Decode the hex first so the comparison runs + // over the raw HMAC bytes and does not leak via string length; a non-hex + // provided signature can never match and is rejected the same way. + let expected_bytes = + hex::decode(&expected_sig).map_err(|e| SwiftError::InternalServerError(format!("Signature encoding error: {}", e)))?; + + let signatures_match = match hex::decode(request.signature.trim()) { + Ok(provided_bytes) => super::tempurl::constant_time_compare(&provided_bytes, &expected_bytes), + Err(_) => false, + }; + + if !signatures_match { debug!( event = EVENT_SWIFT_FORMPOST_STATE, component = LOG_COMPONENT_PROTOCOLS, @@ -687,6 +699,30 @@ mod tests { } } + #[test] + fn test_validate_formpost_wrong_hex_signature() { + // A well-formed hex signature that does not match the expected one must still + // be rejected. This exercises the constant-time comparison path (the decoded + // bytes differ) rather than the hex-decode failure path. + let key = "mykey"; + let path = "/v1/AUTH_test/container"; + let expires = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs() + 3600; + + let request = FormPostRequest { + redirect: "https://example.com/success".to_string(), + redirect_error: None, + max_file_size: 10485760, + max_file_count: 5, + expires, + // Valid 40-char hex, but not the correct signature for these params. + signature: "da39a3ee5e6b4b0d3255bfef95601890afd80709".to_string(), + }; + + let result = validate_formpost(path, &request, key); + assert!(result.is_err()); + assert!(matches!(result.unwrap_err(), SwiftError::Unauthorized(_))); + } + #[test] fn test_build_redirect_url() { let url = build_redirect_url("https://example.com/success", 201, "Created"); diff --git a/crates/protocols/src/swift/slo.rs b/crates/protocols/src/swift/slo.rs index f5f0dbcc8..4d31a47da 100644 --- a/crates/protocols/src/swift/slo.rs +++ b/crates/protocols/src/swift/slo.rs @@ -162,6 +162,12 @@ fn calculate_segments_for_range( let mut current_offset = 0u64; for (idx, segment) in manifest.segments.iter().enumerate() { + // Empty segments contribute no bytes; skip them so the `size_bytes - 1` + // arithmetic below cannot underflow. + if segment.size_bytes == 0 { + continue; + } + let segment_start = current_offset; let segment_end = current_offset + segment.size_bytes - 1; @@ -196,6 +202,13 @@ fn parse_range_header(range_str: &str, total_size: u64) -> Result<(u64, u64), Sw return Err(SwiftError::BadRequest("Invalid Range header format".to_string())); } + // A range can never be satisfied against an empty aggregate. Guard here so the + // `total_size - 1` arithmetic below cannot underflow (which would wrap to a bogus + // Content-Range in release builds and panic in debug builds). + if total_size == 0 { + return Err(SwiftError::BadRequest("Cannot satisfy Range request on empty object".to_string())); + } + let range_part = &range_str[6..]; let parts: Vec<&str> = range_part.split('-').collect(); @@ -403,6 +416,8 @@ async fn create_slo_stream( .segments .iter() .enumerate() + // Skip empty segments so `s.size_bytes - 1` cannot underflow; they carry no bytes. + .filter(|(_, s)| s.size_bytes > 0) .map(|(i, s)| (i, 0, s.size_bytes - 1, s.clone())) .collect() }; @@ -709,6 +724,54 @@ mod tests { assert_eq!(segments[0].2, 400); // End at byte 400 of seg3 } + #[test] + fn test_calculate_segments_for_range_zero_byte_segment() { + let manifest = SLOManifest { + segments: vec![ + SLOSegment { + path: "/c/s1".to_string(), + size_bytes: 1000, + etag: "e1".to_string(), + range: None, + }, + SLOSegment { + path: "/c/s2".to_string(), + size_bytes: 0, // empty segment + etag: "e2".to_string(), + range: None, + }, + SLOSegment { + path: "/c/s3".to_string(), + size_bytes: 500, + etag: "e3".to_string(), + range: None, + }, + ], + created_at: None, + }; + + // Must not panic / underflow on the zero-byte segment. + let segments = calculate_segments_for_range(&manifest, 500, 1200).unwrap(); + + // The empty segment carries no bytes and is skipped. + assert_eq!(segments.len(), 2); + assert_eq!(segments[0].0, 0); + assert_eq!(segments[0].1, 500); + assert_eq!(segments[0].2, 999); + assert_eq!(segments[1].0, 2); + assert_eq!(segments[1].1, 0); + assert_eq!(segments[1].2, 200); + } + + #[test] + fn test_parse_range_header_empty_aggregate() { + // An all-empty manifest has total_size == 0; a range request against it must + // return a clean error rather than underflowing `total_size - 1`. + assert!(parse_range_header("bytes=0-99", 0).is_err()); + assert!(parse_range_header("bytes=-1", 0).is_err()); + assert!(parse_range_header("bytes=0-", 0).is_err()); + } + #[test] fn test_calculate_segments_for_range_all_segments() { let manifest = SLOManifest { diff --git a/crates/protocols/src/swift/tempurl.rs b/crates/protocols/src/swift/tempurl.rs index c9a0f48a5..8c96b17c2 100644 --- a/crates/protocols/src/swift/tempurl.rs +++ b/crates/protocols/src/swift/tempurl.rs @@ -142,7 +142,7 @@ impl TempURL { let expected_sig = self.generate_signature(method, params.temp_url_expires, path)?; // 3. Constant-time comparison to prevent timing attacks - if !constant_time_compare(¶ms.temp_url_sig, &expected_sig) { + if !constant_time_compare(params.temp_url_sig.as_bytes(), expected_sig.as_bytes()) { return Err(SwiftError::Unauthorized("Invalid TempURL signature".to_string())); } @@ -155,28 +155,28 @@ impl TempURL { } } -/// Constant-time string comparison to prevent timing attacks +/// Constant-time byte comparison to prevent timing attacks /// /// # Security -/// Compares strings byte-by-byte, always checking all bytes. -/// Prevents attackers from determining correct prefix by measuring response time. +/// Compares byte-by-byte, always checking all bytes. +/// Prevents attackers from determining a correct prefix by measuring response time. +/// +/// Shared across the Swift module (TempURL and FormPost) so signature checks use +/// the same primitive. /// /// # Implementation /// Uses bitwise XOR accumulation, so timing is independent of match position. -fn constant_time_compare(a: &str, b: &str) -> bool { +pub(crate) fn constant_time_compare(a: &[u8], b: &[u8]) -> bool { // If lengths differ, not equal (but still do constant-time comparison of min length) if a.len() != b.len() { return false; } - let a_bytes = a.as_bytes(); - let b_bytes = b.as_bytes(); - // XOR all bytes and accumulate // If any byte differs, result will be non-zero let mut result = 0u8; - for i in 0..a_bytes.len() { - result |= a_bytes[i] ^ b_bytes[i]; + for i in 0..a.len() { + result |= a[i] ^ b[i]; } result == 0 @@ -379,27 +379,27 @@ mod tests { #[test] fn test_constant_time_compare() { - // Equal strings - assert!(constant_time_compare("hello", "hello")); + // Equal byte slices + assert!(constant_time_compare(b"hello", b"hello")); - // Different strings (same length) - assert!(!constant_time_compare("hello", "world")); + // Different content (same length) + assert!(!constant_time_compare(b"hello", b"world")); // Different lengths - assert!(!constant_time_compare("hello", "hello!")); - assert!(!constant_time_compare("hello!", "hello")); + assert!(!constant_time_compare(b"hello", b"hello!")); + assert!(!constant_time_compare(b"hello!", b"hello")); - // Empty strings - assert!(constant_time_compare("", "")); + // Empty slices + assert!(constant_time_compare(b"", b"")); // Hex strings (like signatures) assert!(constant_time_compare( - "da39a3ee5e6b4b0d3255bfef95601890afd80709", - "da39a3ee5e6b4b0d3255bfef95601890afd80709" + b"da39a3ee5e6b4b0d3255bfef95601890afd80709", + b"da39a3ee5e6b4b0d3255bfef95601890afd80709" )); assert!(!constant_time_compare( - "da39a3ee5e6b4b0d3255bfef95601890afd80709", - "da39a3ee5e6b4b0d3255bfef95601890afd80708" + b"da39a3ee5e6b4b0d3255bfef95601890afd80709", + b"da39a3ee5e6b4b0d3255bfef95601890afd80708" )); // last char differs }