diff --git a/piecrust/CHANGELOG.md b/piecrust/CHANGELOG.md index 3921e740..5a09315b 100644 --- a/piecrust/CHANGELOG.md +++ b/piecrust/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `Error::ArgumentBufferOverflow` variant [#343] + +### Fixed + +- Fix possible under/overflows reported by audit [#343] + ### Changed @@ -392,6 +400,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#347]: https://github.com/dusk-network/piecrust/issues/347 [#344]: https://github.com/dusk-network/piecrust/issues/344 +[#343]: https://github.com/dusk-network/piecrust/issues/343 [#336]: https://github.com/dusk-network/piecrust/issues/336 [#325]: https://github.com/dusk-network/piecrust/issues/325 [#324]: https://github.com/dusk-network/piecrust/issues/324 diff --git a/piecrust/src/call_tree.rs b/piecrust/src/call_tree.rs index 669a4c3e..15a5d1be 100644 --- a/piecrust/src/call_tree.rs +++ b/piecrust/src/call_tree.rs @@ -150,6 +150,9 @@ impl Drop for CallTree { unsafe fn update_spent(node: *mut CallTreeNode) { let node = &mut *node; node.children.iter_mut().for_each(|&mut child| unsafe { + // It should be impossible for this to underflow since the amount spent + // in all child nodes is always less than or equal to the amount spent + // in the parent node. node.elem.spent -= (*child).elem.spent; update_spent(child); }); diff --git a/piecrust/src/error.rs b/piecrust/src/error.rs index dcd516b0..7d07edbc 100644 --- a/piecrust/src/error.rs +++ b/piecrust/src/error.rs @@ -22,6 +22,8 @@ pub type Compo = CompositeSerializerError< /// The error type returned by the piecrust VM. #[derive(Error, Debug)] pub enum Error { + #[error("Argument buffer overflow: {len} > {max_len}")] + ArgumentBufferOverflow { len: usize, max_len: usize }, #[error("Commit error: {0}")] CommitError(Cow<'static, str>), #[error(transparent)] diff --git a/piecrust/src/imports.rs b/piecrust/src/imports.rs index 51d6896c..d31c55db 100644 --- a/piecrust/src/imports.rs +++ b/piecrust/src/imports.rs @@ -91,7 +91,16 @@ pub fn check_ptr( ) -> Result<(), Error> { let mem_len = instance.with_memory(|mem| mem.len()); - if offset + len >= mem_len { + let end = + offset + .checked_add(len) + .ok_or(Error::MemoryAccessOutOfBounds { + offset, + len, + mem_len, + })?; + + if end >= mem_len { return Err(Error::MemoryAccessOutOfBounds { offset, len, diff --git a/piecrust/src/instance.rs b/piecrust/src/instance.rs index 8b60aa60..706deb32 100644 --- a/piecrust/src/instance.rs +++ b/piecrust/src/instance.rs @@ -263,10 +263,23 @@ impl WrappedInstance { }) } - pub(crate) fn write_bytes_to_arg_buffer(&mut self, buf: &[u8]) -> u32 { + pub(crate) fn write_bytes_to_arg_buffer( + &mut self, + buf: &[u8], + ) -> Result { self.with_arg_buf_mut(|arg_buffer| { + if buf.len() > arg_buffer.len() { + return Err(Error::MemoryAccessOutOfBounds { + offset: 0, + len: buf.len(), + mem_len: ARGBUF_LEN, + }); + } + arg_buffer[..buf.len()].copy_from_slice(buf); - buf.len() as u32 + // It is safe to cast to u32 because the length of the buffer is + // guaranteed to be less than 4GiB. + Ok(buf.len() as u32) }) } diff --git a/piecrust/src/session.rs b/piecrust/src/session.rs index d516fc81..b7e62061 100644 --- a/piecrust/src/session.rs +++ b/piecrust/src/session.rs @@ -761,7 +761,7 @@ impl Session { io: Arc::new(err), })?; - let arg_len = instance.write_bytes_to_arg_buffer(&fdata); + let arg_len = instance.write_bytes_to_arg_buffer(&fdata)?; let ret_len = instance .call(fname, arg_len, limit) .map_err(|err| {