Harden UR decoder size gates and add fuzz targets - #6
Conversation
Single-part URs had no size limit at all, and multipart fragment strings were fully allocated and CRC32'd before any bound was checked. Reject both by string length up front, before any allocation: - single-part bodies cap at the 200 KiB message envelope (MAX_SINGLE_BODY_LENGTH) - multipart fragment payloads cap at the 4096-byte fragment envelope plus CBOR/CRC overhead (MAX_MULTI_PAYLOAD_LENGTH) Add a cargo-fuzz workspace targeting the decoding attack surface: decode_parts (arbitrary strings), moving_parts (animated-QR attack scenarios: corrupted/dropped/duplicated/reordered frames, foreign UR types, crafted fountain metadata), roundtrip (encode/decode identity). ~310M executions across the targets: no panics, hangs, or OOM.
test_multi_part_rejects_oversized_fragment_string passed with the multipart gate removed: its "ae"-repeated body failed the CRC check first, so the gate was never exercised. Use CRC-valid bytewords and assert on decode_ur_part directly, which only errors when the gate fires. Both gate tests now fail if their gate is deleted. test_multi_part_max_fragment_length_within_string_gate claimed to exercise MAX_FRAGMENT_LENGTH chunks but the fountain encoder balances fragments, so a 10,000-byte payload produced 3,335-byte chunks (6,706 chars against an 8,328 gate). Size the payload so the CBOR message divides exactly into MAX_FRAGMENT_LENGTH chunks (8,236 chars) and assert every fragment string against the gate. Move craft_multipart_part and rewrite_ur_type into src/test_helpers.rs behind the new `fuzzing` feature; moving_parts.rs had a verbatim copy of the first and a reimplementation of the second. Fix frame addressing in moving_parts: FlipChar/Truncate/Extend/ Duplicate/ChangeType indexed with `parts.get(u8)` while Drop/Swap used `% parts.len()`, so on a typical 3-25 frame scan those five ops no-oped for most inputs. Map fragment_length into the encoder's range in moving_parts and roundtrip too; a raw u16 left ~94% of executions starting from an empty frame list. Same 300k runs from an empty corpus: cov 816 -> 882, ft 3522 -> 3789.
MAX_SINGLE_BODY_LENGTH was derived from MAX_MESSAGE_LENGTH, allowing a 409,608-char single-part body that decodes into a ~200 KiB allocation. But a single-part UR carries its whole message in one fragment, and probe_encode only returns one when the CBOR fits max_fragment_length, so the largest single-part body this library can emit is 8,200 chars — exactly 2 * (MAX_FRAGMENT_LENGTH + 4). Bound it there instead, cutting the worst-case single-QR allocation 50x. MAX_FRAGMENT_LENGTH has only ever grown (200 -> 4096), so no previously encoded single-part UR falls outside the new bound. Cover both sides in the test: the largest emittable single-part UR must still round-trip, and a CRC-valid body one fragment past the envelope must be rejected. Also correct the README's cargo-fuzz install line — cargo-fuzz needs a newer rustc than the nightly pinned in rust-toolchain.
n13
left a comment
n13
left a comment
There was a problem hiding this comment.
Review — UR decoder size gates + fuzzing
Reviewed against the full codebase, with the decoder's bounds re-derived from the encoder rather than taken on trust. The core hardening is correct: both gates sit in decode_ur_part, ahead of decode_minimal_bytewords' Vec::with_capacity, so decode_bytes / decode_hex / is_complete are all covered, and neither gate can reject a fragment this library legitimately emits (verified by construction, not just by the happy-path tests).
Six issues found and fixed in ed81995 and 20f38a8. Details below.
1. Single-part gate was bounded by the wrong envelope — src/lib.rs:36
MAX_SINGLE_BODY_LENGTH was 2 * (MAX_MESSAGE_LENGTH + 4) = 409,608 chars, so a single QR could still be decoded into a ~200 KiB allocation. But a single-part UR carries its whole message in one fragment, and probe_encode only returns is_multi_part == false when the CBOR fits max_fragment_length (≤ MAX_FRAGMENT_LENGTH). Brute-forcing every payload size across fragment lengths, the largest single-part body this library can emit is 8,200 chars — exactly 2 * (MAX_FRAGMENT_LENGTH + 4).
The gate was therefore 50× looser than the envelope it was meant to mirror. Now bounded by the fragment envelope. MAX_FRAGMENT_LENGTH has only ever grown (200 → 4096), so no previously encoded UR falls outside the new bound.
2. The multipart gate had no real test coverage — src/lib.rs:807
test_multi_part_rejects_oversized_fragment_string passed with the gate deleted. Its body was "ae".repeat(...), which decodes fine through the byteword table and then fails the CRC check — also a UrError, so the matches!(.., Err(UrError(_))) assertion could not tell the two apart. Confirmed by deleting the gate and re-running: 30/30 still green.
Rewritten to use CRC-valid bytewords and to assert on decode_ur_part directly, which returns Ok for that input unless the gate fires. Both gate tests now fail if their gate is removed.
3. The boundary test did not reach the boundary — src/lib.rs:824
test_multi_part_max_fragment_length_within_string_gate claimed to exercise "data chunks of MAX_FRAGMENT_LENGTH", but the fountain encoder balances fragments: fragment_length = ceil(msg_len / ceil(msg_len / max)). A 10,000-byte payload yields 3,335-byte chunks → 6,706-char strings against an 8,328-char gate, leaving 20% of the range untested.
Now sized so the CBOR message divides exactly into MAX_FRAGMENT_LENGTH chunks (8,236 chars, 92 from the gate), asserting parts.len() to catch drift and checking every fragment string against the gate.
4. Five of ten fuzz mutations rarely applied — fuzz/fuzz_targets/moving_parts.rs:54
FlipChar/Truncate/Extend/Duplicate/ChangeType addressed frames with parts.get(*part as usize) on an arbitrary u8, while Drop/Swap in the same file used % parts.len(). On a typical 3–25 frame scan the first five no-oped for the large majority of inputs — so the "corrupt / duplicate / relabel a frame" scenarios the PR describes were mostly not being generated. Unified on one frame() helper.
5. ~94% of moving_parts / roundtrip executions started from an empty frame list
fragment_length was fed to the encoder as a raw u16, but encode_bytes_with_options only accepts 1..=4096 — 6.25% of the u16 range. Everything else hit unwrap_or_default(), so most of the quoted 11.8M scenarios never encoded anything to mutate. Now mapped into range in both targets.
Measured, same 300k runs from an empty corpus: cov: 816 ft: 3522 → cov: 882 ft: 3789 (+8.1% edges).
6. Duplicated helper — fuzz/fuzz_targets/moving_parts.rs
craft_multipart_part was a verbatim copy of the src/lib.rs test helper (its own doc comment said "Mirrors the test helper in src/lib.rs"), and Op::ChangeType reimplemented rewrite_ur_type. Both now live in src/test_helpers.rs behind a new fuzzing feature and are used from both places; fuzz/Cargo.toml drops the ur and minicbor deps it needed only for the copy. The default public API is unchanged — verified with cargo build --no-default-features --features core.
Verification
cargo test: 30/30 green; each gate test verified to fail when its gate is deleted.cargo build --no-default-features --features core: clean (no_stdpath intact).- Re-fuzzed the tightened decoder:
decode_parts23.6M runs,moving_parts1.4M,roundtrip262K — no panics, hangs, or OOM; peak RSS ≤ 31 MB. - Round-trip invariant additionally brute-forced over payloads 0–299 × fragment lengths 1–200 (60k combinations): no mismatches, so the
roundtriptarget'sexpect/assert_eq!can't false-positive.
Non-blocking notes
- No CI. There is no
.github/workflows, so neithercargo testnor a short fuzz smoke run gates future PRs. For a decoder whose threat model is untrusted QR input, that is the main thing keeping this hardening from regressing. - Fuzzing runs against different dependency versions than the library ships.
fuzz/.gitignoreignoresCargo.lock, so the fuzz workspace resolvedur-parse-lib/ur-registry1.0.8 while the rootCargo.lockpins 1.0.6. Committingfuzz/Cargo.lockwould make runs reproducible and keep them on the shipped encoder. use hex;atsrc/lib.rs:8tripsclippy::single_component_path_imports— pre-existing, untouched here.
Verdict: approve. The size gates are correct and now bounded by the envelope they actually mirror, the regression tests genuinely cover them, and the fuzz harness exercises the scenarios it advertises.
1 participant
Summary
Inbound UR fragments come from scanned QR codes, i.e. untrusted input. Two size-gate gaps existed on the decode path:
MAX_SINGLE_BODY_LENGTH, the 200 KiB message envelope), before any allocation.MAX_MULTI_PAYLOAD_LENGTH, the 4096-byte fragment envelope + CBOR/CRC overhead), so a set of huge attacker strings can't churn memory before rejection.Both gates sit in
decode_ur_part, sodecode_hex/decode_bytes/is_completeare all covered.Fuzzing
New
fuzz/workspace (cargo-fuzz) attacking the public API as an untrusted QR source:decode_parts— arbitrary strings into all three decode entry points: ~299M runsmoving_parts— animated-QR attack simulator (frame flips, truncation, extension, duplication, drops, reordering, foreign UR types, injected frames with attacker-chosen fountain metadata): ~11.8M scenariosroundtrip— encode→decode byte-identity +is_completeon arbitrary payloads: ~1.6M runsZero panics, hangs, or OOM in ~310M executions. The dependency's fountain decoder is only reachable with simple parts (
sequence ≤ sequence_countis enforced before assembly), keeping its Xoshiro/complex-part machinery out of the attack surface.Note: run targets with
--sanitizer noneon macOS 26 (ASan livelocks during dyld init there).Test plan
cargo test: 30/30 pass, including 3 new regression tests — CRC-valid oversized single-part body, oversized fragment string, and a boundary test proving legit max-size (4096-byte) fragments still pass the string gate.