Skip to content

fix(session): reject out-of-range RFX quant indexes - #2094

Open
Ki Hyun Park (kihyun1998) wants to merge 1 commit into
Devolutions:masterfrom
kihyun1998:fix/rfx-quant-index
Open

Ki Hyun Park (kihyun1998) wants to merge 1 commit into
Devolutions:masterfrom
kihyun1998:fix/rfx-quant-index

Conversation

@kihyun1998

Copy link
Copy Markdown

A RemoteFX tile selects its Y, Cb and Cr quantization tables by index into the tileset's TS_RFX_CODEC_QUANT array (MS-RDPRFX 2.2.2.3.4.1). The session decoder indexed the array without checking them, so one out-of-range byte from the server panicked the client; in the web client the panic surfaced as RuntimeError: unreachable and stalled the tab.

Each index is now checked against the tileset's tables, and an out-of-range one fails the frame with a session error before any of its tiles is decoded, as FreeRDP does.

Fixes #2090

A RemoteFX tile selects its Y, Cb and Cr quantization tables by index
into the tileset's TS_RFX_CODEC_QUANT array (MS-RDPRFX 2.2.2.3.4.1).
The session decoder indexed the array without checking them, so one
out-of-range byte from the server panicked the client; in the web
client the panic surfaced as `RuntimeError: unreachable` and stalled
the tab.

Each index is now checked against the tileset's tables, and an
out-of-range one fails the frame with a session error before any of
its tiles is decoded, as FreeRDP does.

Fixes Devolutions#2090
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Oct 7, 2026
@kihyun1998

Copy link
Copy Markdown
Author

Validation:

  • decode_rejects_tile_quant_index_out_of_range re-encodes the MS-RDPRFX 4.2 example sequence with only the tileset changed, and covers an out-of-range Y, Cb and Cr index with one table, and a tileset with no tables. On master each case panics at the line indexing its own field (rfx.rs:274, 275, 276).
  • The tests live in ironrdp-testsuite-core because ironrdp-session has [lib] test = false; they appear in the cargo xtask check tests run.
  • Each assertion was seen failing under a targeted mutation: indexing Cb directly, an off-by-one bound, a different error, and rejecting valid indexes (caught by the existing decode_decodes_valid_sequence_of_messages).
  • TileSetPdu decoding is unchanged: the PDU tests pin a tile with index 0xff decoding successfully, so the check sits with the consumer.
  • cargo xtask ci passes locally up to the web lint, which fails here only because this Windows checkout has CRLF line endings; no web files change.

Note

Human-tuned, LLM-assisted content.

@github-actions github-actions Bot 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.

The PR fixes a remote-DoS panic in the RFX session decoder: map_tiles_data previously indexed the TS_RFX_TILESET quantVals array unguarded with server-controlled TS_RFX_TILE quantIdxY/Cb/Cr values, so any out-of-range byte panicked the client. It now returns SessionResult via a bounds-checked quant closure and propagates the error at the zip site, failing the frame before any tile decodes, consistent with MS-RDPRFX 2.2.2.3.4.1 and FreeRDP behavior; eager-collection semantics are unchanged and PDU decoding is untouched. Four parameterized rstest cases cover out-of-range Y/Cb/Cr and a zero-table tileset, re-encoding the known-good 4.2.2 sequence with only the tileset mutated. The sole specialist candidate (code-compressor, duplicate-destination-rectangle-construction) is a valid low-severity test-style nit: the new test's destination rectangle construction duplicates lines 14-19 of the adjacent pre-existing test; verified in pr-head and refined only to narrow the cited line range to exc…

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

Comment on lines +49 to +54
let destination = InclusiveRectangle {
left: 0,
top: 0,
right: u16::try_from(IMAGE_WIDTH).unwrap() - 1,
bottom: u16::try_from(IMAGE_HEIGHT).unwrap() - 1,
};

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.

[code-compressor] New test duplicates the existing test's destination rectangle construction — low 🟡 — The InclusiveRectangle built from IMAGE_WIDTH/IMAGE_HEIGHT at lines 49-54 of decode_rejects_tile_quant_index_out_of_range is identical to the construction in decode_decodes_valid_sequence_of_messages (lines 14-19). A small file-local helper (e.g. fn full_image_destination() -> InclusiveRectangle) used by both tests removes the duplication and keeps the tests in sync if frame geometry changes. Purely stylistic: no correctness or behavioral impact, and accepting the duplication for diff minimality is a reasonable alternative.

@github-actions github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Oct 9, 2026

This branch was successfully deployed

1 active deployment
llm-providers — 40f2c4db Deployed Oct 7, 2026 by kihyun1998 via Classify pull request #1852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed kind/protocol Affects RDP or related protocol behavior needs-author-action The pull request author is the current next actor risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure

Development

Successfully merging this pull request may close these issues.

ironrdp-session: validate RemoteFX tile quantization indexes before lookup

1 participant