rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.77k stars 178 forks source link

Rhai is unsound (use after free) #894

Open devzf opened 3 months ago

devzf commented 3 months ago

Consider the following example:

fn main() {
    let global_name: std::rc::Rc<std::cell::RefCell<&'static str>> = Default::default();
    let global_name2 = global_name.clone();
    {
        let mut engine = rhai::Engine::new();

        engine.register_fn("hello", move |name: &'static str| {
            *global_name.borrow_mut() = name;
        });

        engine.eval::<()>(r#"hello("some name")"#).unwrap();
    }

    println!("{:?}", &*global_name2.borrow());
}

rhai v1.19.0

schungx commented 3 months ago

I'm outside right now but on the surface it looks legit... Why do you say therr is OB?

schungx commented 3 months ago

Ah I see it now. The 'static is throwing it off...

Sneaky...

I must have a way to distinguish between &str and &'static str...

schungx commented 3 months ago

So far I have not been able to find a way to avoid the user passing in &'static str... short of disallowing &str altogether which will seriously break code.

That's because, to the Rust compiler, lifetimes do not form part of a type's ID. Therefore, it merrily thinks that &'static str and &str are the same type.

Cerber-Ursi commented 2 months ago

Do you have any reason to allow registered functions take any non-'static value at all? It seems that this use-after-free is possible with any borrowed value, not just &str.

schungx commented 2 months ago

Well, non-'static lifetimes are OK because Rhai doesn't return any lifetime. That's why passing in a normal &str is absolutely OK because within the Rust function there is nothing you can do to abuse that reference with a lifetime -- you cannot store it in anything provided by Rhai because all the functions are + 'static.

Therefore, I'm free to assume that, as long as the reference passed into the function outlives the function call, everything is OK.

Except that if that reference is 'static, then suddenly it satisfies the Rhai function requirements and you can now store those references inside Rhai variables. That's what caused the error.

It is only with &str because that's specially treated by Rhai for special handling... it gets converted to a reference inside an ImmutableString for convenience. Other references I suppose are simply never passed anything because there is no data in Rhai that is stored with references (except for the special treatment for ImmutableString). For any other type the function is simply never called.