Skip to content
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

ci(x509): smoke test x509 via fuzzing #446

Merged
merged 1 commit into from
Feb 28, 2022
Merged

Conversation

bstrie
Copy link
Contributor

@bstrie bstrie commented Feb 23, 2022

This adds fuzzing targets to the x509 crate that make use of libfuzzer (via cargo-fuzz) to fuzz types whose parsers are expected to be exposed to untrusted inputs: x509::request::{CertReq, CertReqInfo}.

On their own, the fuzzing targets can be run manually via cargo fuzz run <name of binary>. Anyone hunting for parser bugs in this crate needs only to fire up the fuzzer and let it run for as long as they like.

This commit also adds a CI action that runs each fuzzer for 30 seconds. This action serves as a smoke test to provide a basic degree of confidence in the quality of any PR that touches this crate.

Note: this PR is absolutely going to fail CI for now. It turns out that the parsers in question have a number of panics that are quite trivial to surface. The fact that it's so easy to crash these parsers should hopefully demonstrate the value of these smoke tests. However, obviously this should not be merged until these panics are fixed. :P

@bstrie
Copy link
Contributor Author

bstrie commented Feb 23, 2022

As expected, fuzzing has caused CertReq::try_from to panic. The problematic input:

[48, 130, 3, 9, 48, 130, 1, 241, 2, 100, 49, 1, 48, 0, 20, 48, 18, 6, 3, 85, 4, 3, 12, 11, 101, 120, 97, 109, 112, 108, 101, 46, 99, 111, 109, 49, 20, 48, 18, 6, 3, 85, 4, 7, 12, 11, 76, 111, 115, 32, 65, 110, 103, 101, 110, 101, 115, 49, 19, 48, 17, 6, 3, 85, 4, 8, 12, 10, 67, 97, 108, 105, 102, 111, 114, 110, 105, 97, 49, 20, 48, 18, 6, 3, 85, 4, 10, 12, 11, 69, 120, 97, 109, 112, 108, 101, 32, 73, 110, 99, 49, 11, 48, 9, 6, 3, 2, 19, 6, 85, 4, 85, 83, 48, 130, 1, 34, 48, 13, 6, 9, 42, 134, 72, 134, 247, 13, 1, 1, 1, 5, 0, 3, 130, 1, 15, 0, 48, 130, 1, 48, 2, 130, 1, 1, 0, 191, 89, 247, 254, 113, 109, 222, 71, 199, 53, 121, 202, 132, 110, 250, 141, 48, 171, 54, 18, 224, 214, 165, 36, 32, 74, 114, 202, 142, 80, 201, 244, 89, 81, 61, 240, 215, 51, 49, 190, 211, 215, 162, 218, 122, 54, 39, 25, 228, 113, 238, 106, 157, 135, 130, 125, 16, 36, 237, 68, 96, 90, 185, 180, 143, 59, 128, 140, 94, 23, 59, 159, 62, 196, 0, 61, 87, 241, 113, 132, 137, 245, 199, 160, 66, 28, 70, 251, 213, 39, 164, 240, 215, 51, 49, 190, 211, 215, 162, 218, 122, 54, 39, 25, 228, 113, 238, 106, 157, 135, 130, 125, 16, 36, 237, 68, 96, 90, 185, 180, 143, 59, 128, 140, 94, 23, 59, 159, 62, 196, 0, 61, 87, 241, 113, 132, 137, 245, 199, 160, 66, 28, 70, 251, 213, 39, 164, 10, 180, 186, 107, 157, 177, 106, 84, 93, 30, 207, 110, 42, 86, 51, 189, 128, 89, 78, 186, 74, 254, 231, 31, 99, 225, 211, 87, 198, 78, 154, 63, 246, 184, 55, 70, 168, 133, 195, 115, 243, 82, 121, 135, 228, 194, 180, 175, 127, 228, 212, 234, 22, 64, 94, 94, 21, 40, 93, 217, 56, 130, 58, 161, 142, 38, 52, 186, 254, 132, 122, 118, 28, 175, 171, 176, 64, 29, 63, 160, 58, 7, 169, 208, 151, 203, 176, 199, 113, 86, 204, 254, 54, 19, 29, 173, 241, 193, 9, 194, 130, 57, 114, 240, 175, 33, 163, 95, 53, 142, 120, 131, 4, 192, 199, 139, 149, 23, 57, 217, 31, 171, 255, 208, 122, 168, 205, 79, 105, 116, 107, 61, 14, 180, 88, 116, 105, 249, 211, 159, 79, 189, 199, 97, 32, 13, 251, 39, 218, 246, 149, 98, 49, 29, 139, 25, 27, 126, 239, 170, 226, 248, 214, 246, 184, 55, 70, 168, 133, 195, 115, 243, 82, 121, 135, 228, 194, 180, 175, 127, 228, 212, 234, 22, 64, 94, 94, 21, 40, 93, 217, 56, 130, 58, 161, 142, 38, 52, 186, 254, 132, 122, 118, 28, 175, 171, 176, 64, 29, 63, 160, 58, 7, 169, 208, 151, 203, 176, 199, 113, 86, 204, 254, 54, 19, 29, 173, 241, 193, 9, 194, 130, 57, 240, 163, 114, 95, 175, 33, 53, 142, 120, 131, 4, 192, 199, 139, 149, 23, 57, 217, 31, 171, 255, 208, 122, 168, 205, 79, 105, 116, 107, 61, 14, 180, 88, 116, 105, 249, 211, 159, 79, 2, 48, 0, 48, 11, 6, 3, 85, 29, 15, 4, 4, 3, 2, 5, 160, 48, 29, 6, 3, 85, 29, 37, 4, 22, 48, 20, 6, 8, 43, 6, 1, 5, 5, 7, 3, 1, 6, 8, 43, 6, 1, 5, 5, 7, 3, 2, 48, 22, 6, 3, 85, 29, 17, 4, 15, 48, 35, 130, 11, 101, 120, 97, 109, 112, 108, 101, 46, 99, 111, 109, 48, 13, 6, 9, 42, 134, 72, 134, 247, 13, 1, 1, 11, 5, 0, 3, 130, 1, 1, 0, 43, 5, 60, 254, 129, 198, 84, 33, 118, 189, 112, 179, 115, 165, 252, 141, 197, 7, 136, 201, 194, 52, 19, 39, 3, 41, 10, 49, 172, 39, 0, 229, 83, 57, 89, 2, 38, 213, 229, 130, 236, 97, 134, 152, 98, 118, 159, 216, 91, 69, 242, 135, 255, 221, 109, 181, 48, 153, 93, 49, 249, 77, 125, 44, 38, 239, 63, 72, 161, 130, 195, 2, 108, 198, 152, 243, 130, 167, 47, 26, 17, 227, 198, 137, 149, 48, 85, 218, 192, 223, 235, 233, 205, 177, 99, 202, 58, 243, 63, 252, 77, 160, 246, 184, 75, 157, 124, 221, 67, 33, 15, 255, 255, 255, 197, 40, 222, 255, 151, 21, 255, 217, 212, 115, 10]

@bstrie bstrie marked this pull request as draft February 23, 2022 15:19
@tarcieri
Copy link
Member

Can you rebase now that #445 is merged?

@bstrie bstrie changed the title ci(pkcs10): smoke test pkcs10 via fuzzing ci(x509): smoke test x509 via fuzzing Feb 24, 2022
@tarcieri
Copy link
Member

tarcieri commented Feb 24, 2022

I managed to reproduce the panic and get the stack trace:

---- decode_rsa_2048_der stdout ----
thread 'decode_rsa_2048_der' panicked at 'source slice length (100) does not match destination slice length (1)', der/src/asn1/integer/uint.rs:33:45
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/panicking.rs:116:14
   2: core::slice::<impl [T]>::copy_from_slice::len_mismatch_fail
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/slice/mod.rs:3205:13
   3: core::slice::<impl [T]>::copy_from_slice
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/slice/mod.rs:3212:13
   4: der::asn1::integer::uint::decode_to_array
             at ../der/src/asn1/integer/uint.rs:33:5
   5: der::asn1::integer::<impl der::value::DecodeValue for u8>::decode_value
             at ../der/src/asn1/integer.rs:80:54
   6: <x509::request::Version as der::value::DecodeValue>::decode_value
             at ./src/request.rs:12:45
   7: <T as der::decodable::Decodable>::decode
             at ../der/src/decodable.rs:34:9
   8: der::decoder::Decoder::decode
             at ../der/src/decoder.rs:54:9
   9: <x509::request::CertReqInfo as der::value::DecodeValue>::decode_value::{{closure}}
             at ./src/request.rs:32:39
  10: der::asn1::sequence::SequenceRef::decode_body
             at ../der/src/asn1/sequence.rs:72:22
  11: <x509::request::CertReqInfo as der::value::DecodeValue>::decode_value
             at ./src/request.rs:32:39

