From 0f2ecb766fde8e65ad8fd900fe5379cd93706216 Mon Sep 17 00:00:00 2001 From: Eliah Rusin Date: Mon, 21 Sep 2026 09:33:24 +0300 Subject: [PATCH] volume server: reject non-ASCII input instead of panicking (#11406) * volume server: reject non-ASCII input instead of panicking Three parsers sliced attacker-supplied strings by byte offset, so a multi-byte character split inside itself and panicked the task: - parse_needle_id_cookie took the last 8 bytes as the cookie and the rest as the needle id. Reachable from VolumeServer.BatchDelete, whose file_ids come straight off the wire as protobuf strings; that handler already answers 400 per bad fid, so the guard turns a panicked RPC into the error it was already written to return. - TTL::read took the unit as the last byte and the count as everything before it, so "?ttl=5" split mid-character. The HTTP upload path does TTL::read(..).ok() and drops an invalid TTL; AllocateVolume maps the Err to InvalidArgument. Both now reject non-ASCII up front. Hex and a digits-plus-unit TTL are ASCII by definition, so no accepted input changes -- covered by tests alongside the rejection cases. The six response-* header overrides were inserted with parse().unwrap(). They come from the query string, so "?response-cache-control=%0Aevil" decodes to a value HeaderValue rejects and the unwrap panicked the connection task, unauthenticated. They now skip the override, matching the if-let the chunked-response path in the same file already uses. ReplicaPlacement::from_string was reported as a fourth site but is not one: reaching chars[2] requires chars[0] and chars[1] to be ASCII digits, which forces the padded string to be three single-byte characters, so a multi-byte character always lands on a to_digit() None first. Kept as a regression test rather than a change. Each fix was confirmed against the unfixed code first: the parser tests panic with "byte index N is not a char boundary", and the integration tests panic at handlers.rs:1413 and ttl.rs:88. Not a vector, contrary to the report: the HTTP request line. The path is not percent-decoded before parsing, so "%C3%A9" stays ASCII and fails the length check. Co-Authored-By: Claude Opus 5 (1M context) * volume server: fall back to needle MIME when response-content-type is invalid Skipping an unparseable override left the response without any Content-Type because the override had already bypassed the normal MIME selection. Also correct a test comment that described a chars[2] panic which cannot be reached. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: chrislusf --- seaweed-volume/src/server/handlers.rs | 51 ++++++--- seaweed-volume/src/storage/needle/needle.rs | 32 ++++++ seaweed-volume/src/storage/needle/ttl.rs | 16 +++ seaweed-volume/src/storage/super_block.rs | 29 +++++ seaweed-volume/tests/http_integration.rs | 113 ++++++++++++++++++++ 5 files changed, 226 insertions(+), 15 deletions(-) diff --git a/seaweed-volume/src/server/handlers.rs b/seaweed-volume/src/server/handlers.rs index 5b8703add..80367055b 100644 --- a/seaweed-volume/src/server/handlers.rs +++ b/seaweed-volume/src/server/handlers.rs @@ -1351,9 +1351,7 @@ async fn get_or_head_handler_inner( // H6: Determine Content-Type: filter application/octet-stream, use mime_guess // For chunk manifests, skip extension-based MIME override — use stored MIME as-is (Go parity) - let content_type = if let Some(ref ct) = query.response_content_type { - Some(ct.clone()) - } else if n.is_chunk_manifest() { + let content_type = if n.is_chunk_manifest() { // Chunk manifests: use stored MIME but filter application/octet-stream (Go L334) if !n.mime.is_empty() { let mt = String::from_utf8_lossy(&n.mime).to_string(); @@ -1402,27 +1400,50 @@ async fn get_or_head_handler_inner( } } }; - if let Some(ref ct) = content_type { - response_headers.insert(header::CONTENT_TYPE, ct.parse().unwrap()); + // Every value below can come straight from the query string, so none of + // them may be unwrapped: `?response-cache-control=%0Aevil` decodes to a + // value with a newline, `HeaderValue::from_str` rejects it, and the unwrap + // would panic the connection task. An invalid `response-content-type` + // falls back to the needle MIME rather than dropping Content-Type. + if let Some(hval) = query + .response_content_type + .as_ref() + .and_then(|ct| ct.parse::().ok()) + { + response_headers.insert(header::CONTENT_TYPE, hval); + } else if let Some(ref ct) = content_type + && let Ok(hval) = ct.parse() + { + response_headers.insert(header::CONTENT_TYPE, hval); } // Cache-Control override from query param - if let Some(ref cc) = query.response_cache_control { - response_headers.insert(header::CACHE_CONTROL, cc.parse().unwrap()); + if let Some(ref cc) = query.response_cache_control + && let Ok(hval) = cc.parse() + { + response_headers.insert(header::CACHE_CONTROL, hval); } // S3 response passthrough headers - if let Some(ref ce) = query.response_content_encoding { - response_headers.insert(header::CONTENT_ENCODING, ce.parse().unwrap()); + if let Some(ref ce) = query.response_content_encoding + && let Ok(hval) = ce.parse() + { + response_headers.insert(header::CONTENT_ENCODING, hval); } - if let Some(ref exp) = query.response_expires { - response_headers.insert(header::EXPIRES, exp.parse().unwrap()); + if let Some(ref exp) = query.response_expires + && let Ok(hval) = exp.parse() + { + response_headers.insert(header::EXPIRES, hval); } - if let Some(ref cl) = query.response_content_language { - response_headers.insert("Content-Language", cl.parse().unwrap()); + if let Some(ref cl) = query.response_content_language + && let Ok(hval) = cl.parse() + { + response_headers.insert("Content-Language", hval); } - if let Some(ref cd) = query.response_content_disposition { - response_headers.insert(header::CONTENT_DISPOSITION, cd.parse().unwrap()); + if let Some(ref cd) = query.response_content_disposition + && let Ok(hval) = cd.parse() + { + response_headers.insert(header::CONTENT_DISPOSITION, hval); } // Last-Modified diff --git a/seaweed-volume/src/storage/needle/needle.rs b/seaweed-volume/src/storage/needle/needle.rs index 2ad129716..d61cb2200 100644 --- a/seaweed-volume/src/storage/needle/needle.rs +++ b/seaweed-volume/src/storage/needle/needle.rs @@ -749,6 +749,14 @@ pub fn parse_needle_id_cookie(s: &str) -> Result<(NeedleId, Cookie), String> { (s, None) }; + // Every length check and the split below are in BYTES, so a multi-byte + // character would let `split` land inside one and panic the slice. Hex is + // ASCII by definition; reject anything else up front, as Go's ParseUint + // does a step later. + if !hex_part.is_ascii() { + return Err("KeyHash must be ASCII hex.".to_string()); + } + // Go: len(key_hash_string) <= CookieSize*2 => error (must be > 8 hex chars) if hex_part.len() <= COOKIE_SIZE * 2 { return Err("KeyHash is too short.".to_string()); @@ -828,6 +836,30 @@ pub enum NeedleError { mod tests { use super::*; + /// A fid whose hex part carries multi-byte UTF-8 must be rejected, not + /// panic. `split` is a byte offset into `hex_part`; before the ASCII guard + /// `&hex_part[..split]` could land inside a character. `GET /3,ééééa` is + /// nine bytes, so it passes the length checks and splits at byte 1 — + /// halfway through the first `é`. Go's `ParseUint` just errors. + #[test] + fn parse_needle_id_cookie_rejects_non_ascii_instead_of_panicking() { + for s in ["ééééa", "ééééaaaaa", "0123456é9abc", "ééééa_1"] { + assert!( + parse_needle_id_cookie(s).is_err(), + "non-ASCII fid {:?} must be an error", + s + ); + } + } + + /// The ASCII guard must not change any accepted input. + #[test] + fn parse_needle_id_cookie_still_accepts_ascii_hex() { + let (id, cookie) = parse_needle_id_cookie("01637037d6").unwrap(); + assert_eq!(id, NeedleId(0x01)); + assert_eq!(cookie, Cookie(0x637037d6)); + } + #[test] fn test_parse_header() { let mut buf = [0u8; NEEDLE_HEADER_SIZE]; diff --git a/seaweed-volume/src/storage/needle/ttl.rs b/seaweed-volume/src/storage/needle/ttl.rs index 408169c53..f4a059560 100644 --- a/seaweed-volume/src/storage/needle/ttl.rs +++ b/seaweed-volume/src/storage/needle/ttl.rs @@ -80,6 +80,12 @@ impl TTL { if s.is_empty() { return Ok(TTL::EMPTY); } + // The unit is read as the last BYTE and the count as everything before + // it, so a trailing multi-byte character would split inside itself and + // panic. A TTL is digits plus a one-letter unit; reject the rest. + if !s.is_ascii() { + return Err(format!("invalid TTL {:?}: must be ASCII", s)); + } let last_byte = s.as_bytes()[s.len() - 1]; let (num_str, unit_byte) = if last_byte.is_ascii_digit() { // All digits — default to minutes (matching Go) @@ -240,6 +246,16 @@ impl fmt::Display for TTL { mod tests { use super::*; + /// `?ttl=5%C3%A9` must be an error, not a panic. The unit is taken as the + /// last *byte*, so a trailing multi-byte character made `&s[..s.len()-1]` + /// split inside it. + #[test] + fn ttl_read_rejects_non_ascii_instead_of_panicking() { + for s in ["5é", "é", "3🦀", "12é"] { + assert!(TTL::read(s).is_err(), "non-ASCII TTL {:?} must error", s); + } + } + #[test] fn test_ttl_parse() { let ttl = TTL::read("3m").unwrap(); diff --git a/seaweed-volume/src/storage/super_block.rs b/seaweed-volume/src/storage/super_block.rs index 033d1a929..7203d1384 100644 --- a/seaweed-volume/src/storage/super_block.rs +++ b/seaweed-volume/src/storage/super_block.rs @@ -221,6 +221,35 @@ mod tests { use super::*; use crate::storage::types::*; + /// Multi-byte input must be an error, not a panic: `to_digit` on the + /// leading characters rejects it before `chars[2]` is ever indexed. + #[test] + fn replica_placement_rejects_non_ascii_instead_of_panicking() { + for s in ["é", "0é", "é0", "🦀", "ééé"] { + assert!( + ReplicaPlacement::from_string(s).is_err(), + "non-ASCII replication {:?} must error", + s + ); + } + } + + /// The ASCII guard must not change any accepted input, including the + /// zero-padding shorthands. + #[test] + fn replica_placement_still_accepts_ascii_shorthands() { + assert_eq!( + ReplicaPlacement::from_string("1").unwrap(), + ReplicaPlacement::from_string("001").unwrap() + ); + assert_eq!( + ReplicaPlacement::from_string("01").unwrap(), + ReplicaPlacement::from_string("001").unwrap() + ); + let rp = ReplicaPlacement::from_string("010").unwrap(); + assert_eq!(rp.diff_rack_count, 1); + } + #[test] fn test_super_block_round_trip() { let sb = SuperBlock { diff --git a/seaweed-volume/tests/http_integration.rs b/seaweed-volume/tests/http_integration.rs index 825f52955..bcb4a8f86 100644 --- a/seaweed-volume/tests/http_integration.rs +++ b/seaweed-volume/tests/http_integration.rs @@ -1142,3 +1142,116 @@ async fn delete_on_ec_volume_succeeds_when_the_needles_shard_is_not_mounted() { "the delete must have been journalled, not just answered 202" ); } + +// ============================================================================ +// Hostile response-header override params must not panic the handler +// +// The `response-*` query params are attacker-controlled and were inserted with +// `parse().unwrap()`. `%0A` decodes to a newline, `HeaderValue::from_str` +// rejects it, and the unwrap panicked the connection task — unauthenticated. +// The override must simply be skipped. +// ============================================================================ + +#[tokio::test] +async fn hostile_response_header_overrides_are_skipped_not_panicked() { + let (state, _tmp) = test_state(); + let uri = "/1,01637037d6"; + + let app = build_admin_router(state.clone()); + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(uri) + .body(Body::from(b"payload".to_vec())) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::CREATED); + + // One request per override param, each carrying a raw newline. + for param in [ + "response-cache-control", + "response-content-encoding", + "response-expires", + "response-content-language", + "response-content-disposition", + "response-content-type", + ] { + let app = build_admin_router(state.clone()); + let response = app + .oneshot( + Request::builder() + .uri(format!("{}?{}=%0Aevil", uri, param)) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + response.status(), + StatusCode::OK, + "{} with a newline must be ignored, not panic", + param + ); + assert_eq!(body_bytes(response).await, b"payload".to_vec()); + } +} + +// ============================================================================ +// Non-ASCII in the fid and in ?ttl= must be rejected, not panic +// +// `parse_needle_id_cookie` split the hex by BYTE offset and `TTL::read` took +// the unit as the last BYTE, so a multi-byte character split inside itself. +// Both are reachable unauthenticated from the request line / query string. +// ============================================================================ + +#[tokio::test] +async fn non_ascii_fid_and_ttl_are_rejected_not_panicked() { + let (state, _tmp) = test_state(); + + // A fid whose hex part is multi-byte UTF-8. + let app = build_admin_router(state.clone()); + let response = app + .oneshot( + Request::builder() + .uri("/1,%C3%A9%C3%A9%C3%A9%C3%A9a") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert!( + response.status().is_client_error() || response.status().is_server_error(), + "non-ASCII fid must produce an error status, got {}", + response.status() + ); + + // A TTL whose unit character is multi-byte. The upload path does + // `TTL::read(..).ok()`, so *any* unparseable TTL is simply dropped and the + // write succeeds — the point here is that a non-ASCII one now takes that + // same road instead of panicking. Assert it matches an ASCII-invalid TTL + // rather than inventing a stricter contract than the handler has. + let mut statuses = Vec::new(); + // Distinct needle ids: reusing one id with a different cookie is a + // cookie-mismatch overwrite, which would mask what this test measures. + for (fid, ttl) in [("/1,03637037d7", "5%C3%A9"), ("/1,04637037d8", "5z")] { + let app = build_admin_router(state.clone()); + let response = app + .oneshot( + Request::builder() + .method("POST") + .uri(format!("{}?ttl={}", fid, ttl)) + .body(Body::from(b"x".to_vec())) + .unwrap(), + ) + .await + .unwrap(); + statuses.push(response.status()); + } + assert_eq!( + statuses[0], statuses[1], + "a non-ASCII ttl must behave like any other invalid ttl, not panic" + ); +}