fix(pdu): reject RSA moduli below 512 bits by se-wo · Pull Request #2225 · Devolutions/IronRDP

se-wo · 2026-10-24T15:59:50Z

The client encrypts the licensing premaster secret with the public key from the server's certificate, and num-bigint panics on a zero modulus, so a server sending an all-zero modulus crashed the client during the license exchange. The proprietary certificate decoder checks only the key length fields, and the X.509 path passes the key through unchanged.

Encryption now fails with an InvalidData error for a modulus shorter than 512 bits, the size of a proprietary certificate key, which also keeps the modulus larger than the 48-byte premaster secret.

The pdu_decode fuzz oracle now builds the client's reply to a decoded license request, so it reaches this encryption, and the crashing input joins its regression corpus.

The client encrypts the licensing premaster secret with the public key from the server's certificate, and num-bigint panics on a zero modulus, so a server sending an all-zero modulus crashed the client during the license exchange. The proprietary certificate decoder checks only the key length fields, and the X.509 path passes the key through unchanged. Encryption now fails with an InvalidData error for a modulus shorter than 512 bits, the size of a proprietary certificate key, which also keeps the modulus larger than the 48-byte premaster secret. The pdu_decode fuzz oracle now builds the client's reply to a decoded license request, so it reaches this encryption, and the crashing input joins its regression corpus. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

se-wo · 2026-10-24T16:00:25Z

Lines changed

+129 / −1 across five files, at 36fe909.

Part	File	+	−
Production	`crates/ironrdp-pdu/src/crypto/rsa.rs`	17	0
Fuzz oracle	`crates/ironrdp-fuzzing/src/oracles/mod.rs`	20	1
Tests	`crates/ironrdp-testsuite-core/tests/pdu/server_license.rs`	91	0
Tests	`crates/ironrdp-testsuite-core/tests/pdu/mod.rs`	1	0
Test data	`crates/ironrdp-testsuite-core/test_data/fuzz_regression/pdu_decode/crash-3639648ab6730a3d8617bc39badcc741c56bdc4f`	binary, 304 bytes

Of the 17 production lines, 7 are logic (the MIN_MODULUS_BITS constant and the guard in encrypt_with_public_key) and 10 are its doc comment and a blank line.
The fuzz oracle now builds both client replies that encrypt with the server's public key, ClientNewLicenseRequest and ClientLicenseInfo, from a decoded license request.

The latest commit adds +20 / −4 and changes no production logic: the MIN_MODULUS_BITS doc comment links MS-RDPBCGR 2.2.1.4.3.1.1.1 and 4.1.4, the oracle builds the second reply, and each test case row carries its own oracle marker.

Validation

licensing_accepts_only_moduli_of_at_least_512_bits covers both licensing replies (ClientNewLicenseRequest and ClientLicenseInfo) with a zero, a 256-bit and a 512-bit modulus. The zero cases panicked with "attempt to calculate with zero modulus!" before this change, and the 256-bit cases were accepted.
The new pdu_decode regression input crash-3639648ab6730a3d8617bc39badcc741c56bdc4f, a Server License Request with an all-zero proprietary modulus, panics check_pdu_decode without the fix.
cargo clippy -D warnings is clean for ironrdp-pdu, ironrdp-fuzzing and ironrdp-testsuite-core, and their tests pass.

Why 512 bits
Proprietary certificates carry 512-bit keys, as in the test vector in server_license_request/tests.rs (BITLEN 0x200). That is the smallest key a server sends, and it keeps the modulus larger than the 48-byte premaster secret. X.509 license server keys are larger.

Out of scope (separate issue)
ServerCertificate::get_public_key passes the proprietary certificate's modulus, which is little-endian on the wire like a CryptoAPI RSA1 blob, to pkcs1::UintRef::new, which expects big-endian. Licensing against a proprietary certificate would then encrypt with the wrong key. This PR's size check works in either byte order, so it does not depend on that fix.

github-actions

Sound, narrowly scoped fix: the 512-bit modulus guard in encrypt_with_public_key stops the reachable zero-modulus num-bigint panic on both licensing reply paths without rejecting the smallest legitimate 512-bit proprietary key, and the fuzz oracle now reaches the crashing encryption path with a regression corpus file. Two low-severity precision caveats on the guard are valid: the proprietary certificate's modulus buffer includes 8 bytes of trailing zero padding (so the effective floor on key material is 448 bits), and the known little-endian modulus bug in ServerCertificate::get_public_key means the check measures a byte-reversed value on the proprietary path until that separate fix lands. The remaining candidates are test-hygiene items: the oracle marker cites a sibling test as a 'reference' source, the fuzz oracle exercises only one of the two encrypting replies, and the MIN_MODULUS_BITS doc comment lacks a spec link. The rust-style comment nit about the marker is rejected because '…

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.