It's failing on parsing x509::request::Version, or more specifically inside of der::asn1::integer::uint::decode_to_array which is being called with a bogus length.

The implementation is not validating that there is sufficient space in the output buffer when attempting to add leading zeroes. So this is ultimately a bug in the der crate's integer parser.

Edit: prospective fix in #447

tarcieri added a commit that referenced this pull request Feb 24, 2022
The previous implementation used `saturating_sub` rather than
`checked_sub` to compute the number of leading zeroes to use, which
would cause a panic if the input exceeded the output (see #446).

This commit switches to `checked_sub`, returning `ErrorKind::Length` in
the event the output buffer is too small for the given input. It also
adds unit tests for this behavior as well as the happy paths.
tarcieri added a commit that referenced this pull request Feb 24, 2022
The previous implementation used `saturating_sub` rather than
`checked_sub` to compute the number of leading zeroes to use, which
would cause a panic if the input exceeded the output (see #446).

This commit switches to `checked_sub`, returning `ErrorKind::Length` in
the event the output buffer is too small for the given input. It also
adds unit tests for this behavior as well as the happy paths.
@tarcieri
Copy link
Member

@bstrie if you rebase, #447 should ensure you get a der::Result::Err for that input instead of a panic

This adds fuzzing targets to the x509 crate that make use of libfuzzer
(via cargo-fuzz) to fuzz types whose parsers are expected to be exposed
to untrusted inputs: x509::request::{CertReq, CertReqInfo}.

On their own, the fuzzing targets can be run manually via
`cargo fuzz run <name of binary>`. Anyone hunting for parser bugs in this
crate needs only to fire up the fuzzer and let it run for as long as they like.

This commit also adds a CI action that runs each fuzzer for 30 seconds.
This action serves as a smoke test to provide a basic degree of confidence in
the quality of any PR that touches this crate.
@bstrie bstrie marked this pull request as ready for review February 25, 2022 17:39
@bstrie
Copy link
Contributor Author

bstrie commented Feb 25, 2022

@tarcieri I actually had a patch fixing the bug on my end, but I was letting the fuzzer run on it for a few hours to see if it would find anything else and you beat me to the punch. :) In any case, this looks fairly solid now. How do you feel about having this run in CI? I'm happy to increase or reduce the time as you see fit; they're your CI minutes, after all. :P

@tarcieri
Copy link
Member

@bstrie might be interesting to e.g. run on a schedule so it doesn’t block the normal CI flow, but in general it seems like a good idea

@npmccallum
Copy link
Contributor

@bstrie I think fuzzing both CertReq and CertReqInfo is redundant (just do the former). OTOH, we probably want fuzzing on Certificate.

@tarcieri
Copy link
Member

Will go ahead and land this.

If it turns out to take up too much CI time, we can convert it to a scheduled job.

@tarcieri tarcieri merged commit 46ed300 into RustCrypto:master Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants