CVE-2021-43790: Use After Free (CWE-416)
value: Lucet is a native WebAssembly compiler and runtime. There is a bug in the main branch of `lucet-runtime` affecting all versions published to crates.io that allows a use-after-free in an Instance object that could result in memory corruption, data race, or other related issues. This bug was introduced early in the development of Lucet and is present in all releases. As a result of this bug, and dependent on the memory backing for the Instance objects, it is possible to trigger a use-after-free when the Instance is dropped. Users should upgrade to the main branch of the Lucet repository. Lucet no longer provides versioned releases on crates.io. There is no way to remediate this vulnerability without upgrading.
CWE description: The product reuses or references memory after it has been freed. At some point afterward, the memory may be allocated again and saved in another pointer, while the original pointer references a location somewhere within the new allocation. Any operations using the original pointer are no longer valid because the memory "belongs" to the code that operates on the new pointer.
CVSS v3 base score: 8.1
Published: 2021-11-30T00:15Z
Repository: https://github.com/bytecodealliance/lucet
Language: Rust
Files changed: lucet-runtime/lucet-runtime-internals/src/instance.rs
Commit date: 2021-11-29 15:00:02 -0800
Commit message: Merge pull request from GHSA-hf79-8hjp-rrvq

* Use manual drop

* Add some comments to `ManuallyDrop` usage

* rustfmt

Co-authored-by: Aaron Turon <aturon@fastly.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Diff: diff --git a/lucet-runtime/lucet-runtime-internals/src/instance.rs b/lucet-runtime/lucet-runtime-internals/src/instance.rs
index b45e626..1252a42 100644
--- a/lucet-runtime/lucet-runtime-internals/src/instance.rs
+++ b/lucet-runtime/lucet-runtime-internals/src/instance.rs
@@ -12,11 +12,10 @@ use crate::context::Context;
 use crate::embed_ctx::CtxMap;
 use crate::error::Error;
 #[cfg(feature = "concurrent_testpoints")]
 use crate::lock_testpoints::LockTestpoints;
 use crate::module::{self, FunctionHandle, Global, GlobalValue, Module, TrapCode};
-use crate::region::RegionInternal;
 use crate::sysdeps::HOST_PAGE_SIZE_EXPECTED;
 use crate::val::{UntypedRetVal, Val};
 use crate::vmctx::Vmctx;
 use crate::WASM_PAGE_SIZE;
 use libc::{c_void, pthread_self, siginfo_t, uintptr_t};
@@ -25,10 +24,11 @@ use memoffset::offset_of;
 use std::any::Any;
 use std::cell::{BorrowError, BorrowMutError, Ref, RefCell, RefMut, UnsafeCell};
 use std::convert::TryFrom;
 use std::marker::PhantomData;
 use std::mem;
+use std::mem::ManuallyDrop;
 use std::ops::{Deref, DerefMut};
 use std::ptr::{self, NonNull};
 use std::sync::Arc;
 
 pub const LUCET_INSTANCE_MAGIC: u64 = 746_932_922;
@@ -145,24 +145,33 @@ impl Drop for InstanceHandle {
     fn drop(&mut self) {
         if self.needs_inst_drop {
             unsafe {
                 let inst = self.inst.as_mut();
 
-                // Grab a handle to the region to ensure it outlives `inst`.
+                // The `inst.alloc` field manages the memory of the instance
+                // itself. Note, though, that this field is in a `ManuallyDrop`
+                // so it won't get dropped automatically in `drop_in_place`.
+                // This is the point where we take over that precise drop.
                 //
-                // This ensures that the region won't be dropped by `inst` being
-                // dropped, which could result in `inst` being unmapped by the
-                // Region *during* drop of the Instance's fields.
-                let region: Arc<dyn RegionInternal> = inst.alloc().region.clone();
+                // By using `take` here we're basically calling `ptr::read`
+                // which "duplicates" the `alloc` since the `alloc` local
+                // variable here is the exact same as `inst.alloc`. All we do
+                // with `inst`, though, is call `drop_in_place`, which
+                // invalidates every other field in `inst`.
+                let alloc: Alloc = ManuallyDrop::take(&mut inst.alloc);
 
                 // drop the actual instance
                 std::ptr::drop_in_place(inst);
 
-                // and now we can drop what may be the last Arc<Region>. If it is
-                // it can safely do what it needs with memory; we're not running
-                // destructors on it anymore.
-                mem::drop(region);
+                // Now that we're 100% done with the instance, destructors and
+                // all, we can release the memory of the instance back to the
+                // original allocator from whence it came (be it mmap or uffd
+                // based). This will run the "official" destructor for `Alloc`
+                // which internally does the release. Note that after this
+                // operation the `inst` pointer is invalid and can no longer be
+                // used.
+                drop(alloc);
             }
         }
     }
 }
 
