libafl_frida: Add tests for ASan for Unix platforms #1781

mkravchik · 2024-01-07T15:08:46Z

The PR contains a basic structure for running multiple tests and a few harness functions with different errors to be caught by ASan.
Apart from that, the draft fixes several errors found in Asan itself. Also, I disabled the stack increasing as is caused a crash on my Linux (the adjacent memory was in use). The code contains a workaround for a bug in frida-rust, that is fixed now, but the fix has not been released yet.

This is a WIP, as:

Destroying Gum causes a segmentation fault; thus, a single test (e.g., heap-uaf-write) is supported using a static Gum object. Ideally, this should be fixed, and a new Gum instance would be created for each test.
70 identical errors are reported by Asan instead of a single one.

Your comments are welcome.

…1) destroying Gum causes segmentation fault and thus a single test is supported by using a static Gum object. Ideally, this should be fixed and a new Gum instance would be created for each test. 2) 70 identical errors are reported by Asan instead of a a single one. Apart from that, the draft fixes a number of errors found in Asan

s1341 · 2024-01-07T15:29:21Z

libafl_frida/harness.cpp

+#include <string>
+
+extern "C" int heap_uaf_read(const uint8_t *_data, size_t _size) {
+  int *array = new int[100];


are you purposely using c++ allocations instead of raw mallocs?

Nothing special; more tests should be added that will cover C. I just started from code present in some other example, and it used C++.

Ok. So we should make sure to include versions of the tests which use the C api.

also, are you certain your tests aren't being optimized out?

Added several malloc-based tests and -O0 to the compilation options to make sure the code is not optimized out. We still need a way to run all the tests. Until AsanRuntime cleans up when dropped this is not possible

Ideally tests should be run by cargo test.

If you run cargo test this one will run. It is already like that

FWIW we can also add external tests in a Makefile.toml and run these with cargo make test (and CI supports it too).
Of course it's nice like it's now, but not everything may be possible that way, so mentioning it as well

I had a brief look at proper AsanRuntime uninitialization and it indeed should be done in a separate PR. I prefer to complete this one to prevent more merges as time passes. So, can we resolve it as it is now? @s1341 I'll need your help with how to shut things down properly. I initially thought of keeping the helper around and creating a new executor for each test. This does not compile due to some dependencies between the lifetime of the harness and of the helper (which I can't figure out yet).

I've figured this out, and now multiple tests are running. Please review

libafl_frida/src/executor.rs

domenukk · 2024-01-07T17:03:43Z

Left some comments about new dependencies - we try to keep these low. Feel free to ignore this until the PR is ready, then we can still clean it up :)
I also convert this to Draft for now (right?)

mkravchik · 2024-01-07T17:41:26Z

Left some comments about new dependencies - we try to keep these low. Feel free to ignore this until the PR is ready, then we can still clean it up :) I also convert this to Draft for now (right?)

Where are these comments?

domenukk

Oops forgot to publish the comments

libafl_frida/Cargo.toml

domenukk · 2024-01-07T17:01:23Z

libafl_frida/src/lib.rs

+    #[test]
+    #[cfg(unix)]
+    fn run_test_asan() {
+        env_logger::init();


libafl_bolts has a SimpleStdoutLogger that you can use to get around this dependency here

The ability to specify the trace level at runtime is very helpful. Does SimpleStdoutLogger support this?

Moved to dev-dependencies

Not sure, but for a test you don't usually see the logs anyway? For development purposes it's a different story, but I'd still prefer to not add crates to the library that are not needed in the normal case

FWIW I usually use something like this to enable trace in debug builds, but you could also make that read an env manually

if cfg!(debug_assertions) { log::set_max_level(log::LevelFilter::Trace); } else { log::set_max_level(log::LevelFilter::Info); } SimpleStdoutLogger.set_logger().unwrap()

I indeed use this while developing the test, because it is quite complicated. I can remove this altogether or switch to SimpleStdoutLogger if it is important. However, I don't see it used anywhere as of now

I've written the code using SimpleStdoutLogger, but the prints don't appear even though writing to stdout stream works...

That's somewhat concerning, you did set the LeveFilter accordingly? Do rust tests close stdout or something?

I found the problem. In cargo test, the output of successfull tests is suppressed by default, both those sent to stdout and stderr. To see the output, run cargo test -- --nocapture. Pushed the code

libafl_frida/src/lib.rs

libafl_frida/build.rs

domenukk · 2024-01-08T13:38:55Z

libafl_frida/build.rs

+    // Build the test harness
+    // clang++ -shared -fPIC -o harness.so harness.cpp
+    #[cfg(unix)]
+    std::process::Command::new("clang++")


This should potentially use the cc crate to have (some) support for other environments

I did not find a way to build a shared library with cc. If you have an idea - please let me know

cc::build::new() .flag("-shared") .flag("-fPIC")

?

Nope, this does not work. It produces a .a file and ignores the -shared. There is no linker running as well.
This is what I run:
cc::Build::new() .flag("-v") .flag("-shared") .flag("-fPIC") .flag("-O0") .file("test_harness.cpp") .compile("test_harness.so");
This is what it resulted in:
file /home/me/LibAFL/target/release/build/libafl_frida-3e26f9fa0e88b5c2/out/libtest_harness.so.a /home/me/LibAFL/target/release/build/libafl_frida-3e26f9fa0e88b5c2/out/libtest_harness.so.a: current ar archive

Hmm, sad...

Alternatively maybe we can reuse the compiler detection from libafl_cc's build.rs here?

LibAFL/libafl_cc/build.rs

Line 293 in 3d126f2

clangcpp = bindir_path.join("clang++");

Assuming clang is in the path may or may not work, I fear.
Or worst case ignore the testcase if clang wasn't found?

I could copy the code over, but am not sure it is worth the effort for the current initial state where a single platform is supported by Asan (and therefore for its test). The compiler detection in libafl_cc is quite complicated, and for UNIX eventually just checks if llvm-config is in the path and then runs it. I guess that if llvm-config is in the path, clang++ will be there as well. I can start with checking for clang presence and skip compilation if not found, then the test will be skipped if the harness is not found.

Added checking for clang and failing the test if the harness not found

It's a bit unfortunate cargo test can fail because of the PATH now, maybe we should make this optional somehow? Also, on MacOS the file extension would usually be dylib, right?
But anyway we can merge and then fix these things later

libafl_frida/harness.cpp

domenukk · 2024-01-08T13:44:09Z

Sorry for nagging, this PR is very much appreciated btw :)

