Improve fuzzing coverage further #2267
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
=======================================
Coverage 94.58% 94.58%
=======================================
Files 104 104
Lines 24025 24025
=======================================
Hits 22725 22725
Misses 1300 1300 ☔ View full report in Codecov by Sentry. |
ac09b45 to
d342431
Compare
djc
reviewed
Dec 11, 2024
There was a problem hiding this comment.
Very cool!
Should we also consider digging into Deterministic Simulation Testing, maybe with Antithesis? (See https://turso.tech/blog/introducing-limbo-a-complete-rewrite-of-sqlite-in-rust#can-we-match-sqlite-s-world-famous-reliability-.)
This aids manual evaluation of how deep these get.
d342431 to
53ad23d
Compare
djc
approved these changes
Dec 11, 2024
|
|
||
| fs::write( | ||
| "../fuzz/corpus/unbuffered/tls12-server.bin", | ||
| [&[0u8], &transcript.server_wrote[..]].concat(), |
There was a problem hiding this comment.
Nit: are the [..] suffixes necessary for these?
There was a problem hiding this comment.
Yeah Concat only works for slices of slices or slice of str I think? At least removing it complains:
error[E0308]: mismatched types
--> rustls-fuzzing-provider/tests/smoke.rs:20:18
|
20 | [&[0u8], transcript.server_wrote].concat(),
| ^^^^^^^^^^^^^^^^^^^^^^^ expected `&[u8; 1]`, found `Vec<u8>`
|
= note: expected reference `&[u8; 1]`
found struct `Vec<u8>`
[[0u8].as_slice(), transcript.server_wrote.as_slice()].concat(), is an alternative formulation with more words but less punctuation -- any preference?
There was a problem hiding this comment.
I prefer the punctuation for its conciseness over the words.
This is intended to be a deterministic and cryptography-free CryptoProvider, so that fuzzing can reach further into the library. Things like HMAC and hashing ignore input and produces fixed output. Signing produces fixed output, verification accepts the same fixed signature which allows clients to accept the certificate in the corpus file, and should allow libfuzzer to explore branches around there. There is a test that checks this can talk to itself, and outputs transcripts into the fuzzing corpus. This is used by the client and server fuzzing harnesses.
This means fuzzing starts at a successful full handshake.
53ad23d to
1b7a274
Compare
I was looking at Antithesis last night. It seems really cool but AFAICT it's proprietary & only available to paying customers? |
A single core would be 7k annually which might be manageable at the current funding rate, or at least good for some experiments? But also I wouldn't be surprised if we can get some sweet sponsorship if we do the integration work & publish a blog post about the process/findings. |
|
I think this gets us closer to being able to do model checking and/or deterministic simulation testing, thanks to removing a large chunk of non-determinism. I think we'd also need to control for time, but we have the interface to do that now. However, I think the model/simulator would be pretty complex if it covered the whole library's APIs. But we could start with a simpler model, such as "data written into one side's |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this PR the client and server fuzzers were pretty limited. Both of them suffered from using real cryptography: fuzzers (obviously) cannot make progress in the face of this, and perform badly if the fuzzed program is non-deterministic, so they would never be able to complete a handshake. The server one was even worse than that: it didn't have any server auth key, so the handshake would be instantly aborted after client hello handling.
This PR introduces a fuzzing-only CryptoProvider that is completely deterministic, and doesn't do any cryptography. The theory of this is fuzzing should be able to access more of the handshake. The corpus for these now contains a whole handshake, so the fuzzers start out in a more advanced position.
To quantify that, here are measurements of coverage in
src/serverorsrc/clientusing the base corpus (that is, files that are checked into the repo.)Before this PR:
After this PR:
After this PR, with 5 minutes fuzzing:
There's still future work to do here: no support for QUIC, client auth, etc.