Repository navigation
fix(pdu): reject RSA moduli below 512 bits #2225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,17 @@ use std::io; | |
| use der_parser::parse_der; | ||
| use num_bigint::BigUint; | ||
|
|
||
| /// 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 [4.1.4] Server MCS Connect Response PDU with GCC | ||
| /// Conference Create Response, whose [2.2.1.4.3.1.1.1] RSA Public Key (RSA_PUBLIC_KEY) has a `bitlen` of 512, and a | ||
| /// modulus of at least 512 bits is larger than the 48-byte premaster secret it encrypts. | ||
| /// | ||
| /// [4.1.4]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/d23f7725-876c-48d4-9e41-8288896a19d3 | ||
| /// [2.2.1.4.3.1.1.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/fe93545c-772a-4ade-9d02-ad1e0d81b6af | ||
| const MIN_MODULUS_BITS: u64 = 512; | ||
|
|
||
| pub(crate) fn encrypt_with_public_key(message: &[u8], public_key_der: &[u8]) -> io::Result<Vec<u8>> { | ||
| let (_, der_object) = parse_der(public_key_der).map_err(|err| { | ||
| io::Error::new( | ||
|
|
@@ -40,6 +51,12 @@ pub(crate) fn encrypt_with_public_key(message: &[u8], public_key_der: &[u8]) -> | |
| })?; | ||
|
|
||
| let n = BigUint::from_bytes_be(n); | ||
| 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"), | ||
| )); | ||
| } | ||
|
Comment on lines
+54
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| let e = BigUint::from_bytes_be(e); | ||
| let m = BigUint::from_bytes_le(message); | ||
| let c = m.modpow(&e, &n); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| use ironrdp_pdu::rdp::headers::{BasicSecurityHeader, BasicSecurityHeaderFlags}; | ||
| use ironrdp_pdu::rdp::server_license::cert::{CertificateType, ProprietaryCertificate, RsaPublicKey}; | ||
| use ironrdp_pdu::rdp::server_license::{ | ||
| ClientLicenseInfo, ClientNewLicenseRequest, LicenseHeader, PreambleFlags, PreambleType, PreambleVersion, | ||
| ProductInfo, ServerCertificate, ServerLicenseRequest, | ||
| }; | ||
| use rstest::rstest; | ||
|
|
||
| /// The zero padding that follows the modulus bytes of a proprietary certificate's public key. | ||
| const MODULUS_PADDING: [u8; 8] = [0; 8]; | ||
|
|
||
| /// The 512-bit modulus of the proprietary certificate in MS-RDPBCGR 4.1.4, in little-endian order without its padding. | ||
| const SPEC_EXAMPLE_MODULUS: [u8; 64] = [ | ||
| 0xcb, 0x81, 0xfe, 0xba, 0x6d, 0x61, 0xc3, 0x55, 0x05, 0xd5, 0x5f, 0x2e, 0x87, 0xf8, 0x71, 0x94, 0xd6, 0xf1, 0xa5, | ||
| 0xcb, 0xf1, 0x5f, 0x0c, 0x3d, 0xf8, 0x70, 0x02, 0x96, 0xc4, 0xfb, 0x9b, 0xc8, 0x3c, 0x2d, 0x55, 0xae, 0xe8, 0xff, | ||
| 0x32, 0x75, 0xea, 0x68, 0x79, 0xe5, 0xa2, 0x01, 0xfd, 0x31, 0xa0, 0xb1, 0x1f, 0x55, 0xa6, 0x1f, 0xc1, 0xf6, 0xd1, | ||
| 0x83, 0x88, 0x63, 0x26, 0x56, 0x12, 0xbc, | ||
| ]; | ||
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| enum LicenseResponse { | ||
| NewLicenseRequest, | ||
| LicenseInfo, | ||
| } | ||
|
|
||
| impl LicenseResponse { | ||
| /// Builds the response, which encrypts the premaster secret with the server's public key. | ||
| fn build(self, license_request: &ServerLicenseRequest) -> bool { | ||
| let client_random = [0x11; 32]; | ||
| let premaster_secret = [0x22; 48]; | ||
| match self { | ||
| Self::NewLicenseRequest => ClientNewLicenseRequest::from_server_license_request( | ||
| license_request, | ||
| &client_random, | ||
| &premaster_secret, | ||
| "user", | ||
| "machine", | ||
| ) | ||
| .is_ok(), | ||
| Self::LicenseInfo => ClientLicenseInfo::from_server_license_request( | ||
| license_request, | ||
| &client_random, | ||
| &premaster_secret, | ||
| [0; 4], | ||
| vec![0x33; 16], | ||
| ) | ||
| .is_ok(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[rstest] | ||
| // oracle: regression crates/ironrdp-testsuite-core/test_data/fuzz_regression/pdu_decode/crash-3639648ab6730a3d8617bc39badcc741c56bdc4f | ||
| #[case::zero_modulus(vec![0; 64], false)] | ||
|
Comment on lines
+53
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| // oracle: mutant a guard that rejects only a zero modulus, accepting a 256-bit key | ||
| #[case::modulus_below_512_bits(vec![0xff; 32], false)] | ||
|
Comment on lines
+55
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| // oracle: spec MS-RDPBCGR §4.1.4 | ||
| #[case::modulus_of_512_bits(SPEC_EXAMPLE_MODULUS.to_vec(), true)] | ||
|
Comment on lines
+54
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| fn licensing_accepts_only_moduli_of_at_least_512_bits( | ||
| #[case] modulus: Vec<u8>, | ||
| #[case] accepted: bool, | ||
| #[values(LicenseResponse::NewLicenseRequest, LicenseResponse::LicenseInfo)] response: LicenseResponse, | ||
| ) { | ||
| // The certificate carries `modulus` in little-endian order, followed by its padding. | ||
| let license_request = 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], | ||
| }), | ||
| }), | ||
|
Comment on lines
+81
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| scope_list: Vec::new(), | ||
| }; | ||
|
|
||
| assert_eq!(response.build(&license_request), accepted); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.