fix(pdu): keep undefined channel option bits instead of refusing by maryny4 · Pull Request #1911 · Devolutions/IronRDP

Marynych Oleksandr (maryny4) · 2026-09-05T17:51:18Z

ChannelDef::decode rejects the whole PDU when options carries a bit outside the eleven defined flags. [MS-RDPBCGR] 2.2.1.3.4.1 does not ask a server to validate that field, and for several of the flags it asks the opposite — CHANNEL_OPTION_INITIALIZED, ENCRYPT_RDP, ENCRYPT_SC and ENCRYPT_CS are each "unused and its value MUST be ignored by the server", and CHANNEL_OPTION_SHOW_PROTOCOL likewise.

So the strictness is not the protocol's, and it costs sessions.

What it costs

rdesktop 1.9.0 cannot connect to an IronRDP server at all. It writes this one field big-endian while the rest of TS_UD_CS_NET is little-endian — secure.c:516 uses out_uint32_be where every neighbouring field uses out_uint32_le — so each channel's flags arrive byte-swapped and every set bit lands outside the mask.

Captured from the wire against an IronRDP-based server:

"cliprdr"  c0 a0 00 00   ->  read little-endian as 0x0000a0c0
"rdpsnd"   c0 00 00 00   ->  0x000000c0
"rdpdr"    80 80 00 00   ->  0x00008080

Every bit is undefined, so the connection dies at GCC, before a single channel is joined. This is a client bug, not a vendor extension — but it is a client that has shipped for years.

The fix

This is the "peer advertisements" case in the new STYLE.md "Decoding unknown values" section, which uses ChannelOptions as its worked example: decode with from_bits_retain. Unknown bits are kept, never fatal, and the decode → encode round-trip reproduces the wire bytes — which is what the round-trip fuzz oracle relies on, and the reason the guide prefers retain over from_bits_truncate.

Two alternative explanations were checked against the same bytes and ruled out: the block is not misaligned (4 + 5*12 = 64 is exactly blockLen) and channelCount is not misread.

Tests

Three: the known bits stay readable while an unknown bit is kept; the wholly-undefined rdesktop value still yields a channel; and the raw bits survive a decode → encode round-trip. All three fail under from_bits_truncate.

