frida / frida-rust

Frida Rust bindings
Other
188 stars 53 forks source link

frida-gum Transformer callback API is unsound #102

Closed Mrmaxmeier closed 1 year ago

Mrmaxmeier commented 1 year ago

LibAFL's frida module is currently running into this issue:

use frida_gum as gum;
use frida_gum::stalker::{Stalker, Transformer};
use gum::stalker::NoneEventSink;
use lazy_static::lazy_static;

lazy_static! {
    static ref GUM: gum::Gum = unsafe { gum::Gum::obtain() };
}

fn make_transformer<'a>() -> Transformer<'a> {
    let mut arr = [0u64; 64];
    // the callback captures arr from the local scope, but arr is dangling after we return
    Transformer::from_callback(&GUM, |basic_block, _output| {
        arr.fill(0x42);
        for instr in basic_block {
            instr.keep();
        }
    })
}

fn main() {
    let transformer = make_transformer();
    let mut stalker = Stalker::new(&GUM);
    stalker.follow_me::<NoneEventSink>(&transformer, None);
    stalker.unfollow_me();
}
$ cargo run
   Compiling frida-gum v0.12.0 (/tmp/frida-rust/frida-gum)
   Compiling stalker v0.1.0 (/tmp/frida-rust/examples/gum/stalker)
    Finished dev [unoptimized + debuginfo] target(s) in 0.30s
     Running `/tmp/frida-rust/target/debug/stalker`
fish: Job 1, 'cargo run' terminated by signal SIGSEGV (Address boundary error)

As far as I can tell, the fix is simple:

diff --git a/frida-gum/src/stalker/transformer.rs b/frida-gum/src/stalker/transformer.rs
index 8183b29..2090260 100644
--- a/frida-gum/src/stalker/transformer.rs
+++ b/frida-gum/src/stalker/transformer.rs
@@ -174,7 +174,7 @@ pub struct Transformer<'a> {
 impl<'a> Transformer<'a> {
     pub fn from_callback<'b>(
         _gum: &'b Gum,
-        callback: impl FnMut(StalkerIterator, StalkerOutput),
+        callback: impl FnMut(StalkerIterator, StalkerOutput) + 'a,
     ) -> Transformer<'a>
     where
         'b: 'a,

This'll break some consumers of the API though. (i.e. the reproducer will fail to compile with something like this:)

error[E0373]: closure may outlive the current function, but it borrows `arr`, which is owned by the current function
  --> examples/gum/stalker/src/main.rs:13:38
   |
13 |     Transformer::from_callback(&GUM, |basic_block, _output| {
   |                                      ^^^^^^^^^^^^^^^^^^^^^^ may outlive borrowed value `arr`
14 |         arr.fill(0x42);
   |         --- `arr` is borrowed here
   |
note: closure is returned here
  --> examples/gum/stalker/src/main.rs:13:5
   |
13 | /     Transformer::from_callback(&GUM, |basic_block, _output| {
14 | |         arr.fill(0x42);
15 | |         for instr in basic_block {
16 | |             instr.keep();
17 | |         }
18 | |     })
   | |______^
help: to force the closure to take ownership of `arr` (and any other referenced variables), use the `move` keyword
   |
13 |     Transformer::from_callback(&GUM, move |basic_block, _output| {
   |                                      ++++