Repository navigation
feat(fuzz): add bulk decompression fuzz targets (MPPC, NCRUSH, XCRUSH) - #1285
Merged
Benoît Cortier (CBenoit) merged 3 commits intoMay 20, 2026
Merged
Benoît Cortier (CBenoit) merged 3 commits into
Benoît Cortier (CBenoit) merged 3 commits into
Conversation
Adds three algorithm-pinned panic-only targets (`bulk_mppc`,
`bulk_ncrush`, `bulk_xcrush`) plus a round-trip target
(`bulk_round_trip`) exercising the symmetric `BulkCompressor`
compress + decompress pairing. Pattern matches existing
`rle_decompression` and `bitmap_stream` codec targets, and
gateway's `jet_message` round-trip shape.
The bulk crate's output buffer is structurally capped at 64 KB
(`OUTPUT_BUFFER_SIZE` in `bulk.rs`), so memory-bomb is not the
dominant risk. The high-risk bug classes here are bit-stream
alignment errors, dictionary-overrun copies, sliding-window
underruns, and livelock on malformed symbol streams. Livelock
is caught by adding `-timeout=10` to the cargo fuzz invocation
in `xtask/src/fuzz.rs`.
Closes the F13 Core Tier fuzz coverage gap for `ironrdp-bulk`
per ARCHITECTURE.md ("all these crates must be fuzzed").
Regression-replay tests added in `ironrdp-testsuite-core`. Seed
corpora are minimal (zero-byte seed per target); fuzz-driven
corpora grow organically and minimized crashers will land in
the same directories per existing convention.
Assisted-by: Claude (Anthropic) <noreply@anthropic.com>
Signed-off-by: Greg Lamberson <greg@lamco.io>
Member
I think it would be better to use the minimized crashers as there is low interest into keeping bigger artifacts in the repository history. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the fuzzing surface for ironrdp-bulk by adding new bulk decompression fuzz targets (MPPC/NCRUSH/XCRUSH) and a compressor→decompressor round-trip oracle, plus wiring them into the existing xtask fuzz workflow and regression replay tests.
Changes:
- Added 4 new fuzz targets (
bulk_mppc,bulk_ncrush,bulk_xcrush,bulk_round_trip) and documented them. - Implemented bulk decompression and round-trip oracles in
ironrdp-fuzzing, and added regression replay plumbing + seed files inironrdp-testsuite-core. - Updated
cargo xtask fuzz runto pass-timeout=10to libFuzzer runs.
Reviewed changes
Copilot reviewed 11 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
xtask/src/main.rs |
Registers the new fuzz targets in the workspace target list. |
xtask/src/fuzz.rs |
Adds libFuzzer -timeout=10 to xtask fuzz run invocations. |
fuzz/README.md |
Documents the new bulk fuzz targets and their input shapes. |
fuzz/fuzz_targets/bulk_mppc.rs |
Adds MPPC decompression fuzz entrypoint. |
fuzz/fuzz_targets/bulk_ncrush.rs |
Adds NCRUSH decompression fuzz entrypoint. |
fuzz/fuzz_targets/bulk_xcrush.rs |
Adds XCRUSH decompression fuzz entrypoint. |
fuzz/fuzz_targets/bulk_round_trip.rs |
Adds BulkCompressor compress→decompress round-trip fuzz entrypoint. |
fuzz/Cargo.toml |
Registers the new fuzz target binaries. |
fuzz/Cargo.lock |
Locks new dependency graph including ironrdp-bulk. |
crates/ironrdp-fuzzing/src/oracles/mod.rs |
Implements bulk decompression and round-trip oracles. |
crates/ironrdp-fuzzing/Cargo.toml |
Adds ironrdp-bulk dependency to the fuzzing/oracle crate. |
crates/ironrdp-testsuite-core/tests/fuzz_regression.rs |
Adds regression replay tests for the new bulk oracles. |
crates/ironrdp-testsuite-core/test_data/fuzz_regression/* |
Adds seed files/directories for new regression replay targets. |
Cargo.lock |
Locks workspace dependency graph including ironrdp-bulk under ironrdp-fuzzing. |
Comments suppressed due to low confidence (2)
crates/ironrdp-fuzzing/src/oracles/mod.rs:49
- This oracle constructs a fresh
BulkCompressoron every fuzz input. SinceBulkCompressor::newallocates large algorithm state (including XCRUSH), this adds substantial per-iteration overhead and can slow fuzzing. Consider reusing a single compressor instance and resetting it between cases to keep runs deterministic.
pub fn bulk_decompress_ncrush(data: &[u8]) {
use ironrdp_bulk::{BulkCompressor, CompressionType, flags};
let Ok(mut bulk) = BulkCompressor::new(CompressionType::Rdp6) else {
return;
};
let _ = bulk.decompress(data, flags::PACKET_COMPRESSED | 0x02);
}
crates/ironrdp-fuzzing/src/oracles/mod.rs:58
- This oracle constructs a fresh
BulkCompressoron every fuzz input.BulkCompressor::newallocates large internal state (notably XCRUSH’s history/chunk tables), so per-iteration construction can substantially reduce fuzzing throughput. Consider keeping a reused instance (thread-local) and resetting between inputs.
pub fn bulk_decompress_xcrush(data: &[u8]) {
use ironrdp_bulk::{BulkCompressor, CompressionType, flags};
let Ok(mut bulk) = BulkCompressor::new(CompressionType::Rdp61) else {
return;
};
let _ = bulk.decompress(data, flags::PACKET_COMPRESSED | 0x03);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bulk_round_trip previously returned early when compress() cleared PACKET_COMPRESSED, treating the uncompressed case as a vacuous round-trip. That skipped coverage of the decompressor's pass-through branch (decompress with the flag clear returns src_data unchanged), which is real wire-visible code. Select the payload from src or sender.compressed_data based on the flag and run decompress unconditionally so the pass-through branch is now fuzzed alongside the real compressed path. Also drop the to_vec() copy: sender.compressed_data returns a borrow that stays valid through the receiver.decompress call. Signed-off-by: Greg Lamberson <greg@lamco.io>
Signed-off-by: Greg Lamberson <greg@lamco.io>
Benoît Cortier (CBenoit)
merged commit May 20, 2026
a8a813a
into
Devolutions:master
21 checks passed
This was referenced May 20, 2026
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 1, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 3, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 3, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 3, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 10, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 16, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 19, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 20, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 27, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 28, 2026
…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.
Greg Lamberson (glamberson)
added a commit
to lamco-admin/IronRDP
that referenced
this pull request
Aug 28, 2026
…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.
Marc-André Moreau (mamoreau-devolutions)
pushed a commit
that referenced
this pull request
Aug 29, 2026
…der against attacker-controlled input (#1333) ## Summary - Implements target 3 of #1316 (egfx fuzz-coverage umbrella): `egfx_zgfx_decompress` oracle and target. - Target shape mirrors PR #1285's `bulk_*` pattern: panic plus sanitizer oracle on `Decompressor::decompress`, fresh decompressor per iteration so history state does not leak between fuzz inputs. - Ships alongside a complete input-validation audit of the ZGFX decoder, following PR #1271's precedent for "target plus the bugs it surfaces in one PR." ## 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: 1. `Bits::try_split_to` (new in `utils.rs`) provides the checked counterpart to `split_to` that every fix below builds on. 2. `SegmentedDataPdu::from_buffer` multipart segment-size: `split_at_checked` plus a defensive `Vec::with_capacity` cap mirroring PR #1271. 3. `decompress_segment` trailing-unused-bits subtraction: `checked_mul` + `checked_sub`. 4. `decompress_segment` token loop: checked prefix indexing + checked `split_to(8)` for the NullLiteral value. 5. `handle_match`: checked distance-bits split, `checked_add` for `distance_base + value`, plus `distance > HISTORY_SIZE` bound check. 6. `read_unencoded_bytes`: checked splits for the 15-bit length, pad-to-boundary, and `length * 8` payload. 7. `read_encoded_bytes`: checked splits + `usize::BITS` bound on `load_be::<usize>()` + `checked_shl` replacing `pow` + `checked_add` for `base + value`. 8. `FixedCircularBuffer::read_with_offset`: defense-in-depth `offset > buffer.len()` bound check below the caller-side guard. Plus memory-budget enforcement, which a match-copy chain makes necessary: it can expand a few bytes of input into unbounded output, and an unbounded run reached a 1.5 GB peak. 9. New `MAX_DECOMPRESSED_PER_SEGMENT = 64 MiB` ceiling on a single segment's decompressed output, enforced per-token in `decompress_segment` and threaded through the match-copy paths so it is checked before any allocation. **This is an implementation resource limit, not a wire requirement**, and the code says so, but MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") does supply a number: a compliant compressor MUST NOT produce any single segment past 65,535 uncompressed bytes. This crate's production compressor path honors that: `wrapper.rs`'s `wrap_compressed` panics above 65,535, and `compress_and_wrap_egfx`, the only production caller, falls back to uncompressed rather than risk exceeding it. The `compress_high_entropy_round_trips_and_bounds_table` test that an earlier revision of this PR body cited as evidence real traffic exceeds 65,535 does not show that: it calls `Compressor::compress` directly and feeds the result straight to `decompress_segment`, bypassing the wrapper every real sender uses. 64 MiB is still used here, not 65,535, because this ceiling exists to catch non-conforming or hostile input, not to police conforming input: a decoder that hard-rejects at exactly the compressor-side limit has no margin for a peer implementation with a minor, benign spec deviation. Same footing as the compositor's total-byte budget in #1460. 10. Multipart running-total check against the declared `uncompressedSize`, surfacing early detection of segments that collectively exceed the wire-declared bound. Unlike item 9 this one *is* spec-grounded: 2.2.5.1 defines `uncompressedSize` as the size of `segmentArray` once reassembled and decompressed, and 3.1.9.1.2.1 states it MUST equal the total number of decompressed bytes across all segments. The sender side is unchanged and already agreed with the spec: `ZGFX_SEGMENTED_MAXSIZE = 65535` in `wrapper.rs` splits into multipart above that, which is exactly the encoder-side behaviour 3.1.9.1.2.1 describes. Seven new `ZgfxError` variants total with matching Display and `Error::source` impl entries. ## Breaking change `ZgfxError` is a public exhaustive enum, so the seven added variants (`InvalidTrailingBitCount`, `SegmentSizeExceedsBuffer`, `IncompleteBitStream`, `MatchDistanceOutOfRange`, `LengthTokenSizeTooLarge`, `SegmentDecompressedSizeExceedsLimit`, `MultipartTotalExceedsDeclared`) break exhaustive matches downstream. Confirmed by `cargo semver-checks --baseline-rev <merge-base>`: seven `enum_variant_added` failures on `ironrdp-graphics`. Title carries `!` accordingly. ## Validation - `cargo xtask check fmt/lints/tests/typos/locks` all pass. - `check_egfx_zgfx_decompress` regression-replay passes against both shipped crash artifacts. - 46 existing ZGFX unit tests continue to pass against the hardened code. - Final 15-minute rigorous fuzz: 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained), peak RSS 84 MB, zero panics, zero sanitizer reports, zero OOMs. Iterative audit progression on a 15-minute libFuzzer + ASan budget per round: - Round 1 (no fixes): crash at iter ~30 (trailing-bit underflow) - Round 2 (after fixes 1-2): crash at iter ~28 (bit-budget) - Round 3 (after F1-F8 audit): OOM at iter ~9877 (1.5 GB peak) - Round 4 (after F1-F10 complete): clean ## Notes - Crash artifacts for the two header-parse bugs added to `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. - The MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") per-segment bound is a semantic change: inputs that previously decoded to more than 65,535 bytes per segment now return `Err`. Such inputs were never spec-conformant. Codebase precedent (PR #1097 et seq.) is to add `ZgfxError` variants without a `!` marker on the commit; following that convention. - Memory-budget design discussion lives on #1120 (`issuecomment-4558356535`) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread. Refs #1316. ## Note on the fuzz numbers above The iteration counts and the 84 MB peak RSS were measured while the per-segment cap was 65,535. Raising it to 64 MiB does not change what the oracle exercises, but those figures describe the earlier revision rather than what ships here. ## Rebase 2026-08-19 Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions colliding with unrelated new master content at the same point: - `fuzz/Cargo.toml`: master's 8 new RDPEUDP/RDPEMT `[[bin]]` entries plus this PR's own `egfx_zgfx_decompress` entry, both kept. - `crates/ironrdp-fuzzing/src/oracles/mod.rs`: master's 8 new oracle functions plus this PR's own, both appended. The automatic merge dropped this PR's own closing brace for `egfx_zgfx_decompress`, attaching the shared trailing syntax to master's last function instead of both (the same failure mode hit #1337's rebase the other way around, master's brace dropped there). Caught before continuing the rebase this time by diffing the resolved file against both source commits directly; fixed and re-verified byte-identical to both. None of the ZGFX hardening code (`utils.rs`, `zgfx/circular_buffer.rs`, `zgfx/control_messages.rs`, `zgfx/mod.rs`) had any conflict; it applied unchanged. All checks re-run on the new head: `cargo xtask check fmt/lints/tests/typos/locks`, a direct `cargo check --all-targets` in `fuzz/`, and `cargo xtask wasm check`, all green. `fuzz/Cargo.lock` unchanged. ## Review round 2026-08-20 Four findings addressed, three of them posted as review-body text rather than inline threads and never actually worked despite the PR reading as fully resolved. (1) `ZgfxError` marked `#[non_exhaustive]`, matching the same convention already on `DecodeErrorKind`/`EncodeErrorKind` in ironrdp-core; this PR adds several new variants and every downstream exhaustive match would have broken without it. (2) Corrected the doc comment on `MAX_DECOMPRESSED_PER_SEGMENT`: MS-RDPEGFX 3.1.9.1.2's compressor-limits list does supply a bound (65,535 uncompressed bytes per segment for a compliant compressor), and this crate's own compressor honors it (`wrapper.rs` panics above it, the only production caller falls back to uncompressed instead of risking it). (3) Closed a real gap: the multipart aggregate memory budget was only checked against the sender's own declared `uncompressedSize`, a fully attacker-controlled 32-bit field, so a message built from small segments could drive real allocation to roughly 4.29 GiB before that check fired. Added `MAX_DECOMPRESSED_TOTAL` (256 MiB, same footing as `MAX_COMPOSITOR_BYTES` in ironrdp-egfx's compositor.rs), checked independently. (4) Masked the trailer byte to its low 3 bits per MS-RDPEGFX 3.1.9.1.2.4 ("the five high-order bits... are reserved"); reading the whole byte let a sender's nonzero reserved bits get misread as part of the unused-bit count. Both (3) and (4) ship with regression tests negative-verified against the code without the fix. All 6 gates green throughout. ## Rebase 2026-08-20 Rebased onto current master, 9 behind to 0. Two conflicts, both pure unions with master's own new fuzz-target entries in `fuzz/Cargo.toml` and `fuzz_regression.rs`, same shape as the earlier rebase round. `oracles/mod.rs` auto-merged without conflict markers this time; explicitly re-checked it did not repeat the closing-brace-drop failure from that earlier round (the function's own closing brace confirmed present, then a full `cargo check -p ironrdp-fuzzing --all-targets` run clean). All 6 gates green, fuzz workspace lock stable.
holovchenko
pushed a commit
to holovchenko/IronRDP
that referenced
this pull request
Sep 25, 2026
…der against attacker-controlled input (Devolutions#1333) - Implements target 3 of Devolutions#1316 (egfx fuzz-coverage umbrella): `egfx_zgfx_decompress` oracle and target. - Target shape mirrors PR Devolutions#1285's `bulk_*` pattern: panic plus sanitizer oracle on `Decompressor::decompress`, fresh decompressor per iteration so history state does not leak between fuzz inputs. - Ships alongside a complete input-validation audit of the ZGFX decoder, following PR Devolutions#1271's precedent for "target plus the bugs it surfaces in one PR." Eight input-validation gaps closed in `ironrdp-graphics` (all class-(c) per Devolutions#1314: reachable via attacker-controlled wire inputs). Each one was surfaced by a rigorous smoke-fuzz iteration: 1. `Bits::try_split_to` (new in `utils.rs`) provides the checked counterpart to `split_to` that every fix below builds on. 2. `SegmentedDataPdu::from_buffer` multipart segment-size: `split_at_checked` plus a defensive `Vec::with_capacity` cap mirroring PR Devolutions#1271. 3. `decompress_segment` trailing-unused-bits subtraction: `checked_mul` + `checked_sub`. 4. `decompress_segment` token loop: checked prefix indexing + checked `split_to(8)` for the NullLiteral value. 5. `handle_match`: checked distance-bits split, `checked_add` for `distance_base + value`, plus `distance > HISTORY_SIZE` bound check. 6. `read_unencoded_bytes`: checked splits for the 15-bit length, pad-to-boundary, and `length * 8` payload. 7. `read_encoded_bytes`: checked splits + `usize::BITS` bound on `load_be::<usize>()` + `checked_shl` replacing `pow` + `checked_add` for `base + value`. 8. `FixedCircularBuffer::read_with_offset`: defense-in-depth `offset > buffer.len()` bound check below the caller-side guard. Plus memory-budget enforcement, which a match-copy chain makes necessary: it can expand a few bytes of input into unbounded output, and an unbounded run reached a 1.5 GB peak. 9. New `MAX_DECOMPRESSED_PER_SEGMENT = 64 MiB` ceiling on a single segment's decompressed output, enforced per-token in `decompress_segment` and threaded through the match-copy paths so it is checked before any allocation. **This is an implementation resource limit, not a wire requirement**, and the code says so, but MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") does supply a number: a compliant compressor MUST NOT produce any single segment past 65,535 uncompressed bytes. This crate's production compressor path honors that: `wrapper.rs`'s `wrap_compressed` panics above 65,535, and `compress_and_wrap_egfx`, the only production caller, falls back to uncompressed rather than risk exceeding it. The `compress_high_entropy_round_trips_and_bounds_table` test that an earlier revision of this PR body cited as evidence real traffic exceeds 65,535 does not show that: it calls `Compressor::compress` directly and feeds the result straight to `decompress_segment`, bypassing the wrapper every real sender uses. 64 MiB is still used here, not 65,535, because this ceiling exists to catch non-conforming or hostile input, not to police conforming input: a decoder that hard-rejects at exactly the compressor-side limit has no margin for a peer implementation with a minor, benign spec deviation. Same footing as the compositor's total-byte budget in Devolutions#1460. 10. Multipart running-total check against the declared `uncompressedSize`, surfacing early detection of segments that collectively exceed the wire-declared bound. Unlike item 9 this one *is* spec-grounded: 2.2.5.1 defines `uncompressedSize` as the size of `segmentArray` once reassembled and decompressed, and 3.1.9.1.2.1 states it MUST equal the total number of decompressed bytes across all segments. The sender side is unchanged and already agreed with the spec: `ZGFX_SEGMENTED_MAXSIZE = 65535` in `wrapper.rs` splits into multipart above that, which is exactly the encoder-side behaviour 3.1.9.1.2.1 describes. Seven new `ZgfxError` variants total with matching Display and `Error::source` impl entries. `ZgfxError` is a public exhaustive enum, so the seven added variants (`InvalidTrailingBitCount`, `SegmentSizeExceedsBuffer`, `IncompleteBitStream`, `MatchDistanceOutOfRange`, `LengthTokenSizeTooLarge`, `SegmentDecompressedSizeExceedsLimit`, `MultipartTotalExceedsDeclared`) break exhaustive matches downstream. Confirmed by `cargo semver-checks --baseline-rev <merge-base>`: seven `enum_variant_added` failures on `ironrdp-graphics`. Title carries `!` accordingly. - `cargo xtask check fmt/lints/tests/typos/locks` all pass. - `check_egfx_zgfx_decompress` regression-replay passes against both shipped crash artifacts. - 46 existing ZGFX unit tests continue to pass against the hardened code. - Final 15-minute rigorous fuzz: 2,942,961 iterations in 901 seconds (~3,266 exec/s sustained), peak RSS 84 MB, zero panics, zero sanitizer reports, zero OOMs. Iterative audit progression on a 15-minute libFuzzer + ASan budget per round: - Round 1 (no fixes): crash at iter ~30 (trailing-bit underflow) - Round 2 (after fixes 1-2): crash at iter ~28 (bit-budget) - Round 3 (after F1-F8 audit): OOM at iter ~9877 (1.5 GB peak) - Round 4 (after F1-F10 complete): clean - Crash artifacts for the two header-parse bugs added to `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. - The MS-RDPEGFX 3.1.9.1.2 ("RDP 8.0 compressor limits") per-segment bound is a semantic change: inputs that previously decoded to more than 65,535 bytes per segment now return `Err`. Such inputs were never spec-conformant. Codebase precedent (PR Devolutions#1097 et seq.) is to add `ZgfxError` variants without a `!` marker on the commit; following that convention. - Memory-budget design discussion lives on Devolutions#1120 (`issuecomment-4558356535`) rather than in this PR body so future readers find the rationale on the canonical fuzz-umbrella thread. Refs Devolutions#1316. The iteration counts and the 84 MB peak RSS were measured while the per-segment cap was 65,535. Raising it to 64 MiB does not change what the oracle exercises, but those figures describe the earlier revision rather than what ships here. Rebased onto current master (was CONFLICTING after the RDPEUDP/RDPEMT series landed). Two conflicts, both purely additive insertions colliding with unrelated new master content at the same point: - `fuzz/Cargo.toml`: master's 8 new RDPEUDP/RDPEMT `[[bin]]` entries plus this PR's own `egfx_zgfx_decompress` entry, both kept. - `crates/ironrdp-fuzzing/src/oracles/mod.rs`: master's 8 new oracle functions plus this PR's own, both appended. The automatic merge dropped this PR's own closing brace for `egfx_zgfx_decompress`, attaching the shared trailing syntax to master's last function instead of both (the same failure mode hit Devolutions#1337's rebase the other way around, master's brace dropped there). Caught before continuing the rebase this time by diffing the resolved file against both source commits directly; fixed and re-verified byte-identical to both. None of the ZGFX hardening code (`utils.rs`, `zgfx/circular_buffer.rs`, `zgfx/control_messages.rs`, `zgfx/mod.rs`) had any conflict; it applied unchanged. All checks re-run on the new head: `cargo xtask check fmt/lints/tests/typos/locks`, a direct `cargo check --all-targets` in `fuzz/`, and `cargo xtask wasm check`, all green. `fuzz/Cargo.lock` unchanged. Four findings addressed, three of them posted as review-body text rather than inline threads and never actually worked despite the PR reading as fully resolved. (1) `ZgfxError` marked `#[non_exhaustive]`, matching the same convention already on `DecodeErrorKind`/`EncodeErrorKind` in ironrdp-core; this PR adds several new variants and every downstream exhaustive match would have broken without it. (2) Corrected the doc comment on `MAX_DECOMPRESSED_PER_SEGMENT`: MS-RDPEGFX 3.1.9.1.2's compressor-limits list does supply a bound (65,535 uncompressed bytes per segment for a compliant compressor), and this crate's own compressor honors it (`wrapper.rs` panics above it, the only production caller falls back to uncompressed instead of risking it). (3) Closed a real gap: the multipart aggregate memory budget was only checked against the sender's own declared `uncompressedSize`, a fully attacker-controlled 32-bit field, so a message built from small segments could drive real allocation to roughly 4.29 GiB before that check fired. Added `MAX_DECOMPRESSED_TOTAL` (256 MiB, same footing as `MAX_COMPOSITOR_BYTES` in ironrdp-egfx's compositor.rs), checked independently. (4) Masked the trailer byte to its low 3 bits per MS-RDPEGFX 3.1.9.1.2.4 ("the five high-order bits... are reserved"); reading the whole byte let a sender's nonzero reserved bits get misread as part of the unused-bit count. Both (3) and (4) ship with regression tests negative-verified against the code without the fix. All 6 gates green throughout. Rebased onto current master, 9 behind to 0. Two conflicts, both pure unions with master's own new fuzz-target entries in `fuzz/Cargo.toml` and `fuzz_regression.rs`, same shape as the earlier rebase round. `oracles/mod.rs` auto-merged without conflict markers this time; explicitly re-checked it did not repeat the closing-brace-drop failure from that earlier round (the function's own closing brace confirmed present, then a full `cargo check -p ironrdp-fuzzing --all-targets` run clean). All 6 gates green, fuzz workspace lock stable. (cherry picked from commit 4232026) Conflict resolution (fork): upstream's context for this commit carried two sibling oracles the fork does not have — egfx_avc444_decode (from 71e0ae7) and egfx_surface_state (from 501d1eb) — which produced conflicts in crates/ironrdp-fuzzing/src/oracles/mod.rs, crates/ironrdp-testsuite-core/tests/fuzz_regression.rs and fuzz/Cargo.toml. Kept the fork's existing oracle/test/bin entries as-is and added only what 4232026 itself introduces: the egfx_zgfx_decompress oracle function and its doc-comment update to egfx_round_trip, the check_egfx_zgfx_decompress regression test, the egfx_zgfx_decompress [[bin]] entry, its fuzz target file, and its three regression test-data files. Dropped the egfx_avc444_decode oracle, test, and [[bin]] entry that appeared only as merge context from the sibling commit the fork lacks. All ZGFX hardening code in ironrdp-graphics (utils.rs, zgfx/circular_buffer.rs, zgfx/control_messages.rs, zgfx/mod.rs) applied cleanly with no conflicts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.
Summary
Adds fuzz coverage for
ironrdp-bulkdecompression. Closes a Core Tier fuzz gap per ARCHITECTURE.md ("all these crates must be fuzzed"). Three algorithm-pinned panic-only targets (bulk_mppc,bulk_ncrush,bulk_xcrush) plus one round-trip target (bulk_round_trip) exercising the symmetricBulkCompressorcompress + decompress pairing.Filed against #1124 acceptance criteria ("Oracles assert invariants (round-trip equality and/or framing invariants)") per the coordination comment on #1120 (#1120 (comment)).
Approach
Each fuzz target binary is a tiny wrapper (3 lines) calling an oracle in
crates/ironrdp-fuzzing/src/oracles/mod.rs. Algorithm dispatch is driven byflags::PACKET_COMPRESSEDplus the algorithm low-bit perCompressionType::from_flags:bulk_mppc: first input byte selects RDP4 (low bit clear, 8 KB history) vs RDP5 (low bit set, 64 KB history) so a single corpus exercises both MPPC modes via libFuzzer mutation across the byte boundary.bulk_ncrush: pinned to RDP6 NCRUSH.bulk_xcrush: pinned to RDP6.1 XCRUSH (2 MB sliding window).bulk_round_trip: first input byte selects algorithm; the remaining bytes are uncompressed input. The oracle callsBulkCompressor::compress, then constructs a freshBulkCompressorfor the receive side, decompresses, and asserts byte-equality. A fresh decompressor per call avoids sliding-window state leaking between fuzz iterations.Pattern matches the existing
rle_decompressionandbitmap_streamcodec targets inironrdp-graphics, anddevolutions-gateway'sjet_messagebyte-for-byte round-trip shape.-timeout=10added to thecargo xtask fuzz runinvocation inxtask/src/fuzz.rs. Bulk decompressors are bit-stream parsers that iterate symbol-by-symbol over a variable-length input until a terminator is observed; adversarial inputs can construct streams that never terminate cleanly. The default libFuzzer timeout (1200 seconds) is too loose to catch livelock during the 5-second CI smoke run. 10 seconds is generous for any legitimate input across the workspace (PDU decoders complete in microseconds) and tight enough to surface livelock as a per-input timeout report. All four bulk targets and all existing targets ran clean locally with the new flag.Regression-replay tests added under
crates/ironrdp-testsuite-core/test_data/fuzz_regression/. Seed corpora are minimal (one zero-byte seed per target); fuzz-driven corpora grow organically and minimized crashers will land in the same directories per existing convention.Alternatives considered
A
cap::Cap-based memory-budget oracle was considered for the bulk targets. After surveying Devolutions org fuzz practice acrossdevolutions-gateway,sspi-rs, andpicky-rs, no precedent exists forcap::Capallocator wrapping or in-process memory tracking in any of those repos. After analyzing theironrdp-bulkbug profile specifically, the crate's output buffer is structurally capped at 64 KB (OUTPUT_BUFFER_SIZEinbulk.rs) and per-call allocation is near-zero by design (sliding windows are pre-allocated atBulkCompressor::new). Memory-bomb is not the dominant risk on this crate. The dominantironrdp-bulkbug classes are bit-stream alignment errors, dictionary-overrun copies, sliding-window underruns, and livelock; the chosen oracle stack (panic + round-trip +-timeout) is matched to those.The memory-budget oracle remains the right shape for
ironrdp-egfxZGFX andironrdp-graphicscodec decoders where output sizing is field-driven (a 100-byte ClearCodec PDU declaring a 65535x65535 tile demands ~16 GB of RAM; historical MSTSC ClearCodec CVE cluster CVE-2020-0610/-0611/-0612/-0655/-0660 sits on this surface). That work is reserved for a separately-filed sibling issue per the coordination comment on #1120.A per-target
bulk_mppc_rdp4/bulk_mppc_rdp5split was considered instead of the first-byte mode selector. The single-target shape was chosen for parity with the other bulk targets and to let libFuzzer mutate across the algorithm boundary in one corpus. Happy to split if reviewers prefer.Differential round-trip against an alternative implementation is deferred.
Trade-offs accepted
The panic-only oracle does not catch silent logic bugs where a decompressor returns wrong output without panicking. The round-trip oracle catches a substantial portion of that class (any compress-then-decompress asymmetry is caught) but does not exhaustively cover decode-of-adversarial-input behaviour, where the input was not produced by a matching compressor.
-timeout=10is uniform across all fuzz targets, not bulk-specific. PDU decoders should complete in microseconds, so 10 seconds is far above any legitimate input cost across the workspace. If a non-bulk target ever needs more than 10 seconds for legitimate input, the value is a one-line change inxtask/src/fuzz.rs.Regression-replay seeds are zero-byte placeholders, not minimized crashers. Crashes surfaced during fuzz runs will land in these directories per existing convention (the same pattern as
cliprdr_format/which has only crash-* and minimized-from-* files).Verification
Local:
cargo xtask check fmt— cleancargo xtask check lints— cleancargo xtask check tests— 6 fuzz regression tests pass (the 2 existing + 4 new)cargo xtask check typos— cleanCI: awaiting on push.