(Reopened after my fork was briefly deleted, which auto-closed the original #1837. Rebased onto current master and switched from from_bits_truncate to from_bits_retain to match the STYLE.md guidance added since.)

`ChannelDef::decode` rejects the whole PDU when `options` carries a bit outside the eleven defined flags. [MS-RDPBCGR] 2.2.1.3.4.1 does not ask a server to validate that field, and for several flags it asks the opposite: CHANNEL_OPTION_INITIALIZED, ENCRYPT_RDP, ENCRYPT_SC and ENCRYPT_CS are each "unused and its value MUST be ignored by the server", and SHOW_PROTOCOL likewise. So the strictness is not the protocol's, and it costs sessions. rdesktop 1.9.0 cannot connect to an IronRDP server at all. It writes this one field big-endian while the rest of TS_UD_CS_NET is little-endian (secure.c:516 uses out_uint32_be where every neighbouring field uses out_uint32_le), so each channel's flags arrive byte-swapped and every set bit lands outside the mask. The connection dies at GCC, before a single channel is joined. Captured from the wire, "cliprdr" arrives as c0 a0 00 00 and read little-endian that is 0x0000a0c0 -- all bits undefined. This is the "peer advertisements" case in STYLE.md's "Decoding unknown values": decode with `from_bits_retain`, which keeps the unknown bits so a decode -> encode round-trip reproduces the wire bytes, rather than `from_bits` (fatal on unknown bits) or `from_bits_truncate` (drops them and breaks the round-trip the replay tooling and fuzz oracle rely on). Three tests: the known bits stay readable while an unknown bit is kept; the wholly-undefined rdesktop value still yields a channel; and the raw bits survive a decode -> encode round-trip.

Greg Lamberson (glamberson) · 2026-09-05T19:04:43Z

I went through this the same way I did on #1837, checking against the spec and the STYLE.md addition rather than taking the PR body's word for it.

The MS-RDPBCGR 2.2.1.3.4.1 reading still holds: Five of the eleven documented ChannelOptions flags carry literal "MUST be ignored by the server" language, and the server-side processing rules in 3.3.5.3.3 never ask for validation of this field. The rdesktop 1.9.0 byte-order bug is the same trace as before: secure.c writes this one field big-endian while the rest of TS_UD_CS_NET is little-endian.

The change from from_bits_truncate to from_bits_retain is the right call given the STYLE.md section added since my last look. That section names ChannelOptions specifically as its "peer advertisements" example, and it gives from_bits_retain as the preferred form precisely because it keeps the decode to encode round trip byte for byte, which matters for replay tooling and the round-trip fuzz oracle in a way truncate does not. Nothing in the workspace re-encodes a decoded ChannelDef today, but that is exactly the kind of invariant worth holding before something depends on it.

All three tests exercise what the rationale calls for: Known bits preserved alongside an unknown one, the actual rdesktop wire value still yielding a channel, and a full round trip. This is a sound fix.

Copilot

🟢 Approval recommended

The implementation is covered by focused tests; only a documentation nit remains.

Pull request overview

Updates GCC channel decoding to preserve undefined option bits, improving compatibility with nonconforming clients.

Changes:

Uses from_bits_retain for channel options.
Adds decoding and round-trip regression tests.
Contains a non-blocking stale STYLE.md reference.

File summaries

File	Description
`crates/ironrdp-pdu/src/gcc/network_data.rs`	Preserves unknown channel option bits and adds interoperability tests.

Review details

Suppressed comments (1)

crates/ironrdp-pdu/src/gcc/network_data.rs:365

This test comment also attributes the behavior to a nonexistent section of STYLE.md. Avoid leaving a second dead reference; the test only needs to state the behavior it verifies.

    /// Retaining unknown bits keeps decode -> encode byte-for-byte, which is
    /// the reason STYLE.md prefers `from_bits_retain` over `from_bits_truncate`
    /// for advertisement fields: replay tooling and the round-trip fuzz oracle
    /// depend on it.

Files reviewed: 1/1 changed files
Comments generated: 1
Review effort level: Balanced

Marynych Oleksandr (maryny4) · 2026-09-10T08:20:33Z

+        // This is the "peer advertisements" case in STYLE.md's "Decoding
+        // unknown values": `from_bits_retain` keeps the unknown bits so that a
+        // decode -> encode round-trip reproduces the wire bytes.


The section does exist — STYLE.md has a “Decoding unknown values” heading (line 141 on current master), and ChannelOptions is the worked example it uses (line 158), which is why the comment points there instead of restating the rationale. The “peer advertisement” wording comes from that same section.

github-actions

The production change is correct and conformant: ChannelDef::decode now retains undefined ChannelOptions bits instead of rejecting the PDU, matching MS-RDPBCGR 2.2.1.3.4.1 (several flags are explicitly 'MUST be ignored by the server', none require validation) and the actual in-repo guidance in crates/ironrdp-pdu/README.md 'On bit flags', restoring the byte-exact decode->encode round-trip and unblocking rdesktop 1.9.0's byte-swapped options field. All five candidates are accepted; each is a low-severity prose or test-quality issue, not a correctness defect: the comment and test doc cite a nonexistent STYLE.md section (real guidance is in the crate README), the test doc misstates what the round-trip fuzz oracle asserts, ChannelOptions lacks the README-prescribed const _ = !0 with a noted activex from_bits ripple, and two minor test simplifications are available (encode_vec helper, dropping a subsumed contains assertion).

github-actions · 2026-09-11T19:59:09Z

+        // This is the "peer advertisements" case in STYLE.md's "Decoding
+        // unknown values": `from_bits_retain` keeps the unknown bits so that a
+        // decode -> encode round-trip reproduces the wire bytes.


[skeptical] Comment cites a STYLE.md section that does not exist at this head — low 🟡 — The comment justifies the change via STYLE.md's 'Decoding unknown values' section with ChannelOptions as its 'peer advertisements' example, but that section is not in this repository: STYLE.md has no such heading and no occurrences of 'retain', 'peer', or 'ChannelOptions', the claimed line 141 falls in the Logging examples, and this PR does not modify STYLE.md. The real in-repo guidance is crates/ironrdp-pdu/README.md 'On bit flags', which prescribes from_bits_retain for resilient parsing; the comment should cite that, and the protocol substance of the fix is unaffected.

github-actions · 2026-09-11T19:59:09Z

+    /// Retaining unknown bits keeps decode -> encode byte-for-byte, which is
+    /// the reason STYLE.md prefers `from_bits_retain` over `from_bits_truncate`
+    /// for advertisement fields: replay tooling and the round-trip fuzz oracle
+    /// depend on it.


[skeptical] Test doc comment misstates what the round-trip fuzz oracle asserts — low 🟡 — The doc comment claims byte-for-byte retention matters because 'the round-trip fuzz oracle depend[s] on it' and STYLE.md prefers from_bits_retain for that reason. Both claims fail at this head: crates/ironrdp-fuzzing/src/oracles/mod.rs (pdu_round_trip_one) documents that byte stability is deliberately NOT asserted and only requires decode -> encode -> re-decode, which from_bits_truncate also satisfies, and STYLE.md contains no such guidance. The assert_eq!(out, bytes) test is fine as a local check, but the stated invariant is false and could mislead someone refactoring the encoder on the assumption the oracle enforces byte equality.

github-actions · 2026-09-11T19:59:09Z

+        // This is the "peer advertisements" case in STYLE.md's "Decoding
+        // unknown values": `from_bits_retain` keeps the unknown bits so that a
+        // decode -> encode round-trip reproduces the wire bytes.
+        let options = ChannelOptions::from_bits_retain(src.read_u32());


[skeptical] Resilient parsing adopted without the README-prescribed const _ = !0 — low 🟡 — crates/ironrdp-pdu/README.md ('On bit flags') prescribes using both from_bits_retain and const _ = !0 when resilient parsing is required, so a future accidental switch to from_bits_truncate cannot silently destroy unknown bits and complement behaves as expected for externally defined bits. The ChannelOptions bitflags block (lines 307-319) lacks the marker, unlike most peer-advertised flag types in this crate. Ripple to check before adding it: const _ = !0 makes ChannelOptions::from_bits infallible, which would silently neuter the existing unknown-bits rejection at ironrdp-activex control.rs:12828 (SetVirtualChannelOptions returns E_INVALIDARG).

github-actions · 2026-09-11T19:59:09Z

+        let mut out = vec![0u8; bytes.len()];
+        decoded.encode(&mut WriteCursor::new(&mut out)).expect("re-encode");
+        assert_eq!(out, bytes);


[code-compressor] Use the existing encode_vec helper instead of a manual WriteCursor round-trip — low 🟡 — The round-trip test hand-rolls buffer allocation and cursor plumbing (vec![0u8; bytes.len()] plus WriteCursor and an expect). The crate already exposes ironrdp_core::encode_vec for exactly this, and it is the established test convention here (gcc/monitor_data.rs and many capability-set tests). Since ChannelDef::size() is the fixed 12-byte CLIENT_CHANNEL_SIZE, encode_vec(&decoded) produces identical bytes while dropping the mutable buffer, the WriteCursor usage, and one expect site.

github-actions · 2026-09-11T19:59:09Z

+
+        let decoded = ChannelDef::decode(&mut ReadCursor::new(&bytes)).expect("undefined bits are not fatal");
+
+        assert!(decoded.options.contains(known));


[code-compressor] Drop the contains assertion subsumed by the exact-bits equality on the next line — low 🟡 — assert!(decoded.options.contains(known)) is logically implied by the very next assertion, assert_eq!(decoded.options.bits(), known.bits() | 0x0000_0044): any value whose bits equal known | extra necessarily contains known. Removing the weaker assertion preserves the test's pass/fail behavior exactly while shrinking it to a single decisive check; keeping both only changes failure-message clarity, not coverage.

Marynych Oleksandr (maryny4) temporarily deployed to llm-providers September 5, 2026 17:51 — with GitHub Actions Inactive

Marynych Oleksandr (maryny4) force-pushed the fix/gcc-channel-options-mask branch from 4d59f4c to a7bf9b9 Compare September 5, 2026 18:25

Marynych Oleksandr (maryny4) temporarily deployed to llm-providers September 5, 2026 18:25 — with GitHub Actions Inactive

Benoît Cortier (CBenoit) requested a balanced review from Copilot September 9, 2026 01:55

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) September 9, 2026 01:56 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

