From 8b359ff4a30ae428384318ab154c181c0851d3d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Cortier?= Date: Mon, 30 Oct 2023 10:46:07 -0400 Subject: [PATCH] fix(pdu): bubble up X509 cert parsing error source (#236) The source for the certificate parsing error was discarded. This patch is modifying the ServerLicenseError::InvalidX509Certificate variant so that it is properly bubbled up. This will help us gather troubleshooting information from our end users. --- .../ironrdp-connector/src/license_exchange.rs | 67 +++++++++++------ crates/ironrdp-pdu/src/rdp/server_license.rs | 71 ++++++++++--------- .../server_license/server_license_request.rs | 10 ++- fuzz/Cargo.lock | 4 +- 4 files changed, 93 insertions(+), 59 deletions(-) diff --git a/crates/ironrdp-connector/src/license_exchange.rs b/crates/ironrdp-connector/src/license_exchange.rs index b272d698..6d7fd2b1 100644 --- a/crates/ironrdp-connector/src/license_exchange.rs +++ b/crates/ironrdp-connector/src/license_exchange.rs @@ -1,3 +1,4 @@ +use core::fmt; use std::mem; use ironrdp_pdu::rdp::server_license; @@ -103,30 +104,54 @@ impl Sequence for LicenseExchangeSequence { let mut premaster_secret = [0u8; server_license::PREMASTER_SECRET_SIZE]; OsRng.fill_bytes(&mut premaster_secret); - let (new_license_request, encryption_data) = - server_license::ClientNewLicenseRequest::from_server_license_request( - &license_request, - &client_random, - &premaster_secret, - &self.username, - self.domain.as_deref().unwrap_or(""), - ) - .map_err(|e| custom_err!("ClientNewLicenseRequest", e))?; + match server_license::ClientNewLicenseRequest::from_server_license_request( + &license_request, + &client_random, + &premaster_secret, + &self.username, + self.domain.as_deref().unwrap_or(""), + ) { + Ok((new_license_request, encryption_data)) => { + trace!(?encryption_data, "Successfully generated Client New License Request"); + info!(message = ?new_license_request, "Send"); - trace!(?encryption_data, "Successfully generated Client New License Request"); - info!(message = ?new_license_request, "Send"); + let written = legacy::encode_send_data_request( + send_data_indication_ctx.initiator_id, + send_data_indication_ctx.channel_id, + &new_license_request, + output, + )?; - let written = legacy::encode_send_data_request( - send_data_indication_ctx.initiator_id, - send_data_indication_ctx.channel_id, - &new_license_request, - output, - )?; + ( + Written::from_size(written)?, + LicenseExchangeState::PlatformChallenge { encryption_data }, + ) + } + Err(error) => { + if let server_license::ServerLicenseError::InvalidX509Certificate { + source: error, + cert_der, + } = &error + { + struct BytesHexFormatter<'a>(&'a [u8]); - ( - Written::from_size(written)?, - LicenseExchangeState::PlatformChallenge { encryption_data }, - ) + impl fmt::Display for BytesHexFormatter<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "0x")?; + self.0.iter().try_for_each(|byte| write!(f, "{byte:02X}")) + } + } + + error!( + %error, + cert_der = %BytesHexFormatter(cert_der), + "Unsupported or invalid X509 certificate received during license exchange step" + ); + } + + return Err(custom_err!("ClientNewLicenseRequest", error)); + } + } } server_license::InitialMessageType::StatusValidClient(_) => { info!("Server did not initiate license exchange"); diff --git a/crates/ironrdp-pdu/src/rdp/server_license.rs b/crates/ironrdp-pdu/src/rdp/server_license.rs index f8041290..55d4b25f 100644 --- a/crates/ironrdp-pdu/src/rdp/server_license.rs +++ b/crates/ironrdp-pdu/src/rdp/server_license.rs @@ -23,7 +23,9 @@ mod server_upgrade_license; pub use self::client_new_license_request::{ClientNewLicenseRequest, PLATFORM_ID}; pub use self::client_platform_challenge_response::ClientPlatformChallengeResponse; pub use self::licensing_error_message::{LicenseErrorCode, LicensingErrorMessage, LicensingStateTransition}; -pub use self::server_license_request::{cert, InitialMessageType, InitialServerLicenseMessage, ServerLicenseRequest}; +pub use self::server_license_request::{ + cert, InitialMessageType, InitialServerLicenseMessage, ProductInfo, Scope, ServerCertificate, ServerLicenseRequest, +}; pub use self::server_platform_challenge::ServerPlatformChallenge; pub use self::server_upgrade_license::{NewLicenseInformation, ServerUpgradeLicense}; @@ -167,69 +169,72 @@ pub enum ServerLicenseError { IOError(#[from] io::Error), #[error("UTF-8 error: {0}")] Utf8Error(#[from] std::string::FromUtf8Error), - #[error("Invalid preamble field: {0}")] + #[error("invalid preamble field: {0}")] InvalidPreamble(String), - #[error("Invalid preamble message type field")] + #[error("invalid preamble message type field")] InvalidLicenseType, - #[error("Invalid error code field")] + #[error("invalid error code field")] InvalidErrorCode, - #[error("Invalid state transition field")] + #[error("invalid state transition field")] InvalidStateTransition, - #[error("Invalid blob type field")] + #[error("invalid blob type field")] InvalidBlobType, - #[error("Unable to generate random number {0}")] + #[error("unable to generate random number {0}")] RandomNumberGenerationError(String), - #[error("Unable to retrieve public key from the certificate")] + #[error("unable to retrieve public key from the certificate")] UnableToGetPublicKey, - #[error("Unable to encrypt RSA public key")] + #[error("unable to encrypt RSA public key")] RsaKeyEncryptionError, - #[error("Invalid License Request key exchange algorithm value")] + #[error("invalid License Request key exchange algorithm value")] InvalidKeyExchangeValue, #[error("MAC checksum generated over decrypted data does not match the server's checksum")] InvalidMacData, - #[error("Invalid platform challenge response data version")] + #[error("invalid platform challenge response data version")] InvalidChallengeResponseDataVersion, - #[error("Invalid platform challenge response data client type")] + #[error("invalid platform challenge response data client type")] InvalidChallengeResponseDataClientType, - #[error("Invalid platform challenge response data license detail level")] + #[error("invalid platform challenge response data license detail level")] InvalidChallengeResponseDataLicenseDetail, - #[error("Invalid x509 certificate")] - InvalidX509Certificate, - #[error("Invalid certificate version")] + #[error("invalid x509 certificate")] + InvalidX509Certificate { + source: x509_cert::der::Error, + cert_der: Vec, + }, + #[error("invalid certificate version")] InvalidCertificateVersion, - #[error("Invalid x509 certificates amount")] + #[error("invalid x509 certificates amount")] InvalidX509CertificatesAmount, - #[error("Invalid proprietary certificate signature algorithm ID")] + #[error("invalid proprietary certificate signature algorithm ID")] InvalidPropCertSignatureAlgorithmId, - #[error("Invalid proprietary certificate key algorithm ID")] + #[error("invalid proprietary certificate key algorithm ID")] InvalidPropCertKeyAlgorithmId, - #[error("Invalid RSA public key magic")] + #[error("invalid RSA public key magic")] InvalidRsaPublicKeyMagic, - #[error("Invalid RSA public key length")] + #[error("invalid RSA public key length")] InvalidRsaPublicKeyLength, - #[error("Invalid RSA public key data length")] + #[error("invalid RSA public key data length")] InvalidRsaPublicKeyDataLength, - #[error("Invalid License Header security flags")] + #[error("invalid License Header security flags")] InvalidSecurityFlags, - #[error("The server returned unexpected error")] + #[error("ihe server returned unexpected error")] UnexpectedError(LicensingErrorMessage), - #[error("Got unexpected license message")] + #[error("got unexpected license message")] UnexpectedLicenseMessage, - #[error("The server has returned an unexpected error")] + #[error("the server has returned an unexpected error")] UnexpectedServerError(LicensingErrorMessage), - #[error("The server has returned STATUS_VALID_CLIENT (not an error)")] + #[error("the server has returned STATUS_VALID_CLIENT (not an error)")] ValidClientStatus(LicensingErrorMessage), - #[error("Invalid Key Exchange List field")] + #[error("invalid Key Exchange List field")] InvalidKeyExchangeAlgorithm, - #[error("Received invalid company name length (Product Information): {0}")] + #[error("received invalid company name length (Product Information): {0}")] InvalidCompanyNameLength(u32), - #[error("Received invalid product ID length (Product Information): {0}")] + #[error("received invalid product ID length (Product Information): {0}")] InvalidProductIdLength(u32), - #[error("Received invalid scope count field: {0}")] + #[error("received invalid scope count field: {0}")] InvalidScopeCount(u32), - #[error("Received invalid certificate length: {0}")] + #[error("received invalid certificate length: {0}")] InvalidCertificateLength(u32), - #[error("Blob too small")] + #[error("blob too small")] BlobTooSmall, } diff --git a/crates/ironrdp-pdu/src/rdp/server_license/server_license_request.rs b/crates/ironrdp-pdu/src/rdp/server_license/server_license_request.rs index d02e0ef3..920a22cc 100644 --- a/crates/ironrdp-pdu/src/rdp/server_license/server_license_request.rs +++ b/crates/ironrdp-pdu/src/rdp/server_license/server_license_request.rs @@ -286,13 +286,17 @@ impl ServerCertificate { Ok(public_key) } CertificateType::X509(certificate) => { - let der = certificate + let cert_der = certificate .certificate_array .last() .ok_or_else(|| ServerLicenseError::InvalidX509CertificatesAmount)?; - let cert = - x509_cert::Certificate::from_der(der).map_err(|_| ServerLicenseError::InvalidX509Certificate)?; + let cert = x509_cert::Certificate::from_der(cert_der).map_err(|source| { + ServerLicenseError::InvalidX509Certificate { + source, + cert_der: cert_der.clone(), + } + })?; let public_key = cert .tbs_certificate diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 9ec53438..5b9c1bfa 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -96,9 +96,9 @@ dependencies = [ [[package]] name = "byteorder" -version = "1.4.3" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "14c189c53d098945499cdfa7ecc63567cf3886b3332b312a5b4585d8d3a6a610" +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" [[package]] name = "cc"