diff --git a/src/parsing/ruby_hash.rs b/src/parsing/ruby_hash.rs index 4ed63afac..712825a9c 100644 --- a/src/parsing/ruby_hash.rs +++ b/src/parsing/ruby_hash.rs @@ -12,8 +12,10 @@ use nom::{ }; use std::num::ParseIntError; +const MAX_RUBY_HASH_DEPTH: usize = 128; + pub(crate) fn parse_ruby_hash(input: &str) -> ExpressionResult { - let result = parse_hash(input) + let result = parse_hash(0)(input) .map_err(|err| match err { nom::Err::Error(err) | nom::Err::Failure(err) => { // Create a descriptive error message if possible. @@ -139,64 +141,84 @@ fn parse_key<'a, E: HashParseError<&'a str>>(input: &'a str) -> IResult<&'a str, ))(input) } -fn parse_array<'a, E: HashParseError<&'a str>>(input: &'a str) -> IResult<&'a str, Value, E> { - context( - "array", - map( - preceded( - char('['), - cut(terminated( - separated_list0(preceded(sp, char(',')), parse_value), - preceded(sp, char(']')), - )), +fn parse_array<'a, E: HashParseError<&'a str>>( + depth: usize, +) -> impl FnMut(&'a str) -> IResult<&'a str, Value, E> { + move |input| { + context( + "array", + map( + preceded( + char('['), + cut(terminated( + separated_list0(preceded(sp, char(',')), parse_value(depth + 1)), + preceded(sp, char(']')), + )), + ), + Value::Array, ), - Value::Array, - ), - )(input) + )(input) + } } fn parse_key_value<'a, E: HashParseError<&'a str>>( - input: &'a str, -) -> IResult<&'a str, (KeyString, Value), E> { - separated_pair( - preceded(sp, parse_key), - cut(preceded(sp, alt((tag(":"), tag("=>"))))), - parse_value, - )(input) + depth: usize, +) -> impl FnMut(&'a str) -> IResult<&'a str, (KeyString, Value), E> { + move |input| { + separated_pair( + preceded(sp, parse_key), + cut(preceded(sp, alt((tag(":"), tag("=>"))))), + parse_value(depth), + )(input) + } } -fn parse_hash<'a, E: HashParseError<&'a str>>(input: &'a str) -> IResult<&'a str, Value, E> { - context( - "map", - map( - preceded( - char('{'), - cut(terminated( - map( - separated_list0(preceded(sp, char(',')), parse_key_value), - |tuple_vec| tuple_vec.into_iter().collect(), - ), - preceded(sp, char('}')), - )), +fn parse_hash<'a, E: HashParseError<&'a str>>( + depth: usize, +) -> impl FnMut(&'a str) -> IResult<&'a str, Value, E> { + move |input| { + context( + "map", + map( + preceded( + char('{'), + cut(terminated( + map( + separated_list0(preceded(sp, char(',')), parse_key_value(depth + 1)), + |tuple_vec| tuple_vec.into_iter().collect(), + ), + preceded(sp, char('}')), + )), + ), + Value::Object, ), - Value::Object, - ), - )(input) + )(input) + } } -fn parse_value<'a, E: HashParseError<&'a str>>(input: &'a str) -> IResult<&'a str, Value, E> { - preceded( - sp, - alt(( - parse_nil, - parse_hash, - parse_array, - map(parse_colon_key, Value::from), - map(parse_bytes, Value::Bytes), - map(double, |value| Value::Float(NotNan::new(value).unwrap())), - map(parse_boolean, Value::Boolean), - )), - )(input) +fn parse_value<'a, E: HashParseError<&'a str>>( + depth: usize, +) -> impl FnMut(&'a str) -> IResult<&'a str, Value, E> { + move |input| { + if depth > MAX_RUBY_HASH_DEPTH { + return Err(nom::Err::Failure(E::from_error_kind( + input, + nom::error::ErrorKind::TooLarge, + ))); + } + preceded( + sp, + alt(( + parse_nil, + parse_hash(depth), + parse_array(depth), + map(parse_colon_key, Value::from), + map(parse_bytes, Value::Bytes), + map(double, |value| Value::Float(NotNan::new(value).unwrap())), + map(parse_boolean, Value::Boolean), + )), + )(input) + } } #[cfg(test)] @@ -339,4 +361,16 @@ mod tests { fn test_non_hash() { assert!(parse_ruby_hash(r#""hello world""#).is_err()); } + + #[test] + fn test_depth_limit_obe10741() { + // OBE-10741: deeply nested ruby hash from attacker-controlled input must be rejected. + // Without the fix, parsing 200 levels of nesting recurses through parse_value/parse_hash + // and can cause stack overflow or DoS. + let open: String = "{:k => ".repeat(200); + let close: String = "}".repeat(200); + let input = format!("{open}1{close}"); + let result = parse_ruby_hash(&input); + assert!(result.is_err(), "hash with 200 nesting levels must be rejected (OBE-10741)"); + } } diff --git a/src/stdlib/parse_xml.rs b/src/stdlib/parse_xml.rs index 012831283..9ab850d9a 100644 --- a/src/stdlib/parse_xml.rs +++ b/src/stdlib/parse_xml.rs @@ -587,4 +587,20 @@ mod tests { assert!(object2.known().is_empty()); assert!(object2.unknown_kind().is_any()); } + + #[test] + fn test_xml_depth_limit_obe10742() { + // OBE-10742: deeply nested XML from attacker-controlled input must be rejected. + // Without the fix, process_node recurses for every element level and can stack overflow. + let depth = 200usize; + let open: String = (0..depth).map(|i| format!("")).collect(); + let close: String = (0..depth).rev().map(|i| format!("")).collect(); + let xml = format!("{open}hi{close}"); + let value = Value::Bytes(xml.into()); + let options = crate::parsing::xml::ParseOptions::default(); + let result = crate::parsing::xml::parse_xml(value, options); + assert!(result.is_err(), "XML with {depth} nesting levels must be rejected (OBE-10742)"); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("nesting limit"), "error must mention nesting limit: {msg}"); + } } diff --git a/src/stdlib/remove.rs b/src/stdlib/remove.rs index 50a004194..77b89e29c 100644 --- a/src/stdlib/remove.rs +++ b/src/stdlib/remove.rs @@ -1,9 +1,12 @@ use crate::compiler::prelude::*; use crate::path::{OwnedSegment, OwnedValuePath}; +use crate::stdlib::util::check_path_segment_limit; fn remove(path: Value, compact: Value, mut value: Value) -> Resolved { let path = match path { Value::Array(path) => { + check_path_segment_limit(path.len())?; + let mut lookup = OwnedValuePath::root(); for segment in path { @@ -212,4 +215,15 @@ mod tests { tdef: TypeDef::object(Collection::any()).fallible(), } ]; + + #[test] + fn test_path_length_limit_obe10739() { + // OBE-10739: attacker-controlled path with > MAX_PATH_SEGMENTS segments must be rejected. + // Without the fix this returns Ok and silently accepts arbitrary-depth traversal. + let segments: Vec = (0..200).map(|i| Value::Bytes(format!("k{i}").into())).collect(); + let result = remove(Value::Array(segments), Value::Boolean(false), Value::Object(ObjectMap::new())); + assert!(result.is_err(), "path with 200 segments must be rejected (OBE-10739)"); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("200 segments"), "error should name the count: {msg}"); + } } diff --git a/src/stdlib/set.rs b/src/stdlib/set.rs index 829353ec0..267fd3a3c 100644 --- a/src/stdlib/set.rs +++ b/src/stdlib/set.rs @@ -1,9 +1,12 @@ use crate::compiler::prelude::*; use crate::path::{OwnedSegment, OwnedValuePath}; +use crate::stdlib::util::check_path_segment_limit; fn set(path: Value, mut value: Value, data: Value) -> Resolved { let path = match path { Value::Array(segments) => { + check_path_segment_limit(segments.len())?; + let mut insert = OwnedValuePath::root(); for segment in segments { @@ -186,4 +189,15 @@ mod tests { tdef: TypeDef::object(Collection::any()).fallible(), } ]; + + #[test] + fn test_path_length_limit_obe10739() { + // OBE-10739: attacker-controlled path with > MAX_PATH_SEGMENTS segments must be rejected. + // Without the fix this returns Ok and accepts arbitrary-depth writes. + let segments: Vec = (0..200).map(|i| Value::Bytes(format!("k{i}").into())).collect(); + let result = set(Value::Array(segments), Value::Object(ObjectMap::new()), Value::Null); + assert!(result.is_err(), "path with 200 segments must be rejected (OBE-10739)"); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("200 segments"), "error should name the count: {msg}"); + } } diff --git a/src/stdlib/unflatten.rs b/src/stdlib/unflatten.rs index be082246f..2e251273e 100644 --- a/src/stdlib/unflatten.rs +++ b/src/stdlib/unflatten.rs @@ -4,25 +4,32 @@ use crate::compiler::prelude::*; static DEFAULT_SEPARATOR: &str = "."; +const MAX_UNFLATTEN_DEPTH: usize = 128; + fn unflatten(value: Value, separator: Value, recursive: Value) -> Resolved { let separator = separator.try_bytes_utf8_lossy()?.into_owned(); let recursive = recursive.try_boolean()?; let map = value.try_object()?; - Ok(do_unflatten(map.into(), &separator, recursive)) + Ok(do_unflatten(map.into(), &separator, recursive, 0)) } -fn do_unflatten(value: Value, separator: &str, recursive: bool) -> Value { +fn do_unflatten(value: Value, separator: &str, recursive: bool, depth: usize) -> Value { match value { - Value::Object(map) => do_unflatten_entries(map, separator, recursive).into(), + Value::Object(map) => do_unflatten_entries(map, separator, recursive, depth).into(), // Note that objects inside arrays are not unflattened _ => value, } } -fn do_unflatten_entries(entries: I, separator: &str, recursive: bool) -> ObjectMap +fn do_unflatten_entries(entries: I, separator: &str, recursive: bool, depth: usize) -> ObjectMap where I: IntoIterator, { + if depth >= MAX_UNFLATTEN_DEPTH { + // Stop splitting keys at the depth limit; collect remainder as literal keys. + return entries.into_iter().collect(); + } + let grouped = entries .into_iter() .map(|(key, value)| { @@ -41,14 +48,15 @@ where match values.pop().expect("exactly one element") { (_, None, value) => { let value = if recursive { - do_unflatten(value, separator, recursive) + do_unflatten(value, separator, recursive, depth + 1) } else { value }; return (key, value); } (_, Some(rest), value) => { - let result = do_unflatten_entry((rest, value), separator, recursive); + let result = + do_unflatten_entry((rest, value), separator, recursive, depth + 1); return (key, result); } } @@ -72,7 +80,7 @@ where rest.map(|rest| (rest, value)) }) .collect::>(); - let result = do_unflatten_entries(new_entries, separator, recursive); + let result = do_unflatten_entries(new_entries, separator, recursive, depth + 1); (key, result.into()) }) .collect() @@ -81,11 +89,22 @@ where // Optimization in the case we have to flatten objects like // { "a.b.c.d": 1 } // and avoid doing recursive calls to `do_unflatten_entries` with a single entry every time -fn do_unflatten_entry(entry: (KeyString, Value), separator: &str, recursive: bool) -> Value { +fn do_unflatten_entry( + entry: (KeyString, Value), + separator: &str, + recursive: bool, + depth: usize, +) -> Value { let (key, value) = entry; - let keys = key.split(separator).map(Into::into).collect::>(); + // splitn caps the segment count at the depth budget remaining after `depth`; the final + // piece retains any remaining separator characters as a literal key (OBE-10744). + let remaining = MAX_UNFLATTEN_DEPTH.saturating_sub(depth); + let keys: Vec = key + .splitn(remaining + 1, separator) + .map(Into::into) + .collect(); let mut result = if recursive { - do_unflatten(value, separator, recursive) + do_unflatten(value, separator, recursive, depth + keys.len()) } else { value }; @@ -413,4 +432,66 @@ mod test { tdef: TypeDef::object(Collection::any()), } ]; + + #[test] + fn test_depth_limit_obe10744() { + // OBE-10744: unflatten with deeply grouped entries must terminate at MAX_UNFLATTEN_DEPTH. + // Build two entries sharing a 200-level common prefix ("k0.k1...k199.x" and "...y"). + // Without the fix, do_unflatten_entries recurses 200 times and can stack overflow. + let prefix: Vec = (0..200).map(|i| format!("k{i}")).collect(); + let key1: KeyString = [prefix.join("."), "x".into()].join(".").into(); + let key2: KeyString = [prefix.join("."), "y".into()].join(".").into(); + let entries: Vec<(KeyString, Value)> = vec![(key1, Value::Integer(1)), (key2, Value::Integer(2))]; + // Must return without panicking and depth must be bounded. + let result = do_unflatten_entries(entries, ".", false, 0); + // Walk result to verify nesting depth is bounded. + let mut depth = 0usize; + let mut cur = Value::Object(result); + loop { + match cur { + Value::Object(ref m) if m.len() == 1 => { + cur = m.values().next().unwrap().clone(); + depth += 1; + assert!(depth <= MAX_UNFLATTEN_DEPTH + 1, "nesting depth {depth} exceeds cap (OBE-10744)"); + } + _ => break, + } + } + } + + #[test] + fn test_unflatten_recursive_forwards_depth_obe10744() { + // OBE-10744 (reset-to-0 regression): `recursive: true` re-enters `do_unflatten` on every + // pre-existing nested `Value::Object`, not just on split dotted keys. If that re-entry + // resets depth to 0 instead of forwarding the accumulated depth, the cap never engages + // for plain nested objects, and a deeply pre-nested value still drives unbounded + // native-stack recursion. + // + // Build MAX_UNFLATTEN_DEPTH + 10 levels of single-key nested objects, with a dotted leaf + // key at the bottom. With depth correctly forwarded, recursion must stop once the cap is + // reached, leaving the leaf-most dotted key un-split. + let total_levels = MAX_UNFLATTEN_DEPTH + 10; + let mut value = value!({ "leaf.key": 1 }); + for i in (0..total_levels).rev() { + value = Value::Object(ObjectMap::from_iter([(format!("l{i}").into(), value)])); + } + + let result = do_unflatten(value, ".", true, 0); + + let mut cur = &result; + for i in 0..total_levels { + let Value::Object(map) = cur else { + panic!("expected object at level {i}, got {cur:?}") + }; + cur = map.values().next().expect("expected exactly one entry"); + } + let Value::Object(leaf_map) = cur else { + panic!("expected leaf object, got {cur:?}") + }; + assert!( + leaf_map.contains_key("leaf.key"), + "past the depth cap, dotted keys must remain un-split — recursion should have \ + stopped instead of continuing to the leaf: {leaf_map:?}" + ); + } } diff --git a/src/stdlib/util.rs b/src/stdlib/util.rs index be351a835..66aa96c59 100644 --- a/src/stdlib/util.rs +++ b/src/stdlib/util.rs @@ -1,4 +1,4 @@ -use crate::compiler::{Context, Expression, Resolved, TypeState}; +use crate::compiler::{Context, Expression, ExpressionError, Resolved, TypeState}; use crate::value::{KeyString, ObjectMap, Value}; /// Maximum number of bytes any `decode_*` decompression function will produce @@ -12,6 +12,21 @@ pub(crate) const DEFAULT_DECOMPRESS_LIMIT: u64 = 64 * 1024 * 1024; // 64 MiB /// Error returned when a decoder's output would exceed [`DEFAULT_DECOMPRESS_LIMIT`]. pub(crate) const DECOMPRESS_LIMIT_ERROR: &str = "decompressed output exceeds size limit"; +/// Maximum number of segments a `set!`/`remove!` path array may contain (OBE-10739). +/// +/// Both functions build an `OwnedValuePath` by recursing once per segment through the +/// generic `crud` walker, so an attacker-controlled path with no length limit can drive +/// unbounded native-stack recursion. +pub(crate) const MAX_PATH_SEGMENTS: usize = 128; + +/// Rejects a `set!`/`remove!` path array with more than [`MAX_PATH_SEGMENTS`] segments. +pub(crate) fn check_path_segment_limit(len: usize) -> Result<(), ExpressionError> { + if len > MAX_PATH_SEGMENTS { + return Err(format!("path has {len} segments, max is {MAX_PATH_SEGMENTS}").into()); + } + Ok(()) +} + /// Rounds the given number to the given precision. /// Takes a function parameter so the exact rounding function (ceil, floor or round) /// can be specified.