feat(fuzz): add egfx_zgfx_decompress oracle and harden the ZGFX decoder against attacker-controlled input#1333
Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Conversation
…er against attacker-controlled input Adds the egfx_zgfx_decompress fuzz target under Devolutions#1316 (target 3 of the egfx fuzz-coverage umbrella), scoped to the ZGFX decompression path in ironrdp-graphics::zgfx. ZGFX is the egfx-specific compression scheme defined in MS-RDPEGFX 2.2.4.1, distinct from ironrdp-bulk's MPPC / NCRUSH / XCRUSH (which carry connection-level RDP traffic). The target shape mirrors the bulk_* targets from PR Devolutions#1285: panic plus sanitizer oracle on Decompressor::decompress, fresh decompressor per iteration so history state does not leak between inputs. A complete input-validation audit of the ZGFX decoder accompanies the target. Iterative rigorous smoke fuzz on a 15-minute budget surfaced each bug class in turn; the fixes below close every one that the audit covers. Final validation run after the last fix completed clean: 2.94M iterations in 901 seconds, peak RSS 84 MB, no panics, no OOMs, no sanitizer reports. Decoder input-validation fixes (all class-(c) per Devolutions#1314's taxonomy: reachable via attacker-controlled wire inputs): - ironrdp-graphics::utils::Bits::try_split_to (new): checked counterpart to split_to that returns None instead of panicking when the requested width exceeds the bit budget. Foundation for the per-call-site checks below. - zgfx/control_messages.rs:34 (SegmentedDataPdu::from_buffer): the multipart-segment loop called slice::split_at on an attacker-controlled u32 size field without bound-checking against buffer.len(). Fixed via split_at_checked surfacing ZgfxError::SegmentSizeExceedsBuffer { size, remaining }. Also caps Vec::with_capacity(segment_count) against the remaining buffer size (each segment is at least 5 bytes), mirroring the pattern from PR Devolutions#1271. - zgfx/mod.rs:90 (decompress_segment): the trailing-unused-bits subtraction 8 * (encoded_data.len() - 1) - last_byte underflowed on any input where last_byte exceeded the available bit budget. Fixed via checked_mul + checked_sub surfacing ZgfxError::InvalidTrailingBitCount(usize). - zgfx/mod.rs decompress_segment token loop: replaced the unchecked bits[..token.prefix.len()] slice indexing with bits.get(...) so a truncated segment whose remaining bits are shorter than every prefix surfaces as TokenBitsNotFound. Replaced bits.split_to(8) (NullLiteral value) with try_split_to(8) returning ZgfxError::IncompleteBitStream { needed, remaining }. - zgfx/mod.rs handle_match: replaced bits.split_to(distance_value_size) with the checked variant. Replaced the unchecked distance_base + value u32 addition with checked_add. Added a defense-in-depth bound check rejecting match distances larger than HISTORY_SIZE (2.5 MB) so the circular buffer's position arithmetic cannot underflow on attacker-controlled distance. - zgfx/mod.rs read_unencoded_bytes: all three bits.split_to calls (15-bit length field, pad-to-boundary, length * 8 payload) replaced with the checked variant. length is bounded by the 15-bit field but length * 8 is now computed via checked_mul. - zgfx/mod.rs read_encoded_bytes: leading-ones + length-token-size arithmetic guarded against overflow. The 2^N base calculation replaced with 1usize.checked_shl(exponent) and bounded against usize::BITS so an attacker-controlled length_token_size cannot drive the exponent into a pow overflow or load_be::<usize>() panic. The base + value sum uses checked_add. New ZgfxError variant LengthTokenSizeTooLarge(usize) surfaces the rejected value. - zgfx/circular_buffer.rs read_with_offset: added a defense-in-depth bound check rejecting offsets larger than the buffer size, so a stray attacker-controlled offset surfaces as a typed I/O error rather than an unsigned-underflow panic in the position calculation. Complements the caller-side bound check in handle_match. Memory-budget oracle (per MS-RDPEGFX section 2.2.1, applied per the design discussion on Devolutions#1120 issuecomment-4558356535): - New const MAX_DECOMPRESSED_PER_SEGMENT = 65_535 in zgfx/mod.rs. Per the spec, a single ZGFX segment must not produce more than 65,535 decompressed bytes. The decompress_segment loop enforces this per-token and at the segment-cumulative level, surfacing ZgfxError::SegmentDecompressedSizeExceedsLimit { decompressed, limit } before allocation rather than after. The check threads through handle_match, read_unencoded_bytes, and read_encoded_bytes via a max_remaining parameter so a match-copy of length N is rejected before the circular buffer read consumes that much memory. - Multipart running-total check after each segment, surfacing ZgfxError::MultipartTotalExceedsDeclared { written, declared } if segments collectively produce more bytes than the uncompressedSize u32 declared in the multipart header. The declared u32 remains the wire-format upper bound; realistic-traffic limits below that are a caller-layer concern. Closes the multipart-bomb variant of the same OOM class. Two of the bugs (segment-size split_at, trailing-bit underflow) shipped with concrete crash reproducer inputs from the original smoke fuzz. Both inputs are added to crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_zgfx_decompress/ as crash-multipart-segment-size-oversize.bin (11 bytes) and crash-trailing-bit-underflow.bin (3 bytes). The regression-replay test check_egfx_zgfx_decompress passes against the fixed code; both inputs now return typed errors rather than panicking. The OOM and bit-budget crash inputs from later rounds are not added as regression entries because the libFuzzer corpus already contains structurally similar inputs and the unit tests cover the typed-error paths directly. The 46 existing ZGFX unit tests continue to pass against the hardened code, confirming no semantic regression for well-formed input. Validation run after all fixes: - 15-minute libFuzzer + ASan run with -timeout=10 - 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained) - Peak RSS: 84 MB (vs 1,565 MB on the pre-F9 OOM) - Zero panics, zero sanitizer reports, zero OOMs - 161 corpus entries accumulated from iterative bug discovery Refs Devolutions#1316. Cross-reference for the memory-budget design rationale: issuecomment-4558356535 on Devolutions#1120.
This was referenced May 28, 2026
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
1 participant
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.
Summary
egfx_zgfx_decompressoracle and target.bulk_*pattern: panic plus sanitizer oracle onDecompressor::decompress, fresh decompressor per iteration so history state does not leak between fuzz inputs.Hardening
Eight input-validation gaps closed in
ironrdp-graphics(all class-(c) per #1314: reachable via attacker-controlled wire inputs). Each one was surfaced by a rigorous smoke-fuzz iteration:Bits::try_split_to(new inutils.rs) provides the checked counterpart tosplit_tothat every fix below builds on.SegmentedDataPdu::from_buffermultipart segment-size:split_at_checkedplus a defensiveVec::with_capacitycap mirroring PR feat(fuzz): add egfx PDU decoders to pdu_decode oracle + fix two surfaced bugs #1271.decompress_segmenttrailing-unused-bits subtraction:checked_mul+checked_sub.decompress_segmenttoken loop: checked prefix indexing + checkedsplit_to(8)for the NullLiteral value.handle_match: checked distance-bits split,checked_addfordistance_base + value, plusdistance > HISTORY_SIZEbound check.read_unencoded_bytes: checked splits for the 15-bit length, pad-to-boundary, andlength * 8payload.read_encoded_bytes: checked splits +usize::BITSbound onload_be::<usize>()+checked_shlreplacingpow+checked_addforbase + value.FixedCircularBuffer::read_with_offset: defense-in-depthoffset > buffer.len()bound check below the caller-side guard.Plus the spec-derived memory-budget enforcement from MS-RDPEGFX 2.2.1 (design rationale documented at
issuecomment-4558356535on #1120):MAX_DECOMPRESSED_PER_SEGMENT = 65_535enforced per-token indecompress_segmentand threaded through the match-copy paths so the cap is checked before any allocation.uncompressedSize, surfacing early detection of segments that collectively exceed the wire-declared bound.Seven new
ZgfxErrorvariants total with matching Display andError::sourceimpl entries.Validation
cargo xtask ciclean locally on1.89.0: fmt, lints, tests, typos, locks all green.check_egfx_zgfx_decompressregression-replay passes against both shipped crash artifacts.Iterative audit progression on a 15-minute libFuzzer + ASan budget per round:
Notes
crates/ironrdp-testsuite-core/test_data/fuzz_regression/egfx_zgfx_decompress/. The later-round crashes are not added as separate regression entries because the libFuzzer corpus accumulated 161 representative inputs and the unit-tested error paths cover the cases directly.Err. Such inputs were never spec-conformant. Codebase precedent (PR feat(zgfx): add LZ77 compression support #1097 et seq.) is to addZgfxErrorvariants without a!marker on the commit; following that convention.arbitrary#1120 (issuecomment-4558356535) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread.Refs #1316.