fix(protocols): constant-time FormPost signature check and guard DLO/SLO range underflow (#4519)

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![

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");

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 {

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(&params.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.
+/// Compares byte-by-byte, always checking all bytes.
-/// Prevents attackers from determining correct prefix by measuring response time.
+/// 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() {
+    for i in 0..a.len() {
-        result |= a_bytes[i] ^ b_bytes[i];
+        result |= a[i] ^ b[i];
     }
 
     result == 0
@@ -379,27 +379,27 @@ mod tests {
 
     #[test]
     fn test_constant_time_compare() {
-        // Equal strings
+        // Equal byte slices
-        assert!(constant_time_compare("hello", "hello"));
+        assert!(constant_time_compare(b"hello", b"hello"));
 
-        // Different strings (same length)
+        // Different content (same length)
-        assert!(!constant_time_compare("hello", "world"));
+        assert!(!constant_time_compare(b"hello", b"world"));
 
         // Different lengths
-        assert!(!constant_time_compare("hello", "hello!"));
+        assert!(!constant_time_compare(b"hello", b"hello!"));
-        assert!(!constant_time_compare("hello!", "hello"));
+        assert!(!constant_time_compare(b"hello!", b"hello"));
 
-        // Empty strings
+        // Empty slices
-        assert!(constant_time_compare("", ""));
+        assert!(constant_time_compare(b"", b""));
 
         // Hex strings (like signatures)
         assert!(constant_time_compare(
-            "da39a3ee5e6b4b0d3255bfef95601890afd80709",
+            b"da39a3ee5e6b4b0d3255bfef95601890afd80709",
-            "da39a3ee5e6b4b0d3255bfef95601890afd80709"
+            b"da39a3ee5e6b4b0d3255bfef95601890afd80709"
         ));
         assert!(!constant_time_compare(
-            "da39a3ee5e6b4b0d3255bfef95601890afd80709",
+            b"da39a3ee5e6b4b0d3255bfef95601890afd80709",
-            "da39a3ee5e6b4b0d3255bfef95601890afd80708"
+            b"da39a3ee5e6b4b0d3255bfef95601890afd80708"
         )); // last char differs
     }