Each modulus case row now carries its own oracle marker: the zero modulus is a regression citing the pdu_decode crash file, the below-512-bit rejection is a mutant check, and the 512-bit acceptance follows the example key in spec MS-RDPBCGR §4.1.4. The pdu_decode fuzz oracle builds both encrypting client replies to a decoded license request, ClientNewLicenseRequest and ClientLicenseInfo. The MIN_MODULUS_BITS doc comment links MS-RDPBCGR 2.2.1.4.3.1.1.1 and 4.1.4, the sections that give the 512-bit proprietary certificate key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KhrixAye4o5msM4tEzqzK

github-actions

PR 2225 fixes a client crash on an all-zero RSA modulus from a server license certificate by rejecting moduli below 512 bits in encrypt_with_public_key, the single choke point shared by both licensing reply builders. The threshold accepts all spec-conformant keys, is byte-order-agnostic, and keeps the modulus larger than the 48-byte premaster secret, so the fix is correct and minimal. The fuzz oracle now reaches the panicking path and the regression input joins the corpus. Remaining items are low-severity: the below-threshold test case rests on a mutant oracle because the 512-bit floor is specified only in a code doc comment, the all-synthetic moduli never exercise real little-endian wire bytes despite an in-repo captured key, and two minor style nits (two sentences on one doc-comment line, a single-use test helper).

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.

github-actions · 2026-10-26T00:12:12Z

+// oracle: mutant a guard that rejects only a zero modulus, accepting a 256-bit key
+#[case::modulus_below_512_bits(vec![0xff; 32], false)]


[test] Below-threshold rejection case rests on a mutant oracle; 512-bit floor specified only in a code comment — low 🟡 ❓ — The modulus_below_512_bits rejection case is oracle-marked 'mutant', i.e. self-derived and deletable by intentional change. The 512-bit minimum is stated in the MIN_MODULUS_BITS doc comment (rsa.rs:6-15) and the PR description, but no normative source states the rejection rule, so the case's expected false lacks an independent source. Ask where to specify the minimum-modulus rule (e.g. an intent or contract file for ironrdp-pdu) and re-mark the case with that source.

github-actions · 2026-10-26T00:12:12Z

+#[rstest]
+// oracle: regression crates/ironrdp-testsuite-core/test_data/fuzz_regression/pdu_decode/crash-3639648ab6730a3d8617bc39badcc741c56bdc4f
+#[case::zero_modulus(vec![0; 64], false)]
+// oracle: mutant a guard that rejects only a zero modulus, accepting a 256-bit key
+#[case::modulus_below_512_bits(vec![0xff; 32], false)]
+// oracle: spec MS-RDPBCGR §4.1.4
+#[case::modulus_of_512_bits(vec![0xff; 64], true)]


[test] Byte-order-symmetric synthetic moduli never exercise a realistic little-endian wire modulus — low 🟡 ❓ — All three case moduli (vec![0; 64], vec![0xff; 32], vec![0xff; 64]) look identical in either byte order and render the 8-byte MODULUS_PADDING trailing zeros irrelevant to bits(). The guard computes from_bytes_be over a modulus that is little-endian on the wire, so the acceptance decision for a real certificate depends on byte order plus padding in a way these inputs cannot distinguish from a wrong-order or wrong-length check. The repo carries a real captured 512-bit key (MODULUS: [u8; 72], 64 LE bytes plus 8 padding) at server_license_request/tests.rs:177 that could join the table.

github-actions · 2026-10-26T00:12:12Z


+/// The smallest RSA modulus accepted from a server, in bits.
+///
+/// A modulus of zero would make `modpow` panic. 512 bits is the size of the proprietary certificate key in


[rust-style] Doc comment line carries two sentences — low 🟡 — Line 8 reads 'A modulus of zero would make `modpow` panic. 512 bits is the size of the proprietary certificate key in' — two sentences on one line, against the one-sentence-per-line rule. Split after 'panic.' onto a new line.

github-actions · 2026-10-26T00:12:12Z

+fn license_request_with_modulus(modulus: Vec<u8>) -> ServerLicenseRequest {
+    ServerLicenseRequest {
+        license_header: LicenseHeader {
+            security_header: BasicSecurityHeader {
+                flags: BasicSecurityHeaderFlags::LICENSE_PKT,
+            },
+            preamble_message_type: PreambleType::LicenseRequest,
+            preamble_flags: PreambleFlags::empty(),
+            preamble_version: PreambleVersion::V3,
+            preamble_message_size: 0,
+        },
+        server_random: vec![0x44; 32],
+        product_info: ProductInfo {
+            version: 0x60000,
+            company_name: "Microsoft Corporation".to_owned(),
+            product_id: "A02".to_owned(),
+        },
+        server_certificate: Some(ServerCertificate {
+            issued_permanently: true,
+            certificate: CertificateType::Proprietary(ProprietaryCertificate {
+                public_key: RsaPublicKey {
+                    public_exponent: 0x0001_0001,
+                    modulus: [modulus.as_slice(), &MODULUS_PADDING].concat(),
+                },
+                signature: vec![0; 72],
+            }),
+        }),
+        scope_list: Vec::new(),
+    }
+}


