From 953a6b3b01d3ea2e7490ed8ea9cfb4c803d32ac8 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 22 Oct 2026 17:57:09 -0400 Subject: [PATCH] security: bound Lisp callbacks, native comparisons, and local file reads (CWE-400, CWE-407) (#1842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * security: bound Lisp callbacks, native comparisons, and local file reads (CWE-400, CWE-407) The command and scripting engine had several unbounded paths a supplied LISP routine or a selected path could exploit to monopolise the thread or exhaust memory: - `call_sync` ran synchronous callbacks (vl-sort comparators, vl-remove-if predicates, …) with `u64::MAX` fuel, so a constant-memory infinite loop never yielded and bypassed the blocking execution cap. It now drives the callback in bounded slices and aborts with an error past a hard step cap (row 2). - `wcmatch`/`glob` explored `*` placements with only a recursion-depth guard, and `equal` re-traversed shared cons subtrees. Both now charge every step against a shared work budget and stop when it is exhausted (row 33). - `setvar` converted a LISP value to JSON unboundedly, so a shared cons DAG expanded into gigabytes of owned arrays. Conversion is now depth- and element-bounded and rejects over-budget values with a LISP error (row 39). - OPEN/LOAD/APPLOAD (`read_text_file`), SCRIPT, and native open (`read_file`, `run_open`) buffered a path with `std::fs::read`, so an endless device exhausted memory before any size check. Reads now go through a new `capped_read` helper that rejects non-regular files and caps bytes (rows 21, 34). New tests cover the callback loop, the oversized setvar structure, and the capped reader. Addresses findings rows 2, 21, 33, 34, 39. * fix: bound native Lisp scans and nonblocking file reads * fix: enforce callback fuel before each execution slice --------- Co-authored-by: skylartaylor <26588250+skylartaylor@users.noreply.github.com> --- Cargo.lock | 1 + crates/engine/Cargo.toml | 3 + crates/engine/src/capped_read.rs | 81 ++++++++++++++++ crates/engine/src/cmd/file.rs | 18 +++- crates/engine/src/lib.rs | 2 + crates/engine/src/lisp/builtins.rs | 146 ++++++++++++++++++++++++----- crates/engine/src/lisp/drawing.rs | 32 +++++-- crates/engine/src/lisp/eval.rs | 30 +++++- crates/engine/src/lisp/mod.rs | 4 +- crates/engine/src/lisp/tests.rs | 58 ++++++++++++ crates/engine/src/lisp/value.rs | 19 +++- 11 files changed, 350 insertions(+), 44 deletions(-) create mode 100644 crates/engine/src/capped_read.rs diff --git a/Cargo.lock b/Cargo.lock index 0e7f5097..6ed70601 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -637,6 +637,7 @@ dependencies = [ "cadcraft-io", "cadcraft-render", "craft-docpath", + "libc", "log", "rstar", "serde", diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index a19da90f..04be2919 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -24,6 +24,9 @@ log = { workspace = true } # R-tree spatial index for picking, window selection and object snaps (MIT OR Apache-2.0). rstar = { version = "0.12", default-features = false } +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [dev-dependencies] # Real DXF files for the external reference tests (io is a lower layer). cadcraft-io = { workspace = true } diff --git a/crates/engine/src/capped_read.rs b/crates/engine/src/capped_read.rs new file mode 100644 index 00000000..d82344bb --- /dev/null +++ b/crates/engine/src/capped_read.rs @@ -0,0 +1,81 @@ +//! Bounded reads of local files. +//! +//! Reading a path with [`std::fs::read`] buffers until EOF, so a path naming an endless readable +//! device (for example `/dev/urandom` or a FIFO) exhausts process memory before any size check +//! runs (CWE-400). [`read_capped`] opens the path once, refuses anything that is not a regular +//! file, and reads through a reader limited to `max + 1` bytes so oversized input is rejected +//! before it is fully buffered. + +#![cfg(not(target_arch = "wasm32"))] + +use std::fs::OpenOptions; +use std::io::{self, Read}; +use std::path::Path; + +/// Read at most `max` bytes from the regular file at `path`. +/// +/// Returns an error if the path is not a regular file, cannot be opened, or holds more than `max` +/// bytes. At most `max + 1` bytes are ever buffered, so an endless device cannot exhaust memory. +pub fn read_capped(path: &Path, max: usize) -> io::Result> { + let mut options = OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NONBLOCK); + } + let file = options.open(path)?; + let meta = file.metadata()?; + if !meta.is_file() { + return Err(io::Error::new(io::ErrorKind::InvalidInput, "not a regular file")); + } + // Read one byte past the limit so we can tell "exactly max" from "too big" without trusting + // the metadata length (which a device may misreport). + let mut buf = Vec::new(); + let cap = (max as u64).saturating_add(1); + let read = file.take(cap).read_to_end(&mut buf)?; + if read as u64 > max as u64 { + return Err(io::Error::new(io::ErrorKind::InvalidData, format!("file is larger than the {max}-byte limit"))); + } + Ok(buf) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reads_within_limit_and_rejects_oversize() { + let dir = std::env::temp_dir().join(format!("cadcraft-cr-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let p = dir.join("f.bin"); + std::fs::write(&p, vec![7u8; 100]).unwrap(); + assert_eq!(read_capped(&p, 100).unwrap().len(), 100); + assert_eq!(read_capped(&p, 200).unwrap().len(), 100); + assert!(read_capped(&p, 99).is_err()); + assert_eq!(read_capped(&p, usize::MAX).unwrap().len(), 100); + std::fs::remove_dir_all(&dir).ok(); + } + + #[cfg(unix)] + #[test] + fn rejects_non_regular_file() { + // /dev/zero is an endless readable device; it must be rejected, not buffered. + let p = Path::new("/dev/zero"); + if p.exists() { + assert!(read_capped(p, 8 << 20).is_err()); + } + } + #[cfg(unix)] + #[test] + fn rejects_fifo_without_a_writer() { + let dir = std::env::temp_dir().join(format!("cadcraft-cr-fifo-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let fifo = dir.join("pipe"); + assert!(std::process::Command::new("mkfifo").arg(&fifo).status().unwrap().success()); + let started = std::time::Instant::now(); + assert_eq!(read_capped(&fifo, 1024).unwrap_err().kind(), io::ErrorKind::InvalidInput); + assert!(started.elapsed() < std::time::Duration::from_secs(2)); + std::fs::remove_dir_all(dir).unwrap(); + } +} diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 06284069..96b86f4e 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -19,6 +19,15 @@ pub use pgp::{MAX_ALIASES, alias_name, command_id}; pub(crate) use pgp::{specs as alias_specs, user_alias}; pub use template::{qnew_template, set_qnew_template}; +/// Largest drawing accepted from a local path, matching the per-file xref limit. A path naming an +/// endless device must be rejected before it exhausts memory (CWE-400), so native opens read +/// through [`crate::capped_read`] rather than buffering to EOF. +#[cfg(not(target_arch = "wasm32"))] +const MAX_DRAWING_BYTES: usize = 512 << 20; +/// Largest `.scr` script accepted from a local path. +#[cfg(not(target_arch = "wasm32"))] +const MAX_SCRIPT_BYTES: usize = 64 << 20; + /// Plot a space to PDF bytes with JSON options (`{paper?, landscape?, fit?, lineweights?, …}`). pub type PlotHook = fn(&Drawing, &cadcraft_doc::Space, &Value) -> std::result::Result, String>; @@ -192,7 +201,8 @@ fn run_script(s: &mut Session, p: &Value) -> Result { let path = _path; #[cfg(not(target_arch = "wasm32"))] { - let bytes = std::fs::read(path).map_err(|e| bad("script", format!("{path}: {e}")))?; + let bytes = + crate::capped_read::read_capped(std::path::Path::new(path), MAX_SCRIPT_BYTES).map_err(|e| bad("script", format!("{path}: {e}")))?; String::from_utf8_lossy(&bytes).into_owned() } #[cfg(target_arch = "wasm32")] @@ -265,7 +275,8 @@ pub fn read_file(path: &str) -> std::result::Result { #[cfg(not(target_arch = "wasm32"))] { let r = std::panic::catch_unwind(|| { - let bytes = std::fs::read(path).map_err(|e| bad("open", format!("{path}: {e}")))?; + let bytes = + crate::capped_read::read_capped(std::path::Path::new(path), MAX_DRAWING_BYTES).map_err(|e| bad("open", format!("{path}: {e}")))?; read_bytes(&bytes, path, Some(path.to_string())) }); match r { @@ -332,7 +343,8 @@ fn run_open(s: &mut Session, p: &Value) -> Result { let path = _path; #[cfg(not(target_arch = "wasm32"))] { - let bytes = std::fs::read(path).map_err(|e| bad("open", format!("{path}: {e}")))?; + let bytes = + crate::capped_read::read_capped(std::path::Path::new(path), MAX_DRAWING_BYTES).map_err(|e| bad("open", format!("{path}: {e}")))?; let i = open_bytes(s, &bytes, path, Some(path.to_string()))?; super::blocks::xref::after_open(s, i); return Ok(opened(s, i, true)); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a4ac9aaf..1bf8e005 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -14,6 +14,8 @@ pub mod atomic_write; pub mod audit; pub mod autosave; pub mod blockedit; +#[cfg(not(target_arch = "wasm32"))] +pub mod capped_read; pub mod cmd; pub mod doc_path; mod finite; diff --git a/crates/engine/src/lisp/builtins.rs b/crates/engine/src/lisp/builtins.rs index 73aea363..9ef90f3b 100644 --- a/crates/engine/src/lisp/builtins.rs +++ b/crates/engine/src/lisp/builtins.rs @@ -679,19 +679,44 @@ fn wcmatch(a: &[Value]) -> R { arity(a, 2, 2)?; let (s, pat) = (string(&a[0])?, string(&a[1])?); let text: Vec = s.chars().collect(); - Ok(Value::bool(split_patterns(&pat).iter().any(|p| match_pattern(&text, p)))) + // Native wildcard matching runs as a single interpreter step, so its internal `*` backtracking + // must not explore exponentially many placements unchecked (CWE-400). Charge every glob step + // against a shared budget and fail the call if it is exhausted. + let mut budget: u64 = WCMATCH_BUDGET; + for p in split_patterns(&pat, &mut budget).map_err(|()| LispError::new("wcmatch: pattern too complex"))? { + match match_pattern(&text, &p, &mut budget) { + Ok(true) => return Ok(Value::bool(true)), + Ok(false) => {} + Err(()) => return Err(LispError::new("wcmatch: pattern too complex")), + } + } + Ok(Value::bool(false)) } -fn split_patterns(pat: &str) -> Vec> { +/// Shared step budget for one `wcmatch` call's wildcard matching. Large enough for the patterns +/// AutoLISP routines legitimately use (including the pathological-but-bounded cases the tests +/// cover), but finite so truly exponential `*`-heavy patterns are rejected rather than hanging. +const WCMATCH_BUDGET: u64 = 200_000_000; + +fn charge_match(budget: &mut u64) -> Result<(), ()> { + *budget = budget.checked_sub(1).ok_or(())?; + Ok(()) +} + +fn split_patterns(pat: &str, budget: &mut u64) -> Result>, ()> { let mut out = vec![Vec::new()]; let mut chars = pat.chars().peekable(); let mut in_set = false; while let Some(c) = chars.next() { + charge_match(budget)?; match c { '`' => { if let Some(cur) = out.last_mut() { cur.push('`'); - cur.extend(chars.next()); + if let Some(escaped) = chars.next() { + charge_match(budget)?; + cur.push(escaped); + } } } '[' => { @@ -714,41 +739,93 @@ fn split_patterns(pat: &str) -> Vec> { } } } - out + Ok(out) } -fn match_pattern(text: &[char], pat: &[char]) -> bool { +fn match_pattern(text: &[char], pat: &[char], budget: &mut u64) -> Result { if let Some(('~', rest)) = pat.split_first().map(|(c, r)| (*c, r)) && !rest.is_empty() { - return !glob(text, rest, 0); + return glob(text, rest, 0, budget).map(|m| !m); } - glob(text, pat, 0) + glob(text, pat, 0, budget) } -/// Iterative-enough glob with `*` backtracking; `depth` bounds the recursion on `*`. -fn glob(text: &[char], pat: &[char], depth: usize) -> bool { +/// Iterative-enough glob with `*` backtracking; `depth` bounds the recursion on `*`. Every step is +/// charged against `budget`; `Err(())` means the shared budget ran out (treated as too complex). +fn glob(text: &[char], pat: &[char], depth: usize, budget: &mut u64) -> Result { + glob_cached(text, pat, depth, budget, &mut std::collections::HashSet::new()) +} + +fn glob_cached( + text: &[char], + pat: &[char], + depth: usize, + budget: &mut u64, + failed: &mut std::collections::HashSet<(usize, usize, usize)>, +) -> Result { + charge_match(budget)?; + let key = (text.len(), pat.len(), depth); + if failed.contains(&key) { + return Ok(false); + } + let matched = glob_inner(text, pat, depth, budget, failed)?; + // Avoid revisiting the same failed suffix exponentially. Bound the memo table independently + // of the step limit so a very large pattern cannot turn the optimization into an allocation bomb. + if !matched && failed.len() < 100_000 { + failed.insert(key); + } + Ok(matched) +} + +fn glob_inner( + text: &[char], + pat: &[char], + depth: usize, + budget: &mut u64, + failed: &mut std::collections::HashSet<(usize, usize, usize)>, +) -> Result { + if *budget == 0 { + return Err(()); + } + *budget -= 1; if depth > 64 { - return false; + return Ok(false); } let (mut t, mut p) = (0usize, 0usize); while p < pat.len() { + charge_match(budget)?; match pat[p] { '*' => { let rest = &pat[p + 1..]; - return (t..=text.len()).any(|i| glob(&text[i..], rest, depth + 1)); + for i in t..=text.len() { + if glob_cached(&text[i..], rest, depth + 1, budget, failed)? { + return Ok(true); + } + } + return Ok(false); } '`' => { - let Some(&lit) = pat.get(p + 1) else { return false }; + let Some(&lit) = pat.get(p + 1) else { return Ok(false) }; if text.get(t) != Some(&lit) { - return false; + return Ok(false); } t += 1; p += 2; } '[' => { - let Some(close) = pat[p..].iter().position(|c| *c == ']').map(|i| p + i) else { return false }; - let Some(&c) = text.get(t) else { return false }; + let mut close = p; + while let Some(c) = pat.get(close) { + charge_match(budget)?; + if *c == ']' { + break; + } + close += 1; + } + if close == pat.len() { + return Ok(false); + } + let Some(&c) = text.get(t) else { return Ok(false) }; let set = &pat[p + 1..close]; let (neg, set) = match set.split_first() { Some(('~', r)) if !r.is_empty() => (true, r), @@ -757,6 +834,7 @@ fn glob(text: &[char], pat: &[char], depth: usize) -> bool { let mut hit = false; let mut i = 0; while i < set.len() { + charge_match(budget)?; if set.get(i + 1) == Some(&'-') && i + 2 < set.len() { if set[i] <= c && c <= set[i + 2] { hit = true; @@ -770,13 +848,13 @@ fn glob(text: &[char], pat: &[char], depth: usize) -> bool { } } if hit == neg { - return false; + return Ok(false); } t += 1; p = close + 1; } pc => { - let Some(&c) = text.get(t) else { return false }; + let Some(&c) = text.get(t) else { return Ok(false) }; let ok = match pc { '#' => c.is_ascii_digit(), '@' => c.is_alphabetic(), @@ -785,14 +863,14 @@ fn glob(text: &[char], pat: &[char], depth: usize) -> bool { other => other == c, }; if !ok { - return false; + return Ok(false); } t += 1; p += 1; } } } - t == text.len() + Ok(t == text.len()) } // ---------------- geometry ---------------- @@ -984,11 +1062,10 @@ fn findfile(a: &[Value]) -> R { #[cfg(not(target_arch = "wasm32"))] pub fn read_text_file(path: &str) -> Option { - let bytes = std::fs::read(path).ok()?; - // Cap what a routine can pull into memory. - if bytes.len() > 64 * 1024 * 1024 { - return None; - } + // Read through a byte-limited, regular-file-only reader so a path naming an endless device + // cannot exhaust memory before the 64 MiB cap is enforced (CWE-400). + const MAX: usize = 64 * 1024 * 1024; + let bytes = crate::capped_read::read_capped(std::path::Path::new(path), MAX).ok()?; Some(String::from_utf8_lossy(&bytes).into_owned()) } #[cfg(target_arch = "wasm32")] @@ -1503,5 +1580,24 @@ pub fn real(r: f64) -> String { /// `wcmatch` for other callers (`ssget` filters). pub fn wild_match(text: &str, pattern: &str) -> bool { let t: Vec = text.chars().collect(); - split_patterns(pattern).iter().any(|p| match_pattern(&t, p)) + let mut budget: u64 = WCMATCH_BUDGET; + split_patterns(pattern, &mut budget).is_ok_and(|patterns| patterns.iter().any(|p| match_pattern(&t, p, &mut budget) == Ok(true))) +} + +#[cfg(test)] +mod wildcard_budget_tests { + use super::*; + + #[test] + fn all_native_scans_consume_fuel() { + assert!(split_patterns("abcdefgh", &mut 4).is_err()); + assert!(split_patterns("`a`b`c", &mut 4).is_err()); + let text: Vec<_> = "abcdefgh".chars().collect(); + assert!(glob(&text, &text, 0, &mut 4).is_err()); + let pattern: Vec<_> = "[abcdefgh]".chars().collect(); + assert!(glob(&['!'], &pattern, 0, &mut 6).is_err()); + // Enough for finding the closing bracket, but not scanning the entire set again. + assert!(glob(&['!'], &pattern, 0, &mut 14).is_err()); + assert_eq!(glob(&['a'], &pattern, 0, &mut 100), Ok(true)); + } } diff --git a/crates/engine/src/lisp/drawing.rs b/crates/engine/src/lisp/drawing.rs index 573eaa24..043b611a 100644 --- a/crates/engine/src/lisp/drawing.rs +++ b/crates/engine/src/lisp/drawing.rs @@ -151,16 +151,32 @@ pub fn from_json(v: &serde_json::Value) -> Value { } } -/// A LISP value as JSON (for `setvar`). -pub fn to_json(v: &Value) -> serde_json::Value { - match v { +/// A LISP value as JSON (for `setvar`), bounded so a shared cons structure cannot expand into +/// exponentially many owned array elements before the setter can reject it (CWE-400). +/// +/// Largest number of elements it may expand, and the deepest nesting it follows. +const TO_JSON_BUDGET: u64 = 2_000_000; +const TO_JSON_MAX_DEPTH: usize = 256; + +fn to_json_bounded(v: &Value, depth: usize, budget: &mut u64) -> Option { + if depth > TO_JSON_MAX_DEPTH || *budget == 0 { + return None; + } + *budget -= 1; + Some(match v { Value::Nil => serde_json::Value::Null, Value::Int(i) => serde_json::json!(i), Value::Real(r) => serde_json::json!(r), Value::Str(s) => serde_json::json!(&**s), - Value::Cons(_) => serde_json::Value::Array(v.iter().map(|x| to_json(&x)).collect()), + Value::Cons(_) => { + let mut arr = Vec::new(); + for x in v.iter() { + arr.push(to_json_bounded(&x, depth + 1, budget)?); + } + serde_json::Value::Array(arr) + } other => serde_json::json!(to_text(other, false)), - } + }) } pub fn getvar(ctx: &mut Ctx<'_>, a: Vec) -> Result { @@ -172,7 +188,11 @@ pub fn getvar(ctx: &mut Ctx<'_>, a: Vec) -> Result { pub fn setvar(ctx: &mut Ctx<'_>, a: Vec) -> Result { arity(&a, 2, 2)?; let name = string(&a[0])?; - match crate::sysvars::set(ctx.s, &name, &to_json(&a[1])) { + let mut budget: u64 = TO_JSON_BUDGET; + let Some(json) = to_json_bounded(&a[1], 0, &mut budget) else { + return Err(LispError::new(format!("AutoCAD variable setting rejected: {} value too large", to_text(&a[0], true)))); + }; + match crate::sysvars::set(ctx.s, &name, &json) { Ok(()) => Ok(Flow::Value(a[1].clone())), Err(_) => Err(LispError::new(format!("AutoCAD variable setting rejected: {} {}", to_text(&a[0], true), to_text(&a[1], true)))), } diff --git a/crates/engine/src/lisp/eval.rs b/crates/engine/src/lisp/eval.rs index e25798f6..3101b393 100644 --- a/crates/engine/src/lisp/eval.rs +++ b/crates/engine/src/lisp/eval.rs @@ -163,7 +163,8 @@ pub struct Ctx<'a> { pub const MAX_STACK: usize = 100_000; /// Nesting depth of synchronous callbacks (`vl-sort` comparing with a LISP function, …). const MAX_SYNC_DEPTH: usize = 32; - +/// Steps per slice while driving a synchronous callback ([`Ctx::call_sync`]). +const SYNC_SLICE: u64 = 400_000; enum State { Eval(Value), Ret(Value), @@ -645,12 +646,31 @@ impl Ctx<'_> { super::value::charge_result(&v)?; return Ok(v); } + let cap = self.s.lisp.limits.callback_steps; let mut m = Machine::calling(f.clone(), args); let mut sub = Ctx { s: &mut *self.s, env: &mut *self.env, depth: self.depth + 1 }; - match m.run(&mut sub, u64::MAX) { - Outcome::Done(v) => Ok(v), - Outcome::Error(e) => Err(e), - Outcome::Wait(_) => Err(LispError::new("this function can't ask for input here")), + // Drive the callback in bounded slices instead of granting it `u64::MAX` fuel. A `Yield` + // (the step budget ran out) is retried until a hard cap, so a constant-memory infinite + // loop inside a comparator or predicate terminates with an error rather than hanging the + // thread and bypassing the blocking execution cap (CWE-400). + let mut spent: u64 = 0; + loop { + let remaining = cap.saturating_sub(spent); + if remaining == 0 { + return Err(LispError::new(format!("callback stopped after {cap} steps"))); + } + let slice = remaining.min(SYNC_SLICE); + match m.run(&mut sub, slice) { + Outcome::Done(v) => return Ok(v), + Outcome::Error(e) => return Err(e), + Outcome::Wait(Need::Yield) => { + spent = spent.saturating_add(slice); + if spent >= cap { + return Err(LispError::new(format!("callback stopped after {cap} steps"))); + } + } + Outcome::Wait(_) => return Err(LispError::new("this function can't ask for input here")), + } } } } diff --git a/crates/engine/src/lisp/mod.rs b/crates/engine/src/lisp/mod.rs index d9fdc9a1..14ac32c6 100644 --- a/crates/engine/src/lisp/mod.rs +++ b/crates/engine/src/lisp/mod.rs @@ -126,11 +126,13 @@ pub(crate) struct Limits { pub cells: usize, /// Bytes of strings, printed text and files read per run. pub string_bytes: u64, + /// Maximum interpreter steps per synchronous callback. + pub callback_steps: u64, } impl Default for Limits { fn default() -> Self { - Limits { cells: value::MAX_CELLS, string_bytes: value::STRING_BUDGET } + Limits { cells: value::MAX_CELLS, string_bytes: value::STRING_BUDGET, callback_steps: 500_000_000 } } } diff --git a/crates/engine/src/lisp/tests.rs b/crates/engine/src/lisp/tests.rs index 04d78f89..20669056 100644 --- a/crates/engine/src/lisp/tests.rs +++ b/crates/engine/src/lisp/tests.rs @@ -221,6 +221,64 @@ fn runaway_routines_are_errors_not_crashes() { assert_eq!(r, Ok("3000000".into())); } +#[test] +fn synchronous_callback_loop_is_an_error_not_a_hang() { + // A comparator with an infinite loop used to run with u64::MAX fuel and hang the thread; it + // now terminates at the synchronous-callback step cap (CWE-400, finding row 2). + let mut s = session(); + s.lisp.limits.callback_steps = 1_000; + assert!(eval(&mut s, "(vl-sort '(1 2) (function (lambda (a b) (while t))))").is_err()); + // A well-behaved comparator still works afterwards. + assert_eq!(ok(&mut s, "(vl-sort '(3 1 2) '<)"), "(1 2 3)"); +} + +#[test] +fn a_zero_callback_budget_does_not_run_the_callback() { + let mut s = session(); + s.lisp.limits.callback_steps = 0; + ok(&mut s, "(setq callback-ran nil)"); + let result = eval(&mut s, "(vl-remove-if '(lambda (x) (setq callback-ran t)) '(1))"); + assert_eq!(result, Err("callback stopped after 0 steps".into())); + assert_eq!(ok(&mut s, "callback-ran"), "nil"); +} + +#[test] +fn a_callback_must_fit_within_a_partial_slice() { + let mut s = session(); + s.lisp.limits.callback_steps = 1_000; + ok(&mut s, "(setq callback-count 0)"); + let result = eval(&mut s, "(vl-remove-if '(lambda (x) (repeat 1000 (setq callback-count (1+ callback-count)))) '(1))"); + assert_eq!(result, Err("callback stopped after 1000 steps".into())); + assert_eq!(ok(&mut s, "(and (> callback-count 0) (< callback-count 1000))"), "T"); +} + +#[test] +fn reaching_a_callback_slice_boundary_does_not_grant_another_slice() { + let counts: Vec = [399_999, 400_000] + .into_iter() + .map(|cap| { + let mut s = session(); + s.lisp.limits.callback_steps = cap; + ok(&mut s, "(setq callback-count 0)"); + let result = eval(&mut s, "(vl-remove-if '(lambda (x) (while t (setq callback-count (1+ callback-count)))) '(1))"); + assert_eq!(result, Err(format!("callback stopped after {cap} steps"))); + ok(&mut s, "callback-count").parse().unwrap() + }) + .collect(); + // One additional VM step can perform at most one assignment, even at SYNC_SLICE's boundary. + assert!(counts[0] > 0); + assert!(counts[1] >= counts[0] && counts[1] - counts[0] <= 1, "{counts:?}"); +} + +#[test] +fn setvar_rejects_an_oversized_shared_structure() { + // A list repeatedly sharing its own contents expands exponentially; setvar's bounded JSON + // conversion rejects it instead of allocating gigabytes (CWE-400, finding row 39). + let mut s = session(); + let build = "(setq x '(1)) (repeat 40 (setq x (list x x))) (setvar \"GRIDUNIT\" x)"; + assert!(eval(&mut s, build).is_err()); +} + #[test] fn printing() { let mut s = session(); diff --git a/crates/engine/src/lisp/value.rs b/crates/engine/src/lisp/value.rs index f05d1716..99f066d7 100644 --- a/crates/engine/src/lisp/value.rs +++ b/crates/engine/src/lisp/value.rs @@ -343,18 +343,29 @@ pub fn eq(a: &Value, b: &Value) -> bool { /// `equal`: the same structure, numbers within `fuzz`. pub fn equal(a: &Value, b: &Value, fuzz: f64) -> bool { - equal_depth(a, b, fuzz, 0) + // A shared cons DAG can expand into exponentially many node visits when compared as a tree. + // Bound the total comparisons so `equal` on adversarial shared structure cannot monopolise the + // thread (CWE-407); exhausting the budget reports "not equal" rather than looping. + let mut budget: u64 = EQUAL_BUDGET; + equal_depth(a, b, fuzz, 0, &mut budget) } -fn equal_depth(a: &Value, b: &Value, fuzz: f64, depth: usize) -> bool { - if depth > MAX_DEPTH { +/// Largest number of node comparisons one `equal` call may perform. +const EQUAL_BUDGET: u64 = 50_000_000; + +fn equal_depth(a: &Value, b: &Value, fuzz: f64, depth: usize, budget: &mut u64) -> bool { + if depth > MAX_DEPTH || *budget == 0 { return false; } + *budget -= 1; let (mut a, mut b) = (a.clone(), b.clone()); loop { + if *budget == 0 { + return false; + } match (&a, &b) { (Value::Cons(x), Value::Cons(y)) => { - if !equal_depth(&x.car, &y.car, fuzz, depth + 1) { + if !equal_depth(&x.car, &y.car, fuzz, depth + 1, budget) { return false; } let (na, nb) = (x.cdr.clone(), y.cdr.clone());