diff --git a/README.md b/README.md index f3a68b4..891718e 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,13 @@ recursively whether anybody asked or not — so ten thousand elements inside each other overflowed the stack and aborted the process. In a browser that is the tab, and the force somebody was building goes with it. +A page gets told which of those it hit. `helm_mul_load` answers `ERR_TOO_LARGE` +rather than `ERR_UNREADABLE` for a file past a limit, and `helm_last_error` +says which one — a file that is not a `.mul` is one somebody picked by +mistake, and a file past a limit is one helm will not read however long it is +looked at, and those are different things to be told. `helm.mjs` puts them on +the thrown `HelmError` as `.tooLarge` and `.detail`. + The XML attacks the format is known for are not reachable: `quick-xml` does no DTD processing, no entity expansion and no external entity resolution, so billion laughs and XXE have nowhere to land. diff --git a/TODO.md b/TODO.md index 5195984..280d33d 100644 --- a/TODO.md +++ b/TODO.md @@ -1289,6 +1289,14 @@ not obvious from any one of them. it. The XML attacks proper were already unreachable - `quick-xml` does no DTD processing, no entity expansion, no external entities. + - [x] **A page is told which refusal it hit.** `ERR_TOO_LARGE` for a + file past a limit against `ERR_UNREADABLE` for one that is not a + `.mul`, and `helm_last_error` for which limit - a code cannot carry + "nested more than 64 deep" and that is the half somebody can act on. + `helm.mjs` puts them on the thrown error as `.tooLarge` and + `.detail`, `smoke.mjs` drives all three cases, and the boundary is at + version 4. + - [x] **`Kept` keeps its fields to itself.** Everything that changes an element goes through a method that forgets the bytes it was read from, so a document cannot be left claiming to be bytes that no longer diff --git a/crates/helm-unitfile/src/lib.rs b/crates/helm-unitfile/src/lib.rs index c8c4108..3174c23 100644 --- a/crates/helm-unitfile/src/lib.rs +++ b/crates/helm-unitfile/src/lib.rs @@ -29,9 +29,12 @@ mod mul; pub use blk::parse_blk; pub use mtf::{armor_location_order, parse_mtf, split_pipe_list, split_system_field}; +/// Why a `.mul` could not be read. Named apart from this crate's own +/// [`Error`], which is about a design rather than a force. +pub use mul::Error as MulError; pub use mul::{ - Fate, ForceNode, ForceRef, Kept, MUL_VERSION, Mul, MulUnit, locations_for, - locations_for_config, parse_force_chain, parse_mul, write_mul, + Fate, ForceNode, ForceRef, Kept, MAX_BYTES, MAX_DEPTH, MAX_ELEMENTS, MUL_VERSION, Mul, MulUnit, + Origin, locations_for, locations_for_config, parse_force_chain, parse_mul, write_mul, }; #[cfg(feature = "library")] diff --git a/crates/helm-unitfile/src/mul.rs b/crates/helm-unitfile/src/mul.rs index f363953..1aecbc9 100644 --- a/crates/helm-unitfile/src/mul.rs +++ b/crates/helm-unitfile/src/mul.rs @@ -349,8 +349,29 @@ pub const MAX_ELEMENTS: usize = 100_000; pub const MAX_BYTES: usize = 16 * 1024 * 1024; /// Why a `.mul` could not be read. +/// +/// Two kinds, and a caller has to be able to tell them apart: a file that is +/// not a `.mul` is one somebody picked by mistake, and a file past one of the +/// limits above is one that will not be read however long it is looked at. #[derive(Debug)] -pub struct Error(String); +pub struct Error(String, Refusal); + +/// Which kind of refusal an [`Error`] is. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Refusal { + /// The XML could not be read, or said nothing this understands. + Unreadable, + /// Past [`MAX_DEPTH`], [`MAX_ELEMENTS`] or [`MAX_BYTES`]. + PastALimit, +} + +impl Error { + /// Whether this file was refused by one of the reader's limits rather + /// than for being unreadable. + pub fn past_a_limit(&self) -> bool { + self.1 == Refusal::PastALimit + } +} impl std::fmt::Display for Error { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { @@ -824,10 +845,13 @@ pub fn parse_mul(text: &str) -> Result { // question is not whether this one is reasonable but what an unreasonable // one is allowed to cost. if text.len() > MAX_BYTES { - return Err(Error(format!( - "{} bytes, and {MAX_BYTES} is the most this reads", - text.len() - ))); + return Err(Error( + format!( + "{} bytes, and {MAX_BYTES} is the most this reads", + text.len() + ), + Refusal::PastALimit, + )); } // Shared once, pointed at by every element that came out of it. let source: Arc = Arc::from(text); @@ -857,7 +881,9 @@ pub fn parse_mul(text: &str) -> Result { // lets a parent that has to be printed again still put an untouched // child back on the line it was on. let from = reader.buffer_position() as usize; - let event = reader.read_event().map_err(|e| Error(e.to_string()))?; + let event = reader + .read_event() + .map_err(|e| Error(e.to_string(), Refusal::Unreadable))?; let to = reader.buffer_position() as usize; let (own_lead, began) = split_lead(&source, from, to); let lead = pending.take().or(own_lead).map(&at); @@ -865,15 +891,17 @@ pub fn parse_mul(text: &str) -> Result { Event::Eof => break, Event::Start(tag) => { if open.len() >= MAX_DEPTH { - return Err(Error(format!( - "nested more than {MAX_DEPTH} deep, which no force is" - ))); + return Err(Error( + format!("nested more than {MAX_DEPTH} deep, which no force is"), + Refusal::PastALimit, + )); } elements += 1; if elements > MAX_ELEMENTS { - return Err(Error(format!( - "more than {MAX_ELEMENTS} elements, which no force has" - ))); + return Err(Error( + format!("more than {MAX_ELEMENTS} elements, which no force has"), + Refusal::PastALimit, + )); } let mut kept = element(&tag)?; kept.lead = lead; @@ -882,15 +910,17 @@ pub fn parse_mul(text: &str) -> Result { } Event::Empty(tag) => { if open.len() >= MAX_DEPTH { - return Err(Error(format!( - "nested more than {MAX_DEPTH} deep, which no force is" - ))); + return Err(Error( + format!("nested more than {MAX_DEPTH} deep, which no force is"), + Refusal::PastALimit, + )); } elements += 1; if elements > MAX_ELEMENTS { - return Err(Error(format!( - "more than {MAX_ELEMENTS} elements, which no force has" - ))); + return Err(Error( + format!("more than {MAX_ELEMENTS} elements, which no force has"), + Refusal::PastALimit, + )); } let mut done = element(&tag)?; done.lead = lead; @@ -1096,13 +1126,13 @@ fn close(open: &mut [(Kept, usize)], done: Kept, mul: &mut Mul) { fn element(tag: &quick_xml::events::BytesStart<'_>) -> Result { let mut attributes = Vec::new(); for attr in tag.attributes() { - let attr = attr.map_err(|e| Error(e.to_string()))?; + let attr = attr.map_err(|e| Error(e.to_string(), Refusal::Unreadable))?; // Unescaped here and nowhere else: `&` in a chassis name is a real // ampersand, and a name that keeps the escape matches no design. attributes.push(( String::from_utf8_lossy(attr.key.as_ref()).into_owned(), attr.unescape_value() - .map_err(|e| Error(e.to_string()))? + .map_err(|e| Error(e.to_string(), Refusal::Unreadable))? .into_owned(), )); } diff --git a/crates/helm-wasm/helm.mjs b/crates/helm-wasm/helm.mjs index 4caff8c..d19e5c5 100644 --- a/crates/helm-wasm/helm.mjs +++ b/crates/helm-wasm/helm.mjs @@ -20,7 +20,7 @@ // including when the call throws. /** The boundary this wrapper was written against; the module must agree. */ -const ABI_VERSION = 5n; +const ABI_VERSION = 6n; const ERRORS = new Map([ [-1n, "no equipment catalogue is loaded"], @@ -32,13 +32,31 @@ const ERRORS = new Map([ [-7n, "that force has been let go of"], [-8n, "the force holds no unit there"], [-9n, "that library has been let go of"], + [-10n, "the file is past one of the reader's limits"], ]); +/** The code for a file helm will not read however long it is looked at. */ +export const ERR_TOO_LARGE = -10; + export class HelmError extends Error { - constructor(code) { + /** + * `detail` is what the module said about this particular failure - which + * limit, or what the XML did wrong - where the message is the class of + * failure. A page telling somebody why their file would not load wants + * both: `tooLarge` decides which thing to say, `detail` says which limit. + */ + constructor(code, detail) { super(ERRORS.get(code) ?? `helm-wasm returned ${code}`); this.name = "HelmError"; this.code = Number(code); + if (detail) { + this.detail = detail; + } + } + + /** Whether the file was refused by a limit rather than for being unreadable. */ + get tooLarge() { + return this.code === ERR_TOO_LARGE; } } @@ -125,9 +143,10 @@ export class Helm { * pass the text again each time. Call `free()` when done with it. */ design(mtf) { - const handle = check(this.#withBytes(mtf, (ptr, len) => - this.#api.helm_design_load(ptr, len), - )); + const handle = check( + this.#withBytes(mtf, (ptr, len) => this.#api.helm_design_load(ptr, len)), + this.#api, + ); return new Design(this.#api, handle, (text, use) => this.#withBytes(text, use), (packed) => this.#take(packed), ); @@ -143,7 +162,10 @@ export class Helm { * is written back is the file that arrived, edited. */ force(mul) { - const handle = check(this.#withBytes(mul, (ptr, len) => this.#api.helm_mul_load(ptr, len))); + const handle = check( + this.#withBytes(mul, (ptr, len) => this.#api.helm_mul_load(ptr, len)), + this.#api, + ); return new Force(this.#api, handle, (text, use) => this.#withBytes(text, use), (packed) => this.#take(packed), ); @@ -168,7 +190,7 @@ export class Helm { /** Read a packed pointer-and-length back as a string, and free it. */ #take(packed) { - const value = check(packed); + const value = check(packed, this.#api); const ptr = Number(value >> 32n); const len = Number(value & 0xffffffffn); try { @@ -419,13 +441,38 @@ export class Design { } } -function check(value) { +/** + * Turn a negative return into a throw, with whatever the module said about it. + * + * `api` is optional only so that a caller with nothing to ask still works; a + * page telling somebody why their `.mul` would not load wants the detail, and + * every path that reads a file passes it. + */ +function check(value, api) { if (typeof value === "bigint" && value < 0n) { - throw new HelmError(value); + throw new HelmError(value, api && lastError(api)); } return value; } +/** What the module said about its last failure, or undefined. */ +function lastError(api) { + if (typeof api.helm_last_error !== "function") { + return undefined; + } + const packed = api.helm_last_error(); + if (typeof packed !== "bigint" || packed <= 0n) { + return undefined; + } + const ptr = Number(packed >> 32n); + const len = Number(packed & 0xffffffffn); + try { + return new TextDecoder().decode(new Uint8Array(api.memory.buffer, ptr, len)); + } finally { + api.helm_free(ptr, len); + } +} + async function instantiate(source) { if (source instanceof WebAssembly.Module) { return WebAssembly.instantiate(source, {}); diff --git a/crates/helm-wasm/smoke.mjs b/crates/helm-wasm/smoke.mjs index d554468..27df8bb 100644 --- a/crates/helm-wasm/smoke.mjs +++ b/crates/helm-wasm/smoke.mjs @@ -7,7 +7,7 @@ // Driven through helm.mjs rather than the raw exports, because the wrapper is // what an application imports and so is the thing worth testing. import { readFileSync } from "node:fs"; -import { Helm, HelmError } from "./helm.mjs"; +import { Helm, HelmError, ERR_TOO_LARGE } from "./helm.mjs"; const [catalogue, design, damaged, designs, mulFile] = process.argv.slice(2); if (!catalogue || !design) { @@ -355,4 +355,50 @@ if (mulFile) { force.free(); } +// A file a page was given rather than one it made. Somebody pastes a link or +// picks a file, and what comes back has to be a thing a page can say, not a +// dead module: a stack overflow in wasm is a trap, and the force somebody was +// building goes with it. +{ + const nested = (deep) => + `${"".repeat(deep)}${"".repeat(deep)}`; + const refused = (what, text, wantCode, says) => { + try { + helm.force(text).free(); + console.log(`FAIL ${what}: it loaded`); + failed++; + } catch (e) { + const ok = + e instanceof HelmError && e.code === wantCode && (e.detail ?? "").includes(says); + console.log(`${ok ? "ok " : "FAIL"} ${what}: ${e.detail ?? e.message}`); + if (!ok) failed++; + } + }; + + refused("a file nested past reading", nested(10_000), ERR_TOO_LARGE, "deep"); + refused("a file longer than this reads", " ".repeat(20 * 1024 * 1024), ERR_TOO_LARGE, "bytes"); + refused( + "a file that is not a .mul", + ' + +`); + check("a real force still loads afterwards", after.units.length, 1); + after.free(); +} + process.exit(failed === 0 ? 0 : 1); diff --git a/crates/helm-wasm/src/lib.rs b/crates/helm-wasm/src/lib.rs index 026166a..5c30972 100644 --- a/crates/helm-wasm/src/lib.rs +++ b/crates/helm-wasm/src/lib.rs @@ -53,6 +53,16 @@ pub const ERR_NO_SUCH_UNIT: i64 = -8; /// No library is loaded at that handle. pub const ERR_NO_LIBRARY: i64 = -9; +/// The file is past one of the reader's limits - too deep, too many elements, +/// or too long. +/// +/// Its own code rather than [`ERR_UNREADABLE`] because the two are different +/// things to be told. A file that is not a `.mul` is a file somebody picked by +/// mistake; a file past a limit is one helm will not read however long it is +/// looked at, and a page has to say so rather than "could not be read". +/// [`helm_last_error`] says which limit. +pub const ERR_TOO_LARGE: i64 = -10; + // The catalogue, and the designs a caller is holding on to. // // Reading a `.mtf` is most of what scoring one costs - the arithmetic is cheap @@ -153,8 +163,9 @@ pub unsafe extern "C" fn helm_mul_to_json(ptr: *const u8, len: usize) -> i64 { let Some(text) = (unsafe { text(ptr, len) }) else { return ERR_NOT_UTF8; }; - let Ok(mul) = helm_unitfile::parse_mul(text) else { - return ERR_UNREADABLE; + let mul = match helm_unitfile::parse_mul(text) { + Ok(mul) => mul, + Err(why) => return failed(why.to_string(), refusal(&why)), }; let units: Vec = mul.machines().map(machine_json).collect(); give(serde_json::json!({ "units": units }).to_string()) @@ -286,6 +297,36 @@ fn condition_json(unit: &helm_unitfile::MulUnit) -> serde_json::Value { }) } +thread_local! { + /// What went wrong most recently, for a caller that wants to say more + /// than the code does. + static LAST_ERROR: RefCell> = const { RefCell::new(None) }; +} + +/// Remember why something failed, and answer the code for it. +/// +/// A `.mul` that is refused is refused for a reason a person can act on - it +/// is nested too deep, or it is longer than this reads - and a code cannot +/// carry that. The reason is held until the next failure and fetched with +/// [`helm_last_error`]. +fn failed(why: impl Into, code: i64) -> i64 { + let why = why.into(); + LAST_ERROR.with(|last| *last.borrow_mut() = Some(why)); + code +} + +/// Why the last call failed, as a pointer and a length, or 0 if nothing has. +/// +/// The caller owns the bytes and must free them. Reading it does not clear +/// it; the next failure replaces it. +#[unsafe(no_mangle)] +pub extern "C" fn helm_last_error() -> i64 { + match LAST_ERROR.with(|last| last.borrow().clone()) { + Some(why) => give(why), + None => 0, + } +} + /// Hand a string to the caller as a packed pointer and length. fn give(text: String) -> i64 { let bytes = text.into_bytes(); @@ -299,6 +340,18 @@ fn give(text: String) -> i64 { ((ptr as u32 as i64) << 32) | (len as u32 as i64) } +/// Which code a refusal is: past a limit, or simply not a `.mul`. +/// +/// Asked of the reader rather than guessed from the message, so a limit that +/// is added later is not silently reported as an unreadable file. +fn refusal(why: &helm_unitfile::MulError) -> i64 { + if why.past_a_limit() { + ERR_TOO_LARGE + } else { + ERR_UNREADABLE + } +} + /// Read a `.mul` and keep it, returning a handle to work on. /// /// The force stays here rather than crossing as JSON, because a `.mul` carries @@ -315,8 +368,9 @@ pub unsafe extern "C" fn helm_mul_load(ptr: *const u8, len: usize) -> i64 { let Some(text) = (unsafe { text(ptr, len) }) else { return ERR_NOT_UTF8; }; - let Ok(mul) = helm_unitfile::parse_mul(text) else { - return ERR_UNREADABLE; + let mul = match helm_unitfile::parse_mul(text) { + Ok(mul) => mul, + Err(why) => return failed(why.to_string(), refusal(&why)), }; FORCES.with(|f| { let mut forces = f.borrow_mut(); @@ -794,7 +848,7 @@ fn with_library_mut(handle: i64, f: impl FnOnce(&mut Filterable) -> i64) -> i64 /// not loaded with what it needs rather than refusing it. #[unsafe(no_mangle)] pub extern "C" fn helm_abi_version() -> i64 { - 5 + 6 } /// How many equipment entries are loaded, so a caller can tell an empty @@ -1340,11 +1394,25 @@ mod tests { for hostile in [deep, " ".repeat(20 * 1024 * 1024)] { let handle = unsafe { helm_mul_load(hostile.as_ptr(), hostile.len()) }; - assert_eq!(handle, ERR_UNREADABLE, "a hostile file was loaded"); + assert_eq!(handle, ERR_TOO_LARGE, "a hostile file was loaded"); let json = unsafe { helm_mul_to_json(hostile.as_ptr(), hostile.len()) }; - assert_eq!(json, ERR_UNREADABLE, "a hostile file was read"); + assert_eq!(json, ERR_TOO_LARGE, "a hostile file was read"); + // Something was said about which limit. That something rather + // than what: reading it means unpacking a pointer this packs into + // the top half of an `i64`, which holds on wasm32 and not on the + // machine a `cargo test` runs on - a host pointer whose low half + // has its top bit set packs to a negative number. `smoke.mjs` + // checks what it says, against a real wasm build. + assert_ne!(helm_last_error(), 0, "nothing was said about why"); } + // A file that is simply not a `.mul` is a different thing to be told. + let nonsense = r#""#; assert!(unsafe { helm_mul_load(real.as_ptr(), real.len()) } > 0);