-
Notifications
You must be signed in to change notification settings - Fork 736
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.
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
Adding tests and fuzzing to der.rs (using cargo-fuzz) #1254
base: main
Are you sure you want to change the base?
Conversation
I tried to run this on an M1 Mac Mini but I don't think that is supported yet by this tooling:
I'm getting a:
when running Maybe a speedy Linux machine is good for fuzzing. |
Hi, I'll take a look at this really soon. I just wanted to jot down my initial thinking: My current thoughts are to try to follow the pattern suggested in https://alastairreid.github.io/papers/hatra2020.pdf:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LMK what you think
fuzz_target!(|data: &[u8]| { | ||
let mut reader = untrusted::Reader::new(untrusted::Input::from(data)); | ||
let _ = ring::io::der::bit_string_with_no_unused_bits(&mut reader); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I would expect here is that the thing inside fuzz_target!
would be a function that can be shared between fuzz tests and unit known-answer tests.
Also, I want to clarify the goal here: The purpose of this test is to verify that bit_string_with_no_unused_bits
never panics or crashes, right?
|
||
fuzz_target!(|data: &[u8]| { | ||
let mut reader = untrusted::Reader::new(untrusted::Input::from(data)); | ||
let _ = ring::io::der::read_tag_and_get_value(&mut reader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
} | ||
|
||
#[test] | ||
fn test_bit_string_with_no_unused_bits() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Independently of the idea of sharing the test logic between the fuzzer and the known-answer tests, it is really important that tests in ring are written as parameterized tests with minimal boilerplate.
use bit_string_with_no_unused_bits_tests::TestCase;
const TEST_CASES: &[TestCase] = &[
// the comment for the test case.
TestCase {
input: &[0x01, 0x01, 0xff],
result: Err(error::Unspecified),
},
...
];
TEST_CASES.for_each(|TestCase { input, result, .. }| {
let mut reader = ...;
assert_eq!(bit_string_with_no_unused_bits(&mut reader), result);
});
}
} | ||
|
||
#[test] | ||
fn fuzz_bit_string_with_no_unused_bits() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this part. Maybe just incomplete?
Rust nightly isn't the end of the world for an initial stab, though we should investigate options that are most likely to work on stable.
Let's consider different goals of the fuzzing:
Not just known good input, but known inputs with known expected results. I suggest we try to find an approach that lets us use your manually-written known-answer tests as inputs to the fuzzer.
If we have to do that, we can. But it would be better, perhaps, to use a framework that lets us define a bunch of fuzzers grouped together in a single file with less boilerplate.
For fuzzing webpki I suggest you take a peek at what Google has done for fuzzing their X.509 library. I know they had to add a way to disable signature verification because otherwise the fuzzer doesn't make enough progress since it will literally never guess a valid signature. |
Also, thanks for working on this! I would suggest a humble project: Factor out the code that can construct a Tag-Length-Value encoding from a (tag, value) pair. Test that parsing such an encoding succeeds when the tag matches and the length is <= the maximum supported length, and returns |
I am curious about your thoughts on https://github.com/AltSysrq/proptest for fuzzing compared to the others you found. Maybe I'm just too biased towards it do to what I've researched so far, but it does seem like a great fit. |
Here is an experiment to add fuzzing to
der.rs
. Bunch of thoughts and notes:cargo-fuzz
andafl.rs
. I picked the first one, not for any specific reason. It may be interesting to try to implement the same withafl.rs
to see what the experience is.fuzz_bit_string_with_no_unused_bits
which may not be the best function to fuzz. I'll add some more der.rs coverage in the coming days.cargo-fuzz
, fuzzers are small standalone binaries. You basically implement a single function that the fuzzer tooling with turn into a binary that can then run in the fuzzing framework.cargo fuzz run fuzz_bit_string_with_no_unused_bits
is very nice and easy to automatetest_*
andfuzz_*
functions easily in a module's test section, but that is not possible. It is definitely possible to extract common code and share that between fuzzing and unit testing. (Although I am not familiar enough with Rust yet to understand how that would work with public/private and test namespaces.)To run this:
Let me know what you think @briansmith .. I'll play around with this a bit more. No expectation that this PR will land - consider it an experiment.
I'm going to try to be more creative with fuzzing in Ring, but I think the more interesting fuzzing should be done in WebPKI - the primitives here are probably good to be fuzzed but I have a gut feeling that it is not going to give interesting results. Fuzzing larger parsers like in WebPKI is a lot more interesting I think.