…st_harness" This reverts commit 2d3494b.

mkravchik · 2024-01-09T07:36:48Z

If possible, I'd prefer to merge this without adding more features or placeholders for more platforms, as Asan doesn't support anything but Unix as of now. Postponing a merge of this just increases the size of the code to merge

tokatoka · 2024-01-09T10:57:03Z

can you run fix the clippy and run ./script/fmt_all.sh

mkravchik · 2024-01-09T13:09:14Z

I ran fmt and merged main. But clippy has nothing to do with my changes; it complains about dependencies in libafl_bolts and libafl. As I wrote above, I'm unsure how that main is in such a state.

mkravchik · 2024-01-09T13:09:34Z

I ran fmt and merged main. But clippy has nothing to do with my changes; it complains about dependencies in libafl_bolts and libafl. As I wrote above, I'm unsure how comes that main is in such a state.

tokatoka · 2024-01-09T13:12:21Z

because clippy updates bring new lints.

tokatoka · 2024-01-09T13:42:01Z

can you fix it as suggested by clippy?

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> libafl_frida/src/lib.rs:400:37
    |
400 |         let asan = AsanRuntime::new(&options);
    |                                     ^^^^^^^^ help: change this to: `options`

Also it's not formatted yet. you need to clang-format your test-harness.cpp

mkravchik · 2024-01-09T17:32:00Z

I ran clippy many times, both nightly and stable, and I don't get this error. I also looked up the GitHub builds and don't see it. What command do you run to get it?

tokatoka · 2024-01-09T17:33:07Z

i didn't run it but the log says so
https://github.com/AFLplusplus/LibAFL/actions/runs/7461926356/job/20307431026?pr=1781#step:9:652

tokatoka · 2024-01-09T18:20:19Z

https://github.com/AFLplusplus/LibAFL/actions/runs/7464790451/job/20312533545#step:9:668
there are more

mkravchik · 2024-01-09T18:22:43Z

Oh, crap. I ran scripts/clippy locally with the latest toolchain and hope this time it is ok

domenukk · 2024-01-10T00:24:24Z

Aweoms, it's green!
So is this ready then?

domenukk · 2024-01-10T00:27:53Z

All of the commented-out stuff is no longer required? Or will you bring it back?

mkravchik · 2024-01-10T04:49:26Z

I left it because it seemed that it might be useful for macos/ios when someone gets ASan working there.
From my POV - it is good to go. I also suggest the tests run as a mandatory part of CI.

domenukk · 2024-01-10T13:49:59Z

How do you mean get asan working there? It was working IIRC? cc @fabianfreyer

tokatoka · 2024-01-10T13:52:41Z

I also suggest the tests run as a mandatory part of CI.

cargo test already is a part of the CI steps

mkravchik · 2024-01-10T14:58:44Z

How do you mean get asan working there? It was working IIRC? cc @fabianfreyer

Maybe it's my misunderstanding, I indeed see some indications of this support, but have no way of testing

mkravchik · 2024-01-11T11:26:20Z

So, will this be merged?

tokatoka · 2024-01-11T12:12:14Z

libafl_frida/src/alloc.rs

        let shadow_mapping_start = map_to_shadow!(self, start);

        let shadow_start = self.round_down_to_page(shadow_mapping_start);
+        // I'm not sure this works as planned. The same address appearing as start and end is mapped to


it's not about the code you did but do you know why create a new mappings here in this function?

after we find the suitable bit in init() then the shadow region is mapped and then we are good and ready to start fuzzing right?
I don't get the purpose of this function.

ah ok i understand now it's like a primitive malloc

domenukk · 2024-01-11T15:29:39Z

I'm still uneasy about the removed parts for MacOS..

domenukk · 2024-01-11T15:36:07Z

It definitely used to work. Can we make sure it still works? @tokatoka did you look at the comments?
mkravchik@c879a0a

domenukk · 2024-01-11T15:43:43Z

Any functional reasons you removed the code or just refactoring?

mkravchik · 2024-01-11T18:00:30Z

Yes - the code was crashing on linux. It tries to extend the stack beyond the originally allocated region. But in all cases I tested there was another allocated region adjacent to it so it was not possible just to stretch the stack over. Maybe on Mac it makes some sense, but I haven't work on Mac for some years and don't have one at hand to test. There were no comments to explain why this was necessary in the first place and it worked correctly with the original stack so I commented it out hoping that whoever wrote this will react.

domenukk · 2024-01-12T02:47:24Z

Ok then, I guess we'll keep it like this and have to hope someone fixes MacOS again (don't have time myself)

mkravchik added 2 commits January 7, 2024 16:52

Fmt fixes

ee7ffd2

s1341 reviewed Jan 7, 2024

View reviewed changes

domenukk marked this pull request as draft January 7, 2024 17:03

domenukk reviewed Jan 7, 2024

View reviewed changes

mkravchik added 2 commits January 8, 2024 12:12

PR comments addressed

4757c5c

Not crashing upon Asan errors while testing

5d2d62b

tokatoka reviewed Jan 8, 2024

View reviewed changes

libafl_frida/build.rs Show resolved Hide resolved

domenukk reviewed Jan 8, 2024

View reviewed changes

libafl_frida/harness.cpp Outdated Show resolved Hide resolved

mkravchik added 5 commits January 8, 2024 19:20

More PR comments: removing env_logger, renaming harness to test_harness

2d3494b

Revert "More PR comments: removing env_logger, renaming harness to te…

e275d2f

…st_harness" This reverts commit 2d3494b.

More PR comments: removing env_logger, renaming harness to test_harness

e710c64

Checking for clang presence and failing the test if harness not found

0d6ce1e

Fmt

d010538

mkravchik added 3 commits January 9, 2024 13:53

Running multiple Asan tests

6b0a1c6

Merge remote-tracking branch 'upstream/main' into pr/libafl-frida-test

490042b

Cpp Fmt

18fc0cd

clang-format

10bf1e6

mkravchik added 2 commits January 9, 2024 19:37

More clippy complaints and Apple compilation

3485cfa

Last clippy complaints (ran scripts/clippy.sh)

4a62c24

mkravchik added 2 commits January 9, 2024 21:08

Fixing unused MacOS function

1cd14cc

Fixing unused MacOS imports

254b093

tokatoka marked this pull request as ready for review January 10, 2024 13:52

domenukk changed the title ~~First draft of a Asan tests. As of now, unix-only. This is a WIP.~~ LibAFL_frida: Add tests for ASan for Unix platforms Jan 10, 2024

domenukk changed the title ~~LibAFL_frida: Add tests for ASan for Unix platforms~~ libafl_frida: Add tests for ASan for Unix platforms Jan 10, 2024

tokatoka reviewed Jan 11, 2024

View reviewed changes

tokatoka merged commit 6a72f8a into AFLplusplus:main Jan 11, 2024

mkravchik deleted the pr/libafl-frida-test branch January 11, 2024 12:27

libafl_frida: Add tests for ASan for Unix platforms #1781

libafl_frida: Add tests for ASan for Unix platforms #1781

Conversation

mkravchik commented Jan 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkravchik Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jan 7, 2024

mkravchik commented Jan 7, 2024

domenukk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jan 8, 2024

mkravchik commented Jan 9, 2024

tokatoka commented Jan 9, 2024

mkravchik commented Jan 9, 2024

mkravchik commented Jan 9, 2024 • edited Loading

tokatoka commented Jan 9, 2024

tokatoka commented Jan 9, 2024

mkravchik commented Jan 9, 2024

tokatoka commented Jan 9, 2024

tokatoka commented Jan 9, 2024

mkravchik commented Jan 9, 2024

domenukk commented Jan 10, 2024

domenukk commented Jan 10, 2024

mkravchik commented Jan 10, 2024 • edited Loading

domenukk commented Jan 10, 2024

tokatoka commented Jan 10, 2024

mkravchik commented Jan 10, 2024

mkravchik commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenukk commented Jan 11, 2024

domenukk commented Jan 11, 2024

domenukk commented Jan 11, 2024

mkravchik commented Jan 11, 2024

domenukk commented Jan 12, 2024

mkravchik Jan 9, 2024 •

edited

Loading

domenukk Jan 8, 2024 •

edited

Loading

mkravchik commented Jan 9, 2024 •

edited

Loading

mkravchik commented Jan 10, 2024 •

edited

Loading