From c915a11f59f9fc50c97ee326cf9433e2fbbf9dfe Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Tue, 14 Jul 2026 00:45:57 +0200 Subject: [PATCH] fix(cdp): truncate client-controlled log previews on char boundaries (#397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit server.rs logged previews of attacker-controlled strings with raw byte slicing: `&text[..text.len().min(200)]` for a malformed CDP frame (inside a `warn!`, which is active at the worker's default log level) and the same for an intercepted event JSON. `&s[..n]` panics when byte `n` falls inside a multi-byte UTF-8 character, so a single WebSocket frame that is invalid JSON, >=201 bytes, with a multi-byte char straddling byte 200 panics the CDP processor task — an unauthenticated one-frame crash. page.rs had the same pattern (behind --verbose debug logging) for evaluated JS expressions. Add truncate_on_char_boundary (str::floor_char_boundary is still unstable) and route all four sites through it. Regression test constructs the exact 'a'*199 + '€' input, asserts the old raw slice panics (via catch_unwind) and the helper truncates on the boundary instead. --- crates/obscura-browser/src/page.rs | 33 ++++++++++++++++++-- crates/obscura-cdp/src/server.rs | 4 +-- crates/obscura-cdp/src/util.rs | 48 ++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 5 deletions(-) diff --git a/crates/obscura-browser/src/page.rs b/crates/obscura-browser/src/page.rs index 7d8f5fe..e1f5538 100644 --- a/crates/obscura-browser/src/page.rs +++ b/crates/obscura-browser/src/page.rs @@ -67,6 +67,21 @@ fn hex_val(b: u8) -> Option { } } +/// Truncate `s` to at most `max` bytes without splitting a UTF-8 character. +/// `&s[..max]` panics if `max` lands inside a multi-byte char; the evaluated +/// expression logged below is caller-controlled, so slice it safely. +/// (`str::floor_char_boundary` would do this but is still unstable.) +fn truncate_on_char_boundary(s: &str, max: usize) -> &str { + if s.len() <= max { + return s; + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + &s[..end] +} + #[cfg(feature = "stealth")] use obscura_net::StealthHttpClient; @@ -1273,7 +1288,7 @@ impl Page { match js.evaluate_with_timeout(expression, timeout) { Ok(val) => val, Err(e) => { - tracing::debug!("JS eval error/timeout for '{}': {}", &expression[..expression.len().min(80)], e); + tracing::debug!("JS eval error/timeout for '{}': {}", truncate_on_char_boundary(expression, 80), e); serde_json::Value::Null } } @@ -1287,7 +1302,7 @@ impl Page { match js.evaluate(expression) { Ok(val) => val, Err(e) => { - tracing::debug!("JS eval error for '{}': {}", &expression[..expression.len().min(80)], e); + tracing::debug!("JS eval error for '{}': {}", truncate_on_char_boundary(expression, 80), e); serde_json::Value::Null } } @@ -1685,7 +1700,19 @@ fn url_matches_cdp_pattern(pattern: &str, url: &str) -> bool { #[cfg(test)] mod tests { - use super::url_matches_cdp_pattern; + use super::{truncate_on_char_boundary, url_matches_cdp_pattern}; + + #[test] + fn truncate_never_splits_a_multibyte_char() { + // A caller-supplied expression whose byte 80 lands inside a multi-byte + // char would make `&expression[..80]` panic; the helper truncates safely. + let s = format!("{}€tail", "a".repeat(79)); + assert!(!s.is_char_boundary(80), "setup: byte 80 splits the € char"); + let t = truncate_on_char_boundary(&s, 80); + assert!(s.starts_with(t)); + assert_eq!(t.len(), 79, "should stop right before the € char"); + assert_eq!(truncate_on_char_boundary("short", 80), "short"); + } #[test] fn url_matches_cdp_pattern_handles_wildcards_across_url_parts() { diff --git a/crates/obscura-cdp/src/server.rs b/crates/obscura-cdp/src/server.rs index fd112b6..c6fc646 100644 --- a/crates/obscura-cdp/src/server.rs +++ b/crates/obscura-cdp/src/server.rs @@ -717,7 +717,7 @@ async fn process_with_interception( "sessionId": session_for_events, }); let event_str = event_json.to_string(); - tracing::info!("INTERCEPTION event JSON: {}", &event_str[..event_str.len().min(300)]); + tracing::info!("INTERCEPTION event JSON: {}", crate::util::truncate_on_char_boundary(&event_str, 300)); let _ = reply_tx.send(event_str); intercepted_paused.insert(intercepted.request_id.clone(), intercepted.resolver); tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; @@ -829,7 +829,7 @@ async fn process_cdp_message( let req: CdpRequest = match serde_json::from_str(text) { Ok(r) => r, Err(e) => { - warn!("Invalid CDP: {}: {}", e, &text[..text.len().min(200)]); + warn!("Invalid CDP: {}: {}", e, crate::util::truncate_on_char_boundary(text, 200)); return; } }; diff --git a/crates/obscura-cdp/src/util.rs b/crates/obscura-cdp/src/util.rs index 10d940e..e08d26e 100644 --- a/crates/obscura-cdp/src/util.rs +++ b/crates/obscura-cdp/src/util.rs @@ -18,6 +18,24 @@ pub(crate) fn url_is_file_scheme(raw: &str) -> bool { }) } +/// Truncate `s` to at most `max` bytes, never splitting a UTF-8 character. +/// +/// `&s[..max]` panics when `max` lands inside a multi-byte character, and the +/// strings we truncate for log previews are attacker-controlled (raw WebSocket +/// frames, intercepted URLs). A single frame whose byte `max` straddles a +/// multi-byte char would otherwise panic the CDP processor task +/// (`str::floor_char_boundary` would do this but is still unstable). +pub(crate) fn truncate_on_char_boundary(s: &str, max: usize) -> &str { + if s.len() <= max { + return s; + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + &s[..end] +} + #[cfg(test)] mod tests { use super::*; @@ -59,4 +77,34 @@ mod tests { assert!(!url_is_file_scheme("notfile:///x")); assert!(!url_is_file_scheme("http://file/")); } + + #[test] + fn truncate_never_splits_a_multibyte_char() { + // 199 ASCII bytes + '€' (3 bytes, occupying indices 199..=201): byte 200 + // falls inside the '€'. This is exactly the shape of a malformed CDP + // frame that would reach the `warn!("Invalid CDP: ...")` preview. + let s = format!("{}€tail", "a".repeat(199)); + assert!(!s.is_char_boundary(200), "setup: byte 200 splits the € char"); + + // The old logging code did `&s[..s.len().min(200)]`, which panics here — + // a single crafted frame would take down the CDP processor task. + let naive = std::panic::catch_unwind(|| { + let _ = &s[..s.len().min(200)]; + }); + assert!(naive.is_err(), "raw byte slice at a non-char-boundary must panic"); + + // The helper truncates on the boundary before the € instead. + let safe = truncate_on_char_boundary(&s, 200); + assert!(s.starts_with(safe)); + assert!(safe.len() <= 200); + assert_eq!(safe.len(), 199, "should stop right before the € char"); + } + + #[test] + fn truncate_returns_whole_string_when_short() { + assert_eq!(truncate_on_char_boundary("hi", 200), "hi"); + assert_eq!(truncate_on_char_boundary("", 10), ""); + // Exact-length and exact-boundary cases are returned unchanged. + assert_eq!(truncate_on_char_boundary("abc", 3), "abc"); + } }