`AbiBuffer::advance` is not exception safe, leading to use after free

[Evian-Zhang]

Hello, we are security researchers targeting Rust security. By running our tool in this repository, we found a soundness issue. This report is written by 100% human. We promise that all you read will never be generated by LLM. Moreover, we are responsible, so if you confirm these issues do exist, we are happy to file PRs to fix it.

The problem happens here:

wit-bindgen/crates/guest-rust/src/rt/async_support/abi_buffer.rs

Lines 122 to 145 in f1e5557

	/// Advances this buffer by `amt` items.
	///
	/// This signals that `amt` items are no longer going to be yielded from
	/// `abi_ptr_and_len`. Additionally this will perform any deallocation
	/// necessary for the starting `amt` items in this list.
	pub(crate) fn advance(&mut self, amt: usize) {
	assert!(amt + self.cursor <= self.rust_storage.len());
	if !self.ops.contains_lists() {
	self.cursor += amt;
	return;
	}
	let (mut ptr, len) = self.abi_ptr_and_len();
	assert!(amt <= len);
	for _ in 0..amt {
	// SAFETY: we're managing the pointer passed to `dealloc_lists` and
	// it was initialized with a `lower`, and then the pointer
	// arithmetic should all be in-bounds.
	unsafe {
	self.ops.dealloc_lists(ptr.cast_mut());
	ptr = ptr.add(self.ops.elem_layout().size());
	}
	}
	self.cursor += amt;
	}

In line 140, self.ops.dealloc_lists is called to free the pointers, and in line 144, self.cursor is updated accordingly to avoid use of those freed pointers. However, self.ops.dealloc_lists will call a user-provided callback function dealloc_lists registered via StreamVtable (line 93):

wit-bindgen/crates/guest-rust/src/rt/async_support/stream_support.rs

Lines 73 to 113 in f1e5557

	pub struct StreamVtable<T> {
	/// The in-memory canonical ABI layout of a single value of `T`.
	pub layout: Layout,
	/// An optional callback where if provided will lower an owned `T` value
	/// into the `dst` pointer.
	///
	/// If this is called the ownership of all of `T`'s lists and resources are
	/// passed to `dst`, possibly by reallocating if `T`'s layout differs from
	/// the canonical ABI layout.
	///
	/// If this is `None` then it means that `T` has the same layout in-memory
	/// in Rust as it does in the canonical ABI. In such a situation the
	/// lower/lift operation can be dropped.
	pub lower: Option<unsafe fn(value: T, dst: *mut u8)>,
	/// Callback used to deallocate any owned lists in `dst` after a value has
	/// been successfully sent along a stream.
	///
	/// `None` means that `T` has no lists internally.
	pub dealloc_lists: Option<unsafe fn(dst: *mut u8)>,
	/// Dual of `lower`, and like `lower` if this is missing then it means that
	/// `T` has the same in-memory representation in Rust and the canonical ABI.
	pub lift: Option<unsafe fn(dst: *mut u8) -> T>,
	/// The raw `stream.write` intrinsic.
	pub start_write: unsafe extern "C" fn(stream: u32, val: *const u8, amt: usize) -> u32,
	/// The raw `stream.read` intrinsic.
	pub start_read: unsafe extern "C" fn(stream: u32, val: *mut u8, amt: usize) -> u32,
	/// The raw `stream.cancel-write` intrinsic.
	pub cancel_write: unsafe extern "C" fn(stream: u32) -> u32,
	/// The raw `stream.cancel-read` intrinsic.
	pub cancel_read: unsafe extern "C" fn(stream: u32) -> u32,
	/// The raw `stream.drop-writable` intrinsic.
	pub drop_writable: unsafe extern "C" fn(stream: u32),
	/// The raw `stream.drop-readable` intrinsic.
	pub drop_readable: unsafe extern "C" fn(stream: u32),
	/// The raw `stream.new` intrinsic.
	pub new: unsafe extern "C" fn() -> u64,
	}

So if user deliberately designs a dealloc_lists, which will panic after freeing certain lists, then self.cursor will not be updated, while some of the lists have been freed. If user recovers from the panic with std::panic::catch_unwind, then self.cursor can be used to access freed memory.

To fix it, I think we can move the self.cursor updating before the for-loop, or update it repeatedly in the for-loop.

[alexcrichton]

Thanks for the report, and this seems real enough that a decision should be made one way or another about this but also minor enough that I don't think it's worth going too too far out of our way to solve this. For exampel dealloc_lists is already and unsafe function on an unsafe trait which means we could pretty easily document "don't panic" as part of the safety contract. Alternatively I think updating self.cursor within the for-loop is also fine. In general panic-safety hasn't been a huge concern on wasm since panics aren't recoverable, but that'll change one day so seems good to prep for that.

If you'd like to make a PR to incrementally update the for-loop I'd be happy to review that!

[Evian-Zhang]

Thank you for quick confirmation! I have filed a PR #1649 to fix this.

I wonder if it is OK that I file a RustSec advisory for this soundness issue, as there are already some exception-safety-related RustSec advisories (like 1, 2, 3). It's totally fine if you think it's not OK (as you said, in wasm panics aren't recoverable), and I will do nothing if you say so.

[alexcrichton]

Personally I'd prefer to not have a RustSec advisory because the soundness issue isn't actually exploitable on wasm today. It requires off-by-default and unstable features in Rust, so while this is a worthwhile completeness fix I don't think there's any need to alert anyone of anything as there's no realistic way to exploit it.

[Evian-Zhang]

OK. Thank you for your time!