github-actions Bot reviewed Sep 11, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed and removed needs-review A human reviewer is the current next actor labels Sep 11, 2026

Marc-André Moreau (mamoreau-devolutions) approved these changes Sep 28, 2026

View reviewed changes

Marc-André Moreau (mamoreau-devolutions) merged commit e56bb42 into Devolutions:master Sep 28, 2026
42 checks passed

Navigation Menu

fix(pdu): keep undefined channel option bits instead of refusing - #1911

fix(pdu): keep undefined channel option bits instead of refusing#1911
Marc-André Moreau (mamoreau-devolutions) merged 1 commit into
Devolutions:masterfrom
maryny4:fix/gcc-channel-options-mask

Marynych Oleksandr (maryny4) commented Sep 5, 2026

Uh oh!

Greg Lamberson (glamberson) commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Marynych Oleksandr (maryny4) Sep 10, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Sep 11, 2026

Uh oh!

github-actions Bot Sep 11, 2026

Uh oh!

github-actions Bot Sep 11, 2026

Uh oh!

github-actions Bot Sep 11, 2026

Uh oh!

github-actions Bot Sep 11, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants


		let decoded = ChannelDef::decode(&mut ReadCursor::new(&bytes)).expect("undefined bits are not fatal");

		assert!(decoded.options.contains(known));

Conversation

Marynych Oleksandr (maryny4) commented Sep 5, 2026

What it costs

The fix

Tests

Uh oh!

Greg Lamberson (glamberson) commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (1)

Uh oh!

Marynych Oleksandr (maryny4) Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants