Skip to content

libafl_frida: Add tests for ASan for Unix platforms #1781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jan 11, 2024
Merged

libafl_frida: Add tests for ASan for Unix platforms #1781

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
6a5de62
First draft of a Asan tests. As of now, unix-only. This is a WIP, as …
mkravchik Jan 7, 2024
ee7ffd2
Fmt fixes
mkravchik Jan 7, 2024
4757c5c
PR comments addressed
mkravchik Jan 8, 2024
5d2d62b
Not crashing upon Asan errors while testing
mkravchik Jan 8, 2024
2d3494b
More PR comments: removing env_logger, renaming harness to test_harness
mkravchik Jan 8, 2024
e275d2f
Revert "More PR comments: removing env_logger, renaming harness to te…
mkravchik Jan 8, 2024
e710c64
More PR comments: removing env_logger, renaming harness to test_harness
mkravchik Jan 9, 2024
0d6ce1e
Checking for clang presence and failing the test if harness not found
mkravchik Jan 9, 2024
d010538
Fmt
mkravchik Jan 9, 2024
6b0a1c6
Running multiple Asan tests
mkravchik Jan 9, 2024
490042b
Merge remote-tracking branch 'upstream/main' into pr/libafl-frida-test
mkravchik Jan 9, 2024
18fc0cd
Cpp Fmt
mkravchik Jan 9, 2024
10bf1e6
clang-format
mkravchik Jan 9, 2024
3485cfa
More clippy complaints and Apple compilation
mkravchik Jan 9, 2024
4a62c24
Last clippy complaints (ran scripts/clippy.sh)
mkravchik Jan 9, 2024
1cd14cc
Fixing unused MacOS function
mkravchik Jan 9, 2024
254b093
Fixing unused MacOS imports
mkravchik Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libafl_frida/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ log = "0.4.20"
mmap-rs = "0.6.0"

yaxpeax-arch = "0.2.7"
clap_builder = "4.0"
env_logger = "0.10.0"
libloading = "0.7"
lazy_static = "1.4"

[dev-dependencies]
serial_test = { version = "2", default-features = false, features = ["logging"] }
12 changes: 12 additions & 0 deletions libafl_frida/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,16 @@ fn main() {
// Force linking against libc++
#[cfg(unix)]
println!("cargo:rustc-link-lib=dylib=c++");

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.arg("-shared")
.arg("-fPIC")
.arg("-o")
.arg("harness.so")
.arg("harness.cpp")
.status()
.expect("Failed to build runtime");
}
33 changes: 33 additions & 0 deletions libafl_frida/harness.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include <stdint.h>
#include <stdlib.h>
#include <string>

extern "C" int heap_uaf_read(const uint8_t *_data, size_t _size) {
int *array = new int[100];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally tests should be run by cargo test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mkravchik mkravchik Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

delete[] array;
fprintf(stdout, "%d\n", array[5]);
return 0;
}

extern "C" int heap_uaf_write(const uint8_t *_data, size_t _size) {
int *array = new int[100];
delete[] array;
array[5] = 1;
return 0;
}

extern "C" int heap_oob_read(const uint8_t *_data, size_t _size) {
int *array = new int[100];
fprintf(stdout, "%d\n", array[100]);
return 0;
}

extern "C" int heap_oob_write(const uint8_t *_data, size_t _size) {
int *array = new int[100];
array[100] = 1;
return 0;
}
extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
// abort();
return 0;
}
104 changes: 79 additions & 25 deletions libafl_frida/src/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,20 @@ impl Allocator {
end: usize,
unpoison: bool,
) -> (usize, usize) {
// log::trace!("start: {:x}, end {:x}, size {:x}", start, end, end - start);

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// different addresses.
let shadow_end = self.round_up_to_page((end - start) / 8) + self.page_size + shadow_start;
log::trace!(
"map_shadow_for_region start: {:x}, end {:x}, size {:x}, shadow {:x}-{:x}",
start,
end,
end - start,
shadow_start,
shadow_end
);
if self.pre_allocated_shadow_mappings.is_empty() {
for range in self.shadow_pages.gaps(&(shadow_start..shadow_end)) {
/*
Expand All @@ -401,28 +409,46 @@ impl Allocator {
self.mappings.insert(range.start, mapping);
}

log::trace!("adding shadow pages {:x} - {:x}", shadow_start, shadow_end);
self.shadow_pages.insert(shadow_start..shadow_end);
} else {
let mut new_shadow_mappings = Vec::new();
for range in self.shadow_pages.gaps(&(shadow_start..shadow_end)) {
for ((start, end), shadow_mapping) in &mut self.pre_allocated_shadow_mappings {
if *start <= range.start && range.start < *start + shadow_mapping.len() {
for gap in self.shadow_pages.gaps(&(shadow_start..shadow_end)) {
for ((pa_start, pa_end), shadow_mapping) in &mut self.pre_allocated_shadow_mappings
{
if *pa_start <= gap.start && gap.start < *pa_start + shadow_mapping.len() {
log::trace!("pa_start: {:x}, pa_end {:x}, gap.start {:x}, shadow_mapping.ptr {:x}, shadow_mapping.len {:x}",
*pa_start, *pa_end, gap.start, shadow_mapping.as_ptr() as usize, shadow_mapping.len());

// Split the preallocated mapping into two parts, keeping the
// part before the gap and returning the part starting with the gap as a new mapping
let mut start_mapping =
shadow_mapping.split_off(range.start - *start).unwrap();
let end_mapping = start_mapping
.split_off(range.end - (range.start - *start))
.unwrap();
new_shadow_mappings.push(((range.end, *end), end_mapping));
shadow_mapping.split_off(gap.start - *pa_start).unwrap();

// Split the new mapping into two parts,
// keeping the part holding the gap and returning the part starting after the gap as a new mapping
let end_mapping = start_mapping.split_off(gap.end - gap.start).unwrap();

//Push the new after-the-gap mapping to the list of mappings to be added
new_shadow_mappings.push(((gap.end, *pa_end), end_mapping));

// Insert the new gap mapping into the list of mappings
self.mappings
.insert(range.start, start_mapping.try_into().unwrap());
.insert(gap.start, start_mapping.try_into().unwrap());

break;
}
}
}
for new_shadow_mapping in new_shadow_mappings {
log::trace!(
"adding pre_allocated_shadow_mappings and shadow pages {:x} - {:x}",
new_shadow_mapping.0 .0,
new_shadow_mapping.0 .1
);
self.pre_allocated_shadow_mappings
.insert(new_shadow_mapping.0, new_shadow_mapping.1);

self.shadow_pages
.insert(new_shadow_mapping.0 .0..new_shadow_mapping.0 .1);
}
Expand Down Expand Up @@ -493,7 +519,7 @@ impl Allocator {
let start = area.as_ref().unwrap().start();
let end = area.unwrap().end();
occupied_ranges.push((start, end));
log::trace!("{:x} {:x}", start, end);
// log::trace!("Occupied {:x} {:x}", start, end);
let base: usize = 2;
// On x64, if end > 2**48, then that's in vsyscall or something.
#[cfg(all(unix, target_arch = "x86_64"))]
Expand Down Expand Up @@ -527,28 +553,56 @@ impl Allocator {
let addr: usize = 1 << try_shadow_bit;
let shadow_start = addr;
let shadow_end = addr + addr + addr;

let mut good_candidate = true;
// check if the proposed shadow bit overlaps with occupied ranges.
for (start, end) in &occupied_ranges {
// log::trace!("{:x} {:x}, {:x} {:x} -> {:x} - {:x}", shadow_start, shadow_end, start, end,
// shadow_start + ((start >> 3) & ((1 << (try_shadow_bit + 1)) - 1)),
// shadow_start + ((end >> 3) & ((1 << (try_shadow_bit + 1)) - 1))
// );
if (shadow_start <= *end) && (*start <= shadow_end) {
log::trace!("{:x} {:x}, {:x} {:x}", shadow_start, shadow_end, start, end);
log::warn!("shadow_bit {try_shadow_bit:x} is not suitable");
good_candidate = false;
break;
}
//check that the entire range's shadow is within the candidate shadow memory space
if (shadow_start + ((start >> 3) & ((1 << (try_shadow_bit + 1)) - 1))
> shadow_end)
|| (shadow_start + (((end >> 3) & ((1 << (try_shadow_bit + 1)) - 1)) + 1)
> shadow_end)
{
log::warn!(
"shadow_bit {try_shadow_bit:x} is not suitable (shadow out of range)"
);
good_candidate = false;
break;
}
}

if let Ok(mapping) = MmapOptions::new(1 << (*try_shadow_bit + 1))
.unwrap()
.with_flags(MmapFlags::NO_RESERVE)
.with_address(addr)
.reserve_mut()
{
shadow_bit = (*try_shadow_bit).try_into().unwrap();

log::warn!("shadow_bit {shadow_bit:x} is suitable");
self.pre_allocated_shadow_mappings
.insert((addr, (addr + (1 << shadow_bit))), mapping);
break;
if good_candidate {
// We reserve the shadow memory space of size addr*2, but don't commit it.
if let Ok(mapping) = MmapOptions::new(1 << (*try_shadow_bit + 1))
.unwrap()
.with_flags(MmapFlags::NO_RESERVE)
.with_address(addr)
.reserve_mut()
{
shadow_bit = (*try_shadow_bit).try_into().unwrap();

log::warn!("shadow_bit {shadow_bit:x} is suitable");
log::trace!(
"adding pre_allocated_shadow_mappings {:x} - {:x} with size {:}",
addr,
(addr + (1 << (shadow_bit + 1))),
mapping.len()
);

self.pre_allocated_shadow_mappings
.insert((addr, (addr + (1 << (shadow_bit + 1)))), mapping);
break;
}
log::warn!("shadow_bit {try_shadow_bit:x} is not suitable - failed to allocate shadow memory");
}
}
}
Expand Down
Loading