@@ -231,12 +240,16 @@ pub struct Instance {
 
     #[cfg(feature = "concurrent_testpoints")]
     /// Conditionally-present helpers to force permutations of possible races in testing.
     pub lock_testpoints: Arc<LockTestpoints>,
 
-    /// The memory allocated for this instance
-    alloc: Alloc,
+    /// The memory allocated for this instance.
+    ///
+    /// Note that this is in a `ManuallyDrop` because this manages the memory of
+    /// this `Instance` itself. To have precise control over this memory we
+    /// handle this in `Drop for InstanceHandle`.
+    alloc: ManuallyDrop<Alloc>,
 
     /// Handler run for signals that do not arise from a known WebAssembly trap, or that involve
     /// memory outside of the current instance.
     fatal_handler: fn(&Instance) -> !,
 
@@ -1053,11 +1066,11 @@ impl Instance {
             ctx: Context::new(),
             state: State::Ready,
             kill_state,
             #[cfg(feature = "concurrent_testpoints")]
             lock_testpoints,
-            alloc,
+            alloc: ManuallyDrop::new(alloc),
             fatal_handler: default_fatal_handler,
             c_fatal_handler: None,
             signal_handler: Box::new(signal_handler_none) as Box<SignalHandler>,
             ensure_signal_handler_installed: true,
             ensure_sigstack_installed: true,

Vulnerable code: use crate::region::RegionInternal;
                // Grab a handle to the region to ensure it outlives `inst`.
                // This ensures that the region won't be dropped by `inst` being
                // dropped, which could result in `inst` being unmapped by the
                // Region *during* drop of the Instance's fields.
                let region: Arc<dyn RegionInternal> = inst.alloc().region.clone();
                // and now we can drop what may be the last Arc<Region>. If it is
                // it can safely do what it needs with memory; we're not running
                // destructors on it anymore.
                mem::drop(region);
    /// The memory allocated for this instance
    alloc: Alloc,
            alloc,
Fixed code: use std::mem::ManuallyDrop;
                // The `inst.alloc` field manages the memory of the instance
                // itself. Note, though, that this field is in a `ManuallyDrop`
                // so it won't get dropped automatically in `drop_in_place`.
                // This is the point where we take over that precise drop.
                // By using `take` here we're basically calling `ptr::read`
                // which "duplicates" the `alloc` since the `alloc` local
                // variable here is the exact same as `inst.alloc`. All we do
                // with `inst`, though, is call `drop_in_place`, which
                // invalidates every other field in `inst`.
                let alloc: Alloc = ManuallyDrop::take(&mut inst.alloc);
                // Now that we're 100% done with the instance, destructors and
                // all, we can release the memory of the instance back to the
                // original allocator from whence it came (be it mmap or uffd
                // based). This will run the "official" destructor for `Alloc`
                // which internally does the release. Note that after this
                // operation the `inst` pointer is invalid and can no longer be
                // used.
                drop(alloc);
    /// The memory allocated for this instance.
    ///
    /// Note that this is in a `ManuallyDrop` because this manages the memory of
    /// this `Instance` itself. To have precise control over this memory we
    /// handle this in `Drop for InstanceHandle`.
    alloc: ManuallyDrop<Alloc>,
            alloc: ManuallyDrop::new(alloc),