The two hand-written parsers that read a zone's bytes are attacked by their own unit suites - #34
Merged
Merged
Conversation
… their own unit suites The broker's request line and kryptik-wlproxy's wire decoder are the two trust boundaries in Kryptik that were written by hand. Both now have seeded mutation tests that run with everything else on every push, with no tool but cargo. The broker: real requests, one per line, in fuzz-corpus/broker-requests. Each is damaged in a hundred-odd ways and sent down a real connection with a payload that may or may not be what the header promised. No panic, nothing held past the request deadline, one well-formed reply every time. The proxy: the corpus is the protocol, one well-formed body for every message in the generated tables, damaged with the lengths steered at their edges; the decoder must not panic or overread and accepts only a body that parses exactly to its end. A header test covers the size field's edges. And a damaged opening conversation goes through a live session in fragments of arbitrary size: the session may refuse, and what it forwarded to the compositor by then must be whole messages. Neither parser broke. The generators are seeded, so a failure is the same failure everywhere; that is also the limit, and the broker design says so: this is not coverage-guided fuzzing, which needs a nightly toolchain and belongs in a scheduled job. Both suites were run three times over: 173 and 52 tests, each time.
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
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.
The roadmap's "Someone else has attacked it" asks for the two hand-written trust boundaries, the broker's protocol and
kryptik-wlproxy's wire parser, to be fuzzed in CI with a corpus kept in the tree. This is that half of the item. The other half, a review of the launch path by a person who did not write it, is not something a change can do.What it adds
Tests only, plus one seed file and the documentation. No dependency, no new tool, no CI change: they are unit tests, so the existing
Compartment layerjob runs them.broker::no_request_a_zone_can_send_breaks_the_brokerfuzz-corpus/broker-requests, each damaged about 150 ways (a flipped bit, a cut, a NUL, a number past 64 bits, 600 bytes of padding), sent down a real connection with a payload that may not match the headerprotocol::no_body_a_client_can_send_makes_the_decoder_panic_or_overreadOkonly for a body that parses exactly to its end, no string with a NULprotocol::no_eight_bytes_make_a_header_that_lies_about_its_lengthsession::whatever_a_client_sends_the_compositor_receives_only_whole_messagesEach test also checks that the generator reached both sides (some inputs accepted, some refused), so it cannot pass by refusing everything.
Result
Neither parser broke. Both suites ran three times over: 173 and 52 tests, each time. The four tests take under half a second together.
The limit, said in the docs too
The generators are seeded, so a failure is the same failure on every machine. It is mutation from a fixed seed, not coverage-guided fuzzing: it finds what a few thousand damaged inputs find. Coverage-guided runs with libFuzzer need a nightly toolchain and belong in a scheduled job, not in the build;
docs/design/broker.mdhas a section on both. An input that ever breaks either parser goes into the corpus and stays.Merge order
Independent of everything open. It touches
compartments/andcompositor/, so it starts a Distro run: not while one is in flight. When #16 and #33 are in, their verbs (time-offset, the threeupdate-*) are worth a line each in the seed file; thetime-offsetlines already there are refused as unknown verbs until then.