[rust-style] Single-use helper without `?` or `return` — low 🟡 — license_request_with_modulus has exactly one call site (line 56) and uses neither '?' nor 'return', so STYLE.md's helper-functions rule prefers inlining the ServerLicenseRequest construction as a block in the test body. Marginal for a readable test fixture; low severity.

The 512-bit acceptance case now uses the proprietary certificate modulus from MS-RDPBCGR 4.1.4 instead of a synthetic all-0xff key, so the case cites the bytes its spec marker names and exercises a real little-endian wire modulus, which reads differently in each byte order. The license request construction moves into the test body, and the MIN_MODULUS_BITS doc comment puts each sentence on its own line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012KhrixAye4o5msM4tEzqzK

github-actions

The PR fixes a real client crash (num-bigint panics on a zero modulus when encrypting the licensing premaster secret) by rejecting RSA moduli below 512 bits in encrypt_with_public_key, extends the pdu_decode fuzz oracle to build both client replies, and adds a focused rstest plus a regression corpus input. Independently verified against pr-head: RsaPublicKey::decode keeps the 8 zero padding bytes of a proprietary RSA_PUBLIC_KEY in the modulus, ServerCertificate::get_public_key embeds them verbatim, and the new guard measures that padded buffer, so the effective floor on the declared key size is ~448 bits rather than the documented 512. The test findings are valid: the 511-bit boundary is untested, the zero-modulus case's regression marker overclaims what the crash file decides, and the X.509 certificate path to the shared guard is unexercised. The code-compressor's test-shape suggestion is rejected: the rstest enum parametrization is idiomatic, keeps per-case diagnostic labels, and th…

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.

github-actions · 2026-10-26T00:57:37Z

+    if n.bits() < MIN_MODULUS_BITS {
+        return Err(io::Error::new(
+            io::ErrorKind::InvalidData,
+            format!("RSA public key modulus is shorter than {MIN_MODULUS_BITS} bits"),
+        ));
+    }


[protocol] Modulus-size guard measures the padded certificate buffer, enforcing only ~448 bits of declared key — medium 🟠 — RsaPublicKey::decode keeps the 8 zero padding bytes of a proprietary RSA_PUBLIC_KEY (keylen = bitlen/8 + 8) in the modulus, and ServerCertificate::get_public_key embeds those bytes verbatim as the PKCS#1 INTEGER, so n.bits() tracks the declared bitlen plus 64 bits whenever the little-endian low modulus byte is nonzero. A spec-conformant key with a declared bitlen around 448 bits passes the 512-bit check, contradicting MIN_MODULUS_BITS's documented minimum and letting undersized server keys encrypt the 48-byte premaster secret. The guard should evaluate the actual modulus (padding stripped) or the decoded bitlen; the new test codifies the padded behavior by appending MODULUS_PADDING and expecting acceptance.

github-actions · 2026-10-26T00:57:37Z

+#[case::zero_modulus(vec![0; 64], false)]
+// oracle: mutant a guard that rejects only a zero modulus, accepting a 256-bit key
+#[case::modulus_below_512_bits(vec![0xff; 32], false)]
+// oracle: spec MS-RDPBCGR §4.1.4
+#[case::modulus_of_512_bits(SPEC_EXAMPLE_MODULUS.to_vec(), true)]


[test] No case at the boundary just below the 512-bit limit, so a threshold-shift mutant survives — low 🟡 — The rows cover 0, 256 and 512 bits; a mutant moving MIN_MODULUS_BITS to 511 passes every row, including the 512-bit acceptance case. Add a modulus_below_512_bits_by_one case with a 64-byte modulus whose top bit is clear expecting rejection, or replace the rows with a proptest asserting accepted == (n.bits() >= 512) to pin the law across generated inputs.

github-actions · 2026-10-26T00:57:37Z

+// oracle: regression crates/ironrdp-testsuite-core/test_data/fuzz_regression/pdu_decode/crash-3639648ab6730a3d8617bc39badcc741c56bdc4f
+#[case::zero_modulus(vec![0; 64], false)]


[test] Regression marker on the zero-modulus case overclaims what the crash file decides — low 🟡 — The referenced crash file pins only that an all-zero-modulus Server License Request must be handled without panicking, which is already pinned standalone by the corpus input through check_pdu_decode. The case's expected false value additionally asserts rejection, which comes from this PR's own 512-bit policy decision like the adjacent mutant-marked 256-bit row, not from the crash file. Scope the regression marker to the no-panic framing and mark the rejection expectation with a mutant or spec marker naming the 512-bit minimum as its deciding source.

github-actions · 2026-10-26T00:57:37Z

+        server_certificate: Some(ServerCertificate {
+            issued_permanently: true,
+            certificate: CertificateType::Proprietary(ProprietaryCertificate {
+                public_key: RsaPublicKey {
+                    public_exponent: 0x0001_0001,
+                    modulus: [modulus.as_slice(), &MODULUS_PADDING].concat(),
+                },
+                signature: vec![0; 72],
+            }),
+        }),


[test] The new guard is exercised only through the proprietary certificate path — low 🟡 — The threshold check lives in the shared encrypt_with_public_key, which both reply builders reach via ServerCertificate::get_public_key, but the test builds only a CertificateType::Proprietary certificate. The X.509 variant reaches the same helper through the DER branch with a differently shaped key (no little-endian layout or padding), so the size rule on that path is unpinned. Add a case with an X.509 certificate holding one accepted and one rejected key, or confirm the proprietary-path cases suffice because the guard sits in the one shared function.

…ktop-scale-factor * upstream/master: (92 commits) fix(server): advertise horizontal mouse wheel input (Devolutions#2236) fix(pdu): reject RSA moduli below 512 bits (Devolutions#2225) ci(ffi): verify generated bindings (Devolutions#2166) build(ffi)!: move to Diplomat 0.16.1-devo.2 (Devolutions#2165) build: make the client's video, camera and decoder crates publishable (Devolutions#2239) ci(release): release the newly published crates (Devolutions#2218) perf(graphics): replace the RLGR decoder's bit reader (Devolutions#2150) chore(graphics): add RemoteFX decode benchmarks (Devolutions#2149) fix(server)!: pass a GfxContext to the GfxServerFactory build methods (Devolutions#2104) fix(pdu): check remaining length when skipping padding and reading client core data (Devolutions#2130) fix(session): avoid hang on empty QOIZ updates (Devolutions#2147) feat(session): expose EGFX compositing (Devolutions#2120) fix(server): send RDP as the Demand Active source descriptor (Devolutions#2119) feat(activex)!: broaden mstscax compatibility (Devolutions#2211) feat(viewer): run RemoteApp programs in native windows (Devolutions#2201) fix(graphics): reject RLGR3 values larger than their remainder (Devolutions#2132) feat(egfx)!: composite surfaces mapped to RemoteApp windows (Devolutions#2197) feat: redirect USB devices over URBDRC with nusb and RemoteFX USB backends (Devolutions#2181) build: mark exposed dependencies as public (Devolutions#2213) chore(release): prepare for publishing (Devolutions#2212) ... # Conflicts: # crates/ironrdp-testsuite-extra/tests/e2e.rs

se-wo temporarily deployed to llm-providers October 24, 2026 16:00 — with GitHub Actions Inactive

github-actions Bot reviewed Oct 24, 2026

View reviewed changes

Comment thread crates/ironrdp-pdu/src/crypto/rsa.rs

Comment thread crates/ironrdp-testsuite-core/tests/pdu/server_license.rs Outdated

Comment thread crates/ironrdp-fuzzing/src/oracles/mod.rs Outdated

Comment thread crates/ironrdp-pdu/src/crypto/rsa.rs Outdated

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 24, 2026

se-wo temporarily deployed to llm-providers October 25, 2026 23:39 — with GitHub Actions Inactive

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 25, 2026

github-actions Bot reviewed Oct 26, 2026

View reviewed changes

github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Oct 26, 2026

se-wo temporarily deployed to llm-providers October 26, 2026 00:22 — with GitHub Actions Inactive

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 26, 2026

github-actions Bot reviewed Oct 26, 2026

View reviewed changes

github-actions Bot added ai-reviewed/3 Final automated review completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/2 Two automated reviews completed labels Oct 26, 2026

Marc-André Moreau (mamoreau-devolutions) approved these changes Oct 26, 2026

View reviewed changes

Marc-André Moreau (mamoreau-devolutions) merged commit c338617 into Devolutions:master Oct 26, 2026
44 checks passed

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 26, 2026

		// oracle: mutant a guard that rejects only a zero modulus, accepting a 256-bit key
		#[case::modulus_below_512_bits(vec![0xff; 32], false)]

		// oracle: regression crates/ironrdp-testsuite-core/test_data/fuzz_regression/pdu_decode/crash-3639648ab6730a3d8617bc39badcc741c56bdc4f
		#[case::zero_modulus(vec![0; 64], false)]

Conversation

se-wo commented Oct 24, 2026

Uh oh!

se-wo commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lines changed

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 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 Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

se-wo commented Oct 24, 2026 •

edited

Loading