Repository navigation
fix(svc): bound compressed static channel reassembly by its declared length - #2083
Mathieu Morrissette (mmorrissette-devolutions) wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback". |
415ac05 to
eafd116
Compare
GitHub Actions
Active
github-actions
Bot
added
automation-failed
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The fragment is allocated and copied before validation, so the buffer is not actually bounded by the declared length.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Bounds compressed static-channel reassembly using the declared uncompressed length.
Changes:
- Applies overflow validation to compressed sequences.
- Adds regression coverage for overflow and state recovery.
| File | Description |
|---|---|
crates/ironrdp-svc/src/lib.rs |
Extends reassembly bounds checking to compressed fragments. |
crates/ironrdp-testsuite-core/tests/svc.rs |
Tests compressed sequence bounds and recovery. |
eafd116 to
b2fc4fb
Compare
GitHub Actions
Active
192182d to
deaf9dd
Compare
GitHub Actions
Active
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
github-actions
Bot
added
kind/protocol
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR moves the reassembly-length overflow check in ChunkProcessor::dechunkify before the buffer append and applies it regardless of PACKET_COMPRESSED, closing a pre-existing unbounded reassembly-buffer growth path for fragmented static virtual channel data. For non-compressed fragments the new pre-append condition rejects exactly the same sequences as the old post-append check; for compressed-flagged fragments, IronRDP never decompresses static channel data and the accumulated bytes are an upper-bounded proxy (conformant bulk compression never expands), so spec-conformant traffic is not newly rejected. saturating_add avoids overflow and the terminal exact-length check is retained. The new test verifies rejection, sequence recovery, and terminal delivery with a minimal test-double change. One valid low-severity open question remains about the exact semantics of the header length field under PACKET_COMPRESSED, which cannot be verified from this repository but does not affect compliant…
- [skeptical] Per-fragment bound assumes the header length field stays meaningful under PACKET_COMPRESSED — low 🟡 ❓ — crates/ironrdp-svc/src/lib.rs
The new pre-append check treats header.length as the declared total of the fragment sequence even when PACKET_COMPRESSED is set. If the protocol defines that field as the uncompressed size under compression, the bound's relationship to accumulated bytes depends on compression never expanding, which cannot be confirmed from this repository (the PR's FreeRDP citation is external). Impact is limited: IronRDP advertises no static-channel compression and never decompresses static channel data, so a compliant peer does not set PACKET_COMPRESSED, and non-expanding compression cannot make accumulated bytes exceed an uncompressed declared total.
github-actions
Bot
added
ai-reviewed/1
…length The reassembly buffer of a fragmented static channel PDU was only checked against the declared length when PACKET_COMPRESSED was clear. A peer could declare a small length and keep sending compressed fragments, growing the buffer without limit until the final fragment. IronRDP advertises VirtualChannelFlags::NO_COMPRESSION (VCCAPS_NO_COMPR) and never decompresses static channel data, so compressed fragments were already unusable and a compliant peer does not send them. Apply the check regardless of compression, and validate each fragment before appending it so the buffer never grows past the declared length. FreeRDP enforces the declared length on every fragment in the same way.
deaf9dd to
ddc2d00
Compare
GitHub Actions
Active
github-actions
Bot
removed
the
needs-author-action
There was a problem hiding this comment.
Verified independently: the PR moves the static virtual channel reassembly bound check before buffer extension and applies it to PACKET_COMPRESSED sequences, closing a real unbounded-memory path where a peer could declare a small length and stream fragments indefinitely. The new condition is equivalent to the old post-append check for non-compressed data, so no legitimate traffic is newly rejected; error paths clear sequence state as before; skipping exact-length terminal checks for compressed data is intentional since IronRDP advertises NO_COMPRESSION and never decompresses static channel data. The new testsuite test exercises rejection of an oversized compressed sequence, recovery, and correct delivered payload using an existing helper. No regressions found. One residual, pre-existing observation: the bound is the peer-declared u32 length with no lower cap, so reassembly memory can still grow up to ~4 GiB per sequence; noted as low severity, not a regression of this PR.
| self.chunked_pdu.extend_from_slice(chunk); | ||
|
|
||
| if !header.flags.contains(ChannelControlFlags::PACKET_COMPRESSED) && self.chunked_pdu.len() > expected_length { | ||
| if expected_length < self.chunked_pdu.len().saturating_add(chunk.len()) { |
There was a problem hiding this comment.
[general] Reassembly buffer remains bounded only by the peer-declared length (up to ~4 GiB) — low 🟡 — The new pre-append check bounds chunked_pdu by header.length, but that length is attacker-controlled u32 with no cap anywhere in the receive path (MAX_CHANNEL_CHUNK_LENGTH only constrains outbound chunking), so a peer can still grow the buffer toward 4 GiB per sequence by declaring a large length and streaming fragments. Pre-existing for uncompressed data and strictly improved by this PR, so low severity; a future hardening could cap the declared length or total buffered bytes.
github-actions
Bot
added
ai-reviewed/2
|
fix for #2081 |
|
This correctly closes the small-declared-length PACKET_COMPRESSED bypass and rejects before appending. One resource-bound gap remains: header.length is peer-controlled and can be up to u32::MAX, so the same Gateway memory-pressure path remains possible by declaring a large value, such as 512 MiB, and sending that amount. Could you also reject declared lengths above an implementation-defined or configurable aggregate SVC limit before starting reassembly? A regression test using u32::MAX should verify immediate rejection and recovery. Current fix is incomplete hardening to the actual OOM consequence. |
Summary
ChunkProcessor::dechunkifyonly checked the reassembly buffer of a fragmented static virtual channel PDU against the declared length whenPACKET_COMPRESSEDwas clear. With the flag set, a peer could declare a small length and keep sending fragments, and the buffer grew without limit until the final fragment.IronRDP advertises
VirtualChannelFlags::NO_COMPRESSION(VCCAPS_NO_COMPR) for static channels and never decompresses static channel data. Compressed fragments were therefore already unusable, and a compliant peer does not send them. This PR makes two changes:The exact-length checks on unfragmented data and on the terminal fragment are still skipped for compressed data. FreeRDP enforces the declared length on every fragment, whatever the compression flag (
channels/client/addin.c).Tests
static_channel_bounds_compressed_chunk_sequences_by_declared_lengthinironrdp-testsuite-core. It checks three things:cargo test -p ironrdp-testsuite-corepasses.