Skip to content

fix(pdu): harden framing and empty output handling - #1515

Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
masterfrom
fix-pdu-framing-empty-output
Aug 2, 2026
Merged

fix(pdu): harden framing and empty output handling#1515
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
masterfrom
fix-pdu-framing-empty-output

Conversation

@mamoreau-devolutions

Copy link
Copy Markdown
Contributor

Reject Fast-Path and TPKT frames whose declared length is smaller than their header or minimum packet size. Also tolerate the zero-length totalLength variation used by empty Update and Pointer output PDUs, while continuing to reject zero-length non-output data PDUs.

Adds regression coverage for malformed frame lengths and empty output compatibility.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 2, 2026 18:11
@github-actions github-actions Bot added scope/core Touches the core architectural tier rust size/S Size: 30-149 lines of code labels Aug 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens RDP frame validation while supporting zero-length empty output PDUs.

Changes:

  • Rejects undersized TPKT and Fast-Path frames.
  • Accepts empty Update and Pointer output PDUs with zero totalLength.
  • Adds regression tests for malformed and compatible framing.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
crates/ironrdp-pdu/src/tpkt.rs Validates minimum TPKT size.
crates/ironrdp-pdu/src/rdp/headers.rs Handles empty output PDUs and adds tests.
crates/ironrdp-pdu/src/lib.rs Validates Fast-Path lengths and tests malformed frames.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mamoreau-devolutions
Marc-André Moreau (mamoreau-devolutions) merged commit 33506e6 into master Aug 2, 2026
27 checks passed
@mamoreau-devolutions
Marc-André Moreau (mamoreau-devolutions) deleted the fix-pdu-framing-empty-output branch August 2, 2026 18:36
Marc-André Moreau (mamoreau-devolutions) added a commit that referenced this pull request Aug 2, 2026
Reject Fast-Path and TPKT frames whose declared length is smaller than
their header or minimum packet size. Also tolerate the zero-length
`totalLength` variation used by empty Update and Pointer output PDUs,
while continuing to reject zero-length non-output data PDUs.

Adds regression coverage for malformed frame lengths and empty output
compatibility.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GlassOnTin added a commit to GlassOnTin/IronRDP that referenced this pull request Aug 3, 2026
`ShareControlHeader::decode` rejects a Data PDU whose `totalLength` is
smaller than the unit it decoded. That check costs whole sessions against
VirtualBox VRDP, which declares 24 on a complete 8550-byte slow-path
pointer update.

`totalLength` is consulted here for exactly one purpose: locating trailing
padding after the inner unit ([MS-RDPBCGR] 2.2.8.1.1.1.1). A value smaller
than what was decoded therefore means there is no padding to skip, and
nothing more. It is not grounds to reject the PDU — the inner unit already
decoded within the cursor's bounds, and where the PDU ends is decided by
the transport framing (the TPKT or fast-path length), never by this field.

Deliberately narrow: a `totalLength` of zero stays rejected for anything
but the no-op output PDUs Devolutions#1515 allowed. A field that is absent entirely
is a different signal from one that is merely wrong, and Devolutions#1515 drew that
line three days ago; this does not move it. All three of its tests still
pass unchanged.

Tests: the reporter's real frame shape (8565 bytes on the wire, 8550 of
MCS user data, totalLength 24) now decodes; zero-length leniency is
asserted not to have widened; over-declared padding is still consumed, and
declared-but-absent padding still errors. Verified to fail — restoring the
strict branch reddens the under-declared test and leaves the three guard
tests green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope/core Touches the core architectural tier size/S Size: 30-149 lines of code

Development

Successfully merging this pull request may close these issues.

2 participants