fix(config): decode urunc.json before validating mandatory fields - #886
Open
u7k4rs6 wants to merge 2 commits into
Open
fix(config): decode urunc.json before validating mandatory fields#886u7k4rs6 wants to merge 2 commits into
u7k4rs6 wants to merge 2 commits into
Conversation
Add two native fuzz targets over the urunc.json config path. Both assert that any config GetUnikernelConfig accepts carries a non-empty unikernelType, hypervisor and binary, since sixteen references in unikontainers.go read those straight out of the state annotations without rechecking them. FuzzConfigMandatoryFields supplies the four field values directly, so the fuzzer mutates the base64 payloads instead of having to build the surrounding JSON. FuzzGetUnikernelConfigJSON supplies arbitrary bytes to the same entry point to cover parsing. Signed-off-by: u7k4rs6 <utkarshbahuguna10@gmail.com>
GetUnikernelConfig validated the urunc.json config while its values were still base64 and only decoded afterwards. The base64 decoder ignores CR and LF, so a field holding only newlines is non-empty when validated and decodes to the empty string. A config whose mandatory fields are newlines was therefore accepted with all three fields empty. Map() drops empty values, so the mandatory keys never reach the state annotations and every later Get on that container fails with ErrNotUnikernel. Swap the order so that validation runs on decoded values. Add the case to the FuzzConfigMandatoryFields seed corpus as a regression test. Fixes: urunc-dev#885 Signed-off-by: u7k4rs6 <utkarshbahuguna10@gmail.com>
✅ Deploy Preview for urunc canceled.
|
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.
Description
GetUnikernelConfigvalidated theurunc.jsonconfig while its values werestill base64 encoded, and only decoded them afterwards. The base64 decoder
skips CR and LF, so a field holding only newlines is non-empty when
validate()inspects it and decodes to the empty string immediately after.The result is that a config whose three mandatory fields are newlines is
accepted with all three fields empty.
Map()skips empty values, so thosekeys never reach the state annotations, and every later
Geton the containerfails with
ErrNotUnikernel. Sixteen references inunikontainers.goreadthose annotations back without rechecking them, so
validate()is the onlygate in front of all of them.
This PR swaps the two calls so that validation runs on decoded values, and
adds fuzz coverage for the config path, which had none.
Two commits:
test(unikontainers)adds the fuzz targets on their own, so they can beread independently of the behaviour change.
fix(config)makes the change and adds the failing input to the seedcorpus as a regression test.
Related issues
validate()runs beforedecode(), so base64 fields containing only CR/LF pass validation and decode to empty #885How was this tested?
make unittestpasses.golangci-lintin the project'sv2.9container reports no issues in anyfile this PR touches. It does report 4 pre-existing issues elsewhere, in
cmd/urunc/create.goandpkg/containerd-shim/guest_rootfs.go, which areuntouched here.
testdata/fuzz/FuzzConfigMandatoryFields/newline_decodes_to_emptyfails onmainand passes with the fix, under plaingo testwith no-fuzzflag,so the regression is deterministic in normal CI.
go test -fuzz FuzzConfigMandatoryFieldsandgo test -fuzz FuzzGetUnikernelConfigJSONwere each run for 60 to 90seconds against the fix with no new failures.
One caveat worth stating plainly. An earlier version of
FuzzConfigMandatoryFieldscomposedvalidate()anddecode()directly withno filesystem access, and rediscovered this bug from neutral seeds in about
1.5 seconds. Driving the same property through
GetUnikernelConfigis themore meaningful invariant, but it costs roughly two orders of magnitude of
throughput, and in that form the fuzzer did not rediscover the input within 60
seconds. That is why the failing input is checked into the seed corpus rather
than left for the fuzzer to refind. If you would prefer the faster unit-level
target instead of, or alongside, the end-to-end one, I am happy to change it.
Scope note: only CR and LF bypass the check. Space and tab produce
illegal base64 data at input byte 0.This is the
urunc.jsonpath only. I have not touched the spec annotationpath, since 5a654d3 establishes that those annotations arrive already decoded.
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).