rust-sciter icon indicating copy to clipboard operation
rust-sciter copied to clipboard

Unsoundness in get

Open lwz23 opened this issue 9 months ago • 2 comments

Hello, thank you for your contribution in this project, I an testing our static analysis tool in github's Rust project and I notice the following code:

pub struct AssetPtr<T> {
    ptr: *mut T,
}

impl<T> AssetPtr<T> {
    /// Attach to an existing iasset pointer without reference increment.
    fn attach(lp: *mut T) -> Self {
        assert!(!lp.is_null());
        Self {
            ptr: lp
        }
    }
    /// Attach to an iasset pointer and increment its reference count.
    pub fn adopt(lp: *mut T) -> Self {
        let mut me = Self::attach(lp);
        me.get().add_ref();
        me
    }
    /// Get as an iasset type.
    fn get(&mut self) -> &mut iasset {
        let ptr = self.ptr as *mut iasset;
        unsafe { &mut *ptr }
    }
}

The issue is that AssetPtr::adopt is a public function that accepts any type T, but internally it assumes T can be safely cast to an iasset. There's no trait bound or type checking to ensure this is valid. The problematic code path is:

adopt takes any *mut T pointer It calls get() which unsafely casts self.ptr to *mut iasset and dereferences it It then calls add_ref() on the supposed iasset

Since there's no guarantee that T is actually an iasset or has the same memory layout, this creates undefined behavior when given a pointer to an incompatible type. The function provides a safe interface (no unsafe required to call it) but can lead to memory safety violations. A valid path to call this fn: pub fn adopt -> fn get

POC

fn main() {
    // Create a struct that is NOT an iasset
    struct NotAnIAsset {
        some_field: u32,
    }
    
    // Create an instance of NotAnIAsset
    let not_iasset = Box::new(NotAnIAsset { some_field: 42 });
    
    // Convert the Box to a raw pointer
    let raw_ptr = Box::into_raw(not_iasset);
    
    // Call the public function with a pointer to something that is not an iasset
    let asset_ptr = AssetPtr::adopt(raw_ptr as *mut NotAnIAsset);
    
    // The above call will cause undefined behavior when it tries to:
    // 1. Cast the NotAnIAsset pointer to iasset in the get() method
    // 2. Call add_ref() on something that doesn't have that method
}

lwz23 avatar Mar 04 '25 11:03 lwz23

other bug: 15:24:17|RAP|INFO|: 2: Public function with direct parameter to unsafe operation: utf::u2s 15:24:17|RAP|INFO|: unsafe operations: 15:24:17|RAP|INFO|: (1) std::ffi::CStr::from_ptr(_8), 15:24:17|RAP|INFO|:

15:24:17|RAP|INFO|: 3: Public function with direct parameter to unsafe operation: utf::w2sn 15:24:17|RAP|INFO|: unsafe operations: 15:24:17|RAP|INFO|: (1) std::slice::from_raw_parts(_11, _12), 15:24:17|RAP|INFO|:

15:24:17|RAP|INFO|: 4: Public function with direct parameter to unsafe operation: utf::u2sn 15:24:17|RAP|INFO|: unsafe operations: 15:24:17|RAP|INFO|: (1) std::slice::from_raw_parts(_11, _13), 15:24:17|RAP|INFO|:

15:24:17|RAP|INFO|: Total pattern1 functions found: 4 15:24:17|RAP|INFO|: ===== 过程间污点传播分析报告 ===== 15:24:17|RAP|INFO|: 1: 过程间污点传播: utf::w2s -> utf::wcslen 15:24:17|RAP|INFO|: unsafe operations: 15:24:17|RAP|INFO|: (1) std::ptr::const_ptr::<impl *const T>::offset(_12, _13), 15:24:17|RAP|INFO|: (2) *_11, 15:24:17|RAP|INFO|:

15:24:17|RAP|INFO|: 总计发现 1 个过程间污点传播

lwz23 avatar Apr 22 '25 07:04 lwz23

https://github.com/sciter-sdk/rust-sciter/blob/789013a5353826b681c896eef489a450ece84c9c/src/utf.rs#L155 This is a similar problem, although here we check that the pointer is not null and that its length is not 0. But in practice any invalid pointer or too large a length will trigger UB.

lwz23 avatar May 25 '25 08:05 lwz23