frida / frida-rust

Frida Rust bindings
Other
188 stars 53 forks source link

Frida-rust links multiple versions of capstone. #81

Closed WorksButNotTested closed 1 year ago

WorksButNotTested commented 1 year ago

After a long debugging session of LibAFL, it was discovered that Stalker was not working correctly as a result of being linked against multiple different versions of capstone. https://github.com/AFLplusplus/LibAFL/issues/1018. One version was as a result of dependency on the capstone-rs crate and the other as a result of linking against the frida-gum-devkit of which capstone is an embedded component (indeed it is built from a FRIDA hosted fork of the next branch of capstone). Frida-rust also makes use of capstone-rs and also introduces the same version conflict in its own right.

The proposed solution (which has also been tested in LibAFL) is to substitute the capstone-rs crate with a fork (https://github.com/domenukk/capstone-rs) built using FRIDA's fork of capstone so that the versions match. Given that frida-rust, and the native FRIDA fork of capstone are both hosted by FRIDA in github, it seems that this would be a natural place for this fork of capstone-rs to be hosted also. I've discuss this approach with @oleavr and he seems in agreement, though obviously happy to discuss if you think there is a better option.

The suggested fix is therefore:

domenukk commented 1 year ago

I think it would be even better if we get the needed changes upstreamed, see PR https://github.com/capstone-rust/capstone-rs/pull/140

meme commented 1 year ago

If we are able to get it upstreamed, that would be preferable, however I am also open to having a fork of capstone-rs inside of the Frida organization, and using it inside of the Frida Rust bindings. Alternatively, we could also maintain the capstone-rs bindings inside of this repository as well as part of the Cargo workspace in the interim.

WorksButNotTested commented 1 year ago

I’m happy either way. If there’s a long wait for capstone to upstream, it would be great if you’re able to pursue one of the other options as an interim measure.

Also if we’re in a position to move forward with the arm support (https://github.com/frida/frida-rust/pull/74), that would be really helpful as it would cut down the number of upstream changes I need to work with right now.

meme commented 1 year ago

I wonder if the patch that @domenukk supplied is sufficient, or if we should instead rewrite the Capstone bindings to use the symbols exposed from frida-gum-sys? That would ensure that there is really only 1 version of Capstone.

In the meantime I have addressed arm support. Thanks.

domenukk commented 1 year ago

The patch should be all that's needed, apart from the two todos, see https://github.com/capstone-rust/capstone-rs/pull/140/files#diff-840483b72458d4076f7d2f023cdeefce874a517435c1352a32a974988986c53eR74

I'd advise not to copy the folder into this repo, but to fork the bindings and publish the fork on crates, that way syncing new changes from upstream is less work. But either way is fine for me

WorksButNotTested commented 1 year ago

If we are able to get it upstreamed, that would be preferable, however I am also open to having a fork of capstone-rs inside of the Frida organization, and using it inside of the Frida Rust bindings. Alternatively, we could also maintain the capstone-rs bindings inside of this repository as well as part of the Cargo workspace in the interim.

Seems capstone have reservations about Frida specific changes. https://github.com/capstone-rust/capstone-rs/pull/140#pullrequestreview-1304478930. Are we able to pursue the alternatives?

domenukk commented 1 year ago

I think frida should vendor its own capstone-rs, as it also vendors its own capstone.

oleavr commented 1 year ago

I think frida should vendor its own capstone-rs, as it also vendors its own capstone.

Makes sense. Let's do it. Anyone interested in taking a stab at it?

WorksButNotTested commented 1 year ago

Most of the hard work should’ve already been done here I think.

https://github.com/domenukk/capstone-rs

Just the repo could do with being moved to FRIDAs organisation and then the dependencies need updating in Frida-rust.

meme commented 1 year ago

Hi all. I've asked Ole to create https://github.com/frida/capstone-rs so we can make some progress here.

I have reviewed Dominik's repo, but I am wondering if it is possible to avoid vendoring Frida's Capstone entirely. Specifically, I think we can simply use frida-gum-sys from Frida's fork of Capstone, then assume that Frida Rust links against libfrida-gum which includes Capstone symbols.

domenukk commented 1 year ago

The issue is that Frida's capstone added some instructions that are not supported in the upstream capstone bindings (and that they do not plan to add)

(See capstone-rs/src/arch/arm64.rs in my fork)

meme commented 1 year ago

I understand, we still need our own fork of Capstone. My point is that we do not any build machinery or bindgen at all from our fork of Capstone, because it can just use frida-gum-sys to provide Capstone definitions. Then I can apply your patches for updating the wrapper code to use the frida-gum-sys definitions (added instructions, etc.)

How does that sound? I think it would be much simpler, so I'll explore it.

domenukk commented 1 year ago

I don't fully understand, but if you think you have a simpler solution I'm all for it ;)

aviramha commented 1 year ago

I think I understood @meme idea - implemented here https://github.com/frida/frida-rust/pull/99

meme commented 1 year ago

Going to close this, as we no longer have a dependency on Capstone (other than the one that's in Frida already) https://github.com/frida/frida-rust/pull/99