brson · GitHub

What

Change Host::decode_vmslice to return an error when an out-of-bounds slice is requested.

Why

A guest may request log_from_linear_memory to log a very large slice, in which case it will preallocate a vector to hold it, potentially leading to out of memory.

This only occurs when debug logging is on.

Fixing the problem defensively in decode_vmslice also provides protection for all other callers, though from a visual inspection I don't see that other callers are obviously susceptible to this problem.

log_from_linear_memory, annotated with the bug:

    fn log_from_linear_memory(
        &self,
        vmcaller: &mut VmCaller<Host>,
        msg_pos: U32Val,
        msg_len: U32Val,
        vals_pos: U32Val,
        vals_len: U32Val,
    ) -> Result<Void, HostError> {
        if self.is_debug() {
            self.as_budget().with_free_budget(|| {
                let VmSlice { vm, pos, len } = self.decode_vmslice(vmcaller, msg_pos, msg_len)?;
                // XXX this allocation is not protected from a malicious guest XXX
                let mut msg: Vec<u8> = vec![0u8; len as usize];
                self.metered_vm_read_bytes_from_linear_memory(vmcaller, &vm, pos, &mut msg)?;
                let msg = String::from_utf8_lossy(&msg);
                let VmSlice { vm, pos, len } = self.decode_vmslice(vmcaller, vals_pos, vals_len)?;
                // XXX possibly this one too XXX
                let mut vals: Vec<RawVal> = vec![RawVal::VOID.to_raw(); len as usize];
                self.metered_vm_read_vals_from_linear_memory::<8, RawVal>(
                    vmcaller,
                    &vm,
                    pos,
                    vals.as_mut_slice(),
                    |buf| RawVal::from_payload(u64::from_le_bytes(*buf)),
                )?;
                self.log_diagnostics(&msg, &vals)
            })?;
        }
        Ok(RawVal::VOID)
    }

Known limitations

This problem is hard to detect because a single instance of the allocation as not too large for typical machines to handle, but it is a very large allocation. Subsequent code will discover that the request is out of bounds, but by that time the allocation has been made. As such, the test case here does not actually OOM without the fix, just errors in a different way.

The fuzzing harness does detect this as an oom because it has an artificial limit on how much memory can be malloced at once.

This adds a second hostile test-wasm, hostile2, to hold the test case: the hostile test cases do not create a full Soroban environment, just a basic VM, so they are not sufficient to exercise the log_from_linear_memeory syscall.

Something on main has changed just today such that I can't figure out how to regenerate the test-wasms, so this PR can't be merged presently. I am submitting for review anyway, but will seemingly have to wait until the SDK is updated to regenerate the test-wasms and be mergeable.

Read the original on github.com ↗