Closed PlasmaPower closed 6 years ago
This is what I've used to check that the library is functional back when I was writing the C implementation: https://gist.github.com/Shnatsel/0c024a51b64c6e0b6c6e66f991904816
I've described how I've tested the original implementation in a blog post: https://medium.com/@shnatsel/how-ive-found-vulnerability-in-a-popular-rust-crate-and-you-can-too-3db081a67fb
I'll test it myself, including on a real-world differential fuzzing harness, and report results. Thank you!
For your testing, it may be important to note that unless you set AFL_LD_DETERMINISTIC_INIT
it'll start at a random byte. I can change this to opt-in instead of opt-out if you'd like.
I pushed some commits, and it's now working with your test program (though when your program is built in debug mode, it attempts to add with overflow causing a panic).
I wonder if AFL_LD_DETERMINISTIC_INIT
is automatically set by AFL. I'd rather not have anything happen automagically because different behavior under fuzzer and without it is incredibly frustrating.
Since fuzzing is the primary use-case, I'd rather have the non-deterministic behavior to be opt-in. It's great that it's present, though - it will help with auditing black-box binaries immensely.
What would you like the new opt-in variable to be called?
And would you like to rename the extra memory variable? I was just going with the existing env variable naming conventions I saw in the current code.
Names were inherited from libdislocator code, which was developed primarily for use with AFL. It's probably time for libdiffuzz to differentiate a bit. CONF_
should probably be replaced with LIBDIFFUZZ_
to avoid any potential clashes with other tools.
The best name I came up with is LIBDIFFUZZ_NONDETERMINISTIC
, which is probably not great, I'm open to suggestions just call it whatever you like so we don't waste time on bikeshedding.
Because the static isn't pub, I'm pretty sure namespace clashes aren't an issue there. I'll rename the environment variables.
Oh right, I mixed up AFL_LD_ALLOC_EXTRA_MEM
and CONF_
. I'll take a closer look at the code once I can think straight. Thanks again!
I also got rid of the PAGE_SIZE stuff. I'm not completely sure what the point of that was, but let me know if I should add it back in.
That's just something I've inherited from libdislocator and never really understood. I suppose it was related to the behavior of allocating an extra page and calling mprotect on it, which could interact with e.g. transparent huge pages in a funky way.
I've tested the Rust rewrite today. The results are exciting: it is noticeably faster than the C implementation, and does not produce false positives or malfunction in any discernible way.
Also, my apologies for the somewhat n00b and negative comments on the review. I wanted to poke and prod at the design to make sure it's sound, and to improve my understanding of the domain since I'll have to maintain the code going forward. I imagine that can be frustrating.
I am merging the pull request now. Sorry it took so long, and thanks again!
I haven't been able to test this with uninitialized memory yet, because malloc keeps giving me zeroed buffers (even without this
LD_PRELOAD
ed). Perhaps gcc is messing with it?This may be better suited for a separate repo, but I'm not really interested in maintaining it long term because I'm not planning on using it personally.
This also fixes all outstanding issues at the time of writing (#1 #2 #3).