diff --git a/src/isa/memory.rs b/src/isa/memory.rs index e01805cd..0ca9773c 100644 --- a/src/isa/memory.rs +++ b/src/isa/memory.rs @@ -3,6 +3,21 @@ use crate::{ }; use rwasm_fuel_policy::{MEMORY_BYTES_PER_FUEL, MEMORY_BYTES_PER_FUEL_LOG2}; +// SAFETY NOTE on `MEMORY_BYTES_PER_FUEL`, applies to every bulk prologue in this file. +// +// Fuel is charged as `(n + MEMORY_BYTES_PER_FUEL - 1) >> MEMORY_BYTES_PER_FUEL_LOG2`, where the +// `i32.add` wraps for `n >= 2^32 - (MEMORY_BYTES_PER_FUEL - 1)`. For such `n` the round-up wraps +// to a tiny value, so a nominally multi-gigabyte request is charged (almost) nothing. +// +// This is not a metering bypass today: a memory is capped at `N_MAX_ALLOWED_MEMORY_PAGES` pages +// (2 GiB), so any `n` large enough to wrap traps on the runtime bounds check before a single byte +// is touched, and the undercharged operation never performs work. The correctness of the charge +// therefore rests entirely on that bounds check, not on this arithmetic. Growing +// `MEMORY_BYTES_PER_FUEL` toward `u32::MAX`, or admitting bulk operations whose length is not +// bounded by the memory limit, would make this exploitable — in that case replace the round-up +// with an overflow-safe form such as `(n >> LOG2) + ((n & MASK) != 0)`, or do the arithmetic in +// 64 bits. + impl InstructionSet { pub const MSH_I64_LOAD: u32 = 2; pub const MSH_I64_LOAD8_S: u32 = 1; @@ -118,6 +133,16 @@ impl InstructionSet { } /// Max stack height: 2 + /// + /// # Safety + /// + /// The limit check below is a *signed* compare over quantities that are unsigned by + /// construction, and the sum `memory_size+d` uses a wrapping `i32.add`. A delta close to + /// `i32::MAX` therefore produces a negative sum that passes the guard. That is harmless only + /// because `memory.grow` itself re-checks the resulting page count against + /// [`crate::N_MAX_ALLOWED_MEMORY_PAGES`] and fails (returning `u32::MAX`) for every input that + /// can wrap. Do not treat this guard as the bounds check — see the note on + /// [`crate::N_MAX_ALLOWED_MEMORY_PAGES`] before raising any of the limits it depends on. pub fn op_memory_grow_checked(&mut self, max_pages: Option, inject_fuel_check: bool) { // we must do max memory check before an execution if let Some(max_pages) = max_pages { @@ -125,19 +150,21 @@ impl InstructionSet { self.op_memory_size(); // memory_size self.op_i32_add(); // memory_size+d self.op_i32_const(max_pages); // max_pages - self.op_i32_gt_s(); // memory_size+d>max_pages + self.op_i32_gt_s(); // memory_size+d>max_pages, signed: see the note above self.op_br_if_eqz(4); self.op_drop(); self.op_i32_const(u32::MAX); self.op_br(if inject_fuel_check { 8 } else { 2 }); } - // now we know that pages can't exceed i32::MAX, - // so we can safely multiply the num of pages to the page size - // to calculate fuel required for memory to grow + // fuel for the growth is charged from the requested number of pages; + // note that the guard above does NOT establish that `n` is small: a wrapped `n` reaches + // this code, and `n*65536` wraps too, so the charge can be far below the nominal size. + // it is not a metering bypass because the `memory.grow` below rejects every such `n` + // without allocating anything, so no undercharged work is ever performed if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(N_BYTES_PER_MEMORY_PAGE); // size of each memory page - self.op_i32_mul(); // overflow is impossible here (we pass max pages in trustless mode) + self.op_i32_mul(); // wrapping, see the note above self.op_i32_const(MEMORY_BYTES_PER_FUEL_LOG2); // 2^6=64 self.op_i32_shr_u(); // delta/64 self.op_consume_fuel_stack(); @@ -152,7 +179,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(MEMORY_BYTES_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(MEMORY_BYTES_PER_FUEL_LOG2); // 2^6=64 self.op_i32_shr_u(); // delta/64 self.op_consume_fuel_stack(); @@ -167,7 +194,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(MEMORY_BYTES_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(MEMORY_BYTES_PER_FUEL_LOG2); // 2^6=64 self.op_i32_shr_u(); // delta/64 self.op_consume_fuel_stack(); @@ -218,13 +245,16 @@ impl InstructionSet { data_segment_index: DataSegmentIdx, inject_fuel_check: bool, ) { - // do an overflow check + // do an overflow check; + // note that this compare is signed and `n + s` wraps, so operands close to `i32::MAX` + // slip past it — the data segment bounds check inside `memory.init` below is what + // actually rejects them, this guard only turns the common case into an early trap if let Some(length) = rewrite_length.filter(|v| *v > 0) { self.op_local_get(1); // n self.op_local_get(3); // s self.op_i32_add(); // n + s self.op_i32_const(length); - self.op_i32_gt_s(); // n + s > length + self.op_i32_gt_s(); // n + s > length, signed: see the note above self.op_br_if_eqz(2); self.op_trap(TrapCode::MemoryOutOfBounds); } @@ -239,7 +269,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(MEMORY_BYTES_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(MEMORY_BYTES_PER_FUEL_LOG2); // 2^6=64 self.op_i32_shr_u(); // delta/64 self.op_consume_fuel_stack(); diff --git a/src/isa/table.rs b/src/isa/table.rs index a2a6d234..b4583905 100644 --- a/src/isa/table.rs +++ b/src/isa/table.rs @@ -1,6 +1,25 @@ use crate::{ElementSegmentIdx, InstructionSet, TableIdx, TrapCode}; use rwasm_fuel_policy::{TABLE_ELEMS_PER_FUEL, TABLE_ELEMS_PER_FUEL_LOG2}; +// SAFETY NOTE on `TABLE_ELEMS_PER_FUEL` and the limit checks, applies to every prologue below. +// +// Two pieces of the injected arithmetic are unsound in isolation: +// +// * The limit checks use a signed `i32.gt_s` over quantities that are unsigned by construction, +// and the `n+s` / `n+table_size` sums use a wrapping `i32.add`. An `n` close to `i32::MAX` +// makes the sum negative, so the guard passes. +// * Fuel is charged as `(n + TABLE_ELEMS_PER_FUEL - 1) >> TABLE_ELEMS_PER_FUEL_LOG2`, where the +// `i32.add` wraps for `n >= 2^32 - (TABLE_ELEMS_PER_FUEL - 1)` and rounds down to (almost) zero +// fuel for a nominally 4 G-element operation. +// +// Neither is reachable today: a table holds at most `N_MAX_TABLE_SIZE` (1024) elements, so every +// `n` big enough to wrap is rejected by the runtime table bounds check (`grow_untyped` uses +// `checked_add` plus the size cap, the bulk ops go through slice bounds checks) before any element +// is touched, and the undercharged operation never performs work. The guards here are an early +// trap, not the bounds check. Raising `N_MAX_TABLE_SIZE` toward `i32::MAX`, or growing +// `TABLE_ELEMS_PER_FUEL`, requires switching these compares to `i32.gt_u` and replacing the +// round-up with an overflow-safe form such as `(n >> LOG2) + ((n & MASK) != 0)` first. + impl InstructionSet { pub const MSH_TABLE_INIT_CHECKED: u32 = 2; pub const MSH_TABLE_GROW_CHECKED: u32 = 2; @@ -21,7 +40,7 @@ impl InstructionSet { self.op_local_get(3); // s self.op_i32_add(); // n+s self.op_i32_const(length); // length - self.op_i32_gt_s(); // n+s>length + self.op_i32_gt_s(); // n+s>length, signed: see the SAFETY NOTE at the top of this file self.op_br_if_eqz(2); self.op_trap(TrapCode::TableOutOfBounds); // we need to replace the offset on the stack with the new value @@ -35,7 +54,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(TABLE_ELEMS_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(TABLE_ELEMS_PER_FUEL_LOG2); // 2^4=16 self.op_i32_shr_u(); // n/16 self.op_consume_fuel_stack(); @@ -57,7 +76,7 @@ impl InstructionSet { self.op_table_size(table_idx); // table_size self.op_i32_add(); // n+table_size self.op_i32_const(limit); // limit - self.op_i32_gt_s(); // n+table_size>limit + self.op_i32_gt_s(); // n+table_size>limit, signed: see the SAFETY NOTE at the top self.op_br_if_eqz(5); self.op_drop(); self.op_drop(); @@ -69,7 +88,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(TABLE_ELEMS_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(TABLE_ELEMS_PER_FUEL_LOG2); // 2^4=16 self.op_i32_shr_u(); // n/16 self.op_consume_fuel_stack(); @@ -82,7 +101,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(TABLE_ELEMS_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(TABLE_ELEMS_PER_FUEL_LOG2); // 2^4=16 self.op_i32_shr_u(); // n/16 self.op_consume_fuel_stack(); @@ -100,7 +119,7 @@ impl InstructionSet { if inject_fuel_check { self.op_local_get(1); // n self.op_i32_const(TABLE_ELEMS_PER_FUEL - 1); // upper round - self.op_i32_add(); + self.op_i32_add(); // wrapping, see the SAFETY NOTE at the top of this file self.op_i32_const(TABLE_ELEMS_PER_FUEL_LOG2); // 2^4=16 self.op_i32_shr_u(); // n/16 self.op_consume_fuel_stack(); diff --git a/src/types/mod.rs b/src/types/mod.rs index d83773ed..49ab341b 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -47,6 +47,22 @@ pub const N_DEFAULT_MAX_MEMORY_PAGES: u32 = 1024; /// A hard limit on the maximum number of memory pages that can be allocated. /// This value is driven from a Wasm standard, the maximum number of memory pages is 32,768. +/// +/// # Safety +/// +/// The compiler-injected prologues for `memory.grow`/`memory.init` (see [`crate::InstructionSet`] +/// in `src/isa/memory.rs`) compare `size + delta` against this limit with a *signed* `i32.gt_s` +/// and compute fuel with a *wrapping* `i32.add` round-up. Both can be defeated by an operand +/// close to `i32::MAX`/`u32::MAX`: the sum wraps negative and passes the guard, and the fuel +/// round-up wraps down to (nearly) zero fuel. +/// +/// This is currently not exploitable only because such inputs always trap on the runtime bounds +/// check that runs behind the guard: `32768 * 65536` fits in `u32`, so no reachable page count +/// can make the wrapped path touch memory or skip metering for work actually performed. +/// Raising this constant, or otherwise letting the guards see values that overflow `i32`, would +/// turn those prologues into a real bounds/metering bypass with no visible change to this code. +/// Change it only together with switching the guards to unsigned comparisons and overflow-safe +/// fuel arithmetic. pub const N_MAX_ALLOWED_MEMORY_PAGES: u32 = 32768; /// A default memory index in a Wasm binary. @@ -79,6 +95,16 @@ pub const N_MAX_TABLES: u32 = 100; /// /// The original standard allows `100_000` element segments with an unlimited number of elements /// inside. +/// +/// # Safety +/// +/// The same caveat as for [`N_MAX_ALLOWED_MEMORY_PAGES`] applies: the injected prologues for +/// `table.grow`/`table.init` (`src/isa/table.rs`) use signed `i32.gt_s` on `size + delta` and a +/// wrapping `i32.add` for the fuel round-up. With this limit the guarded sums stay far below +/// `i32::MAX`, so the signedness and the wrap are unobservable, and any input large enough to +/// wrap traps on the runtime table bounds check anyway. Raising this constant to anything near +/// `i32::MAX` requires switching those guards to unsigned comparisons and overflow-safe fuel +/// arithmetic first. pub const N_MAX_TABLE_SIZE: u32 = 1024; pub type InstrLoc = u32;