From 81e1dcec5aa97361c0f6c10f39a0fb3ef2779381 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 14 Nov 2026 12:46:47 -0500 Subject: [PATCH] craft-dwg: refuse to convert without a worker instead of running acadrust here The bridge fell back to decoding in the calling process whenever no worker was configured, or the worker failed to start, which is exactly the case the audit called out: acadrust allocates from sizes a hostile file declares, and an allocation failure or stack overflow ends the process. Conversions are now refused without a worker, a worker that cannot start is an error, and allow_in_process() is the explicit opt-in for input known to be trusted. --- crates/dwg/README.md | 4 ++- crates/dwg/src/lib.rs | 12 ++++++- crates/dwg/src/worker.rs | 73 +++++++++++++++++++++++++++++++++------- 3 files changed, 74 insertions(+), 15 deletions(-) diff --git a/crates/dwg/README.md b/crates/dwg/README.md index ed52bdcfc..80088a89e 100644 --- a/crates/dwg/README.md +++ b/crates/dwg/README.md @@ -11,7 +11,9 @@ allocation failure inside the decoder. On native applications, call `use_own_wor startup and call `serve_if_asked()` before normal argument handling. Subsequent `decode` and `encode_*` calls then run in a child process with a 30-minute timeout; a crash or stack overflow is contained to that child. The worker has no operating-system memory limit yet. Without a -configured worker, conversions run in the current process. +configured worker, conversions are refused rather than run in the current process: call +`allow_in_process()` only for input that is known to be trusted (tests, or bytes this process +produced itself). Coordinates, once a worker is configured but cannot start, also fail closed. The bridge is not built for `wasm32` or FreeBSD/aarch64. Those targets retain the public API and return a clear unsupported-build error. The `acadrust` 0.6.6 dependency is used unmodified under diff --git a/crates/dwg/src/lib.rs b/crates/dwg/src/lib.rs index 2db7afeae..05c1a673c 100644 --- a/crates/dwg/src/lib.rs +++ b/crates/dwg/src/lib.rs @@ -21,6 +21,11 @@ //! These bound the input- and output-size variants. A small file that declares a huge internal //! table or section can still make the dependency allocate before any size is known, so the app //! runs both directions in a child process ([`worker`]): what that kills is the child. +//! +//! Isolation is not optional for untrusted input. With no worker configured the conversion is +//! refused rather than run here, and a worker that cannot start is an error, so a missing worker +//! never turns into decoding a hostile file in the caller's process. [`worker::allow_in_process`] +//! is the explicit exception, for input the caller knows is trusted. #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] #![forbid(unsafe_code)] @@ -65,7 +70,10 @@ fn mb(n: u64) -> String { #[cfg(not(no_dwg_bridge))] pub mod worker; #[cfg(not(no_dwg_bridge))] -pub use worker::{WORKER_ARG, crash_worker_for_test, decode, encode_as, encode_kept_noted, encode_noted, serve_if_asked, use_own_worker, use_worker}; +pub use worker::{ + WORKER_ARG, allow_in_process, crash_worker_for_test, decode, encode_as, encode_kept_noted, encode_noted, serve_if_asked, use_own_worker, + use_worker, +}; #[cfg(not(no_dwg_bridge))] mod proxy_transport; @@ -1569,6 +1577,8 @@ pub fn serve_if_asked() -> Option { #[cfg(no_dwg_bridge)] pub fn use_own_worker() {} #[cfg(no_dwg_bridge)] +pub fn allow_in_process() {} +#[cfg(no_dwg_bridge)] pub fn decode(bytes: &[u8], limits: Limits) -> Result<(Vec, Vec), String> { dwg_to_dxf_noted(bytes, limits) } diff --git a/crates/dwg/src/worker.rs b/crates/dwg/src/worker.rs index 0c273257b..1d16f5701 100644 --- a/crates/dwg/src/worker.rs +++ b/crates/dwg/src/worker.rs @@ -6,9 +6,11 @@ //! that reads one request from standard input and writes one reply to standard output. A file that //! kills the worker then costs that one open or save, and every other open drawing stays safe. //! -//! Without a worker (libraries, tests, a build that can't start one) [`decode`] and [`encode_as`] -//! run in the calling process, as they always did. There's no memory limit on the worker yet: that -//! takes per-platform system calls, which `unsafe_code = "forbid"` keeps out of this crate. +//! Without a worker [`decode`] and [`encode_as`] refuse rather than run the conversion in the +//! calling process, because the third-party code is the part that can end a process. +//! [`allow_in_process`] is the explicit exception, for input the caller knows is trusted (tests, or +//! bytes this process produced itself). There's no memory limit on the worker yet: that takes +//! per-platform system calls, which `unsafe_code = "forbid"` keeps out of this crate. //! //! A reply comes from a process that just handled a possibly hostile file, so it is parsed as //! untrusted input. @@ -18,6 +20,7 @@ use std::io::{Read, Write}; use std::path::PathBuf; use std::process::{Command, Stdio}; use std::sync::Mutex; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{RecvTimeoutError, channel}; use std::time::Duration; @@ -38,6 +41,9 @@ const MAX_NOTES_BYTES: u64 = 64 << 20; const MAX_DWG_REPLY: u64 = 2 << 30; /// Most bytes one request holds: the largest input, whichever the direction, and its header. const MAX_REQUEST: u64 = (2 << 30) + 256; +/// What a conversion says when it has no worker and is not allowed to run here. +const NO_WORKER: &str = "DWG: no worker is configured, so this file was not converted; \ + call use_worker or use_own_worker at startup, or allow_in_process for input that is trusted"; struct Worker { program: PathBuf, @@ -45,6 +51,8 @@ struct Worker { } static WORKER: Mutex> = Mutex::new(None); +/// Whether this process may run the conversion itself, set only by [`allow_in_process`]. +static IN_PROCESS: AtomicBool = AtomicBool::new(false); /// Start `program args` as the worker from now on. `program` must call [`serve_if_asked`]. pub fn use_worker(program: PathBuf, args: Vec) { @@ -62,12 +70,27 @@ pub fn use_own_worker() { } } +/// Let [`decode`] and [`encode_as`] run the conversion in this process when no worker is set. +/// +/// This opts out of the isolation the crate exists for: the third-party code allocates from sizes +/// a hostile file declares, and a failed allocation ends the process. Call it for input the caller +/// knows is trusted (tests, or bytes this process produced), never for a file that arrived from +/// outside. +pub fn allow_in_process() { + IN_PROCESS.store(true, Ordering::Relaxed); +} + +fn in_process_allowed() -> bool { + IN_PROCESS.load(Ordering::Relaxed) +} + /// Serve one request and return the exit code if this process was started as a worker. pub fn serve_if_asked() -> Option { (std::env::args_os().nth(1)? == WORKER_ARG).then(|| serve(std::io::stdin().lock(), std::io::stdout().lock())) } -/// [`super::dwg_to_dxf_noted`] in the worker, when there is one. +/// [`super::dwg_to_dxf_noted`] in the worker. With no worker configured this is refused unless +/// [`allow_in_process`] was called; a configured worker that cannot start is an error. pub fn decode(bytes: &[u8], limits: Limits) -> Result<(Vec, Vec), String> { // Before starting a process, and so the same limit holds with or without one. if bytes.len() > limits.max_dwg_bytes { @@ -76,17 +99,19 @@ pub fn decode(bytes: &[u8], limits: Limits) -> Result<(Vec, Vec), St let reply_limit = (limits.max_dxf_bytes as u64).saturating_add(MAX_NOTES_BYTES); match call(&request(DECODE, limits, ""), bytes, reply_limit, "reading this file") { Some(reply) => reply?, - None => super::dwg_to_dxf_noted(bytes, limits), + None if in_process_allowed() => super::dwg_to_dxf_noted(bytes, limits), + None => Err(NO_WORKER.to_string()), } } -/// [`super::dxf_to_dwg_as`] in the worker, when there is one. +/// [`super::dxf_to_dwg_as`] in the worker. Refused without a worker unless [`allow_in_process`] +/// was called. pub fn encode_as(dxf: &[u8], version: &str) -> Result, String> { encode_noted(dxf, version).map(|(dwg, _)| dwg) } -/// [`super::dxf_to_dwg_noted`] in the worker, when there is one. What the writer left out comes -/// back as notes of `TYPEcount`. +/// [`super::dxf_to_dwg_noted`] in the worker. What the writer left out comes back as notes of +/// `TYPEcount`. pub fn encode_noted(dxf: &[u8], version: &str) -> Result<(Vec, super::Unwritten), String> { encode_kept_noted(dxf, version, None) } @@ -103,7 +128,8 @@ pub fn encode_kept_noted(dxf: &[u8], version: &str, kept: Option<&str>) -> Resul push_str(&mut header, kept.unwrap_or("")); match call(&header, dxf, MAX_DWG_REPLY, "writing this file") { Some(reply) => reply?.map(|(dwg, notes)| (dwg, unwritten_from(¬es))), - None => encode_here(dxf, version, kept), + None if in_process_allowed() => encode_here(dxf, version, kept), + None => Err(NO_WORKER.to_string()), } } @@ -147,8 +173,9 @@ fn push_str(r: &mut Vec, text: &str) { type Reply = Result<(Vec, Vec), String>; -/// Run one request in a worker. `None`: none could be started, so the caller does the work. -/// `Err`: the worker went wrong, and `doing` says at what, for the user. +/// Run one request in a worker. `None`: no worker is configured, so the caller decides what to do +/// (convert here only with [`allow_in_process`], or refuse). `Err`: a configured worker could not +/// start or went wrong, and `doing` says at what, for the user. fn call(header: &[u8], payload: &[u8], reply_limit: u64, doing: &str) -> Option> { let mut command = { let worker = WORKER.lock().ok()?; @@ -167,14 +194,15 @@ fn call(header: &[u8], payload: &[u8], reply_limit: u64, doing: &str) -> Option< let mut child = match command.spawn() { Ok(c) => c, Err(e) => { + // A worker is configured but cannot run: never fall back to decoding here. log::warn!("DWG worker: can't start it: {e}"); - return None; + return Some(Err(format!("the DWG worker could not start ({e}); nothing else is affected"))); } }; let (Some(mut to_child), Some(from_child)) = (child.stdin.take(), child.stdout.take()) else { let _ = child.kill(); let _ = child.wait(); - return None; + return Some(Err("the DWG worker could not start (no pipes); nothing else is affected".to_string())); }; let received = std::thread::scope(|scope| { // Written beside the read: a worker that replies before it has read everything, or dies, @@ -367,6 +395,25 @@ mod tests { } } + /// A conversion is refused rather than run here, and a worker that cannot start is an error: + /// neither case may turn into decoding a (possibly hostile) file in this process. + #[test] + fn a_conversion_is_refused_without_a_worker_and_never_falls_back_to_this_process() { + // No worker is configured and nothing opted in. + assert_eq!(decode(b"AC1032", Limits::DEFAULT).unwrap_err(), NO_WORKER); + assert_eq!(encode_as(b"not a dxf", "AC1032").unwrap_err(), NO_WORKER); + // Trusted input may run here, and the reader still reports a file it cannot read. + allow_in_process(); + assert!(in_process_allowed()); + assert!(decode(b"AC1032", Limits::DEFAULT).is_err()); + assert!(encode_as(b"not a dxf", "AC1032").is_err()); + // A configured worker that cannot start is an error even after the opt-in: the conversion + // never runs here instead. + use_worker(PathBuf::from("/nonexistent/craft-dwg-worker"), Vec::new()); + assert!(decode(b"AC1032", Limits::DEFAULT).unwrap_err().contains("could not start")); + assert!(encode_as(b"not a dxf", "AC1032").unwrap_err().contains("could not start")); + } + #[test] fn serving_reports_a_decode_failure_as_a_reply() { let mut request = self::request(DECODE, Limits::DEFAULT, "");