From ca11e338d7231c86f60a110627a5d864377d8594 Mon Sep 17 00:00:00 2001 From: Gabriel Bauman <967743+gabrielbauman@users.noreply.github.com> Date: Thu, 28 Aug 2025 04:06:21 -0700 Subject: [PATCH] feat: preserve RDP negotiation failure details in RDCleanPath error responses (#930) * Both web and desktop clients check for X.224 negotiation failure data in RDCleanPath error responses before falling back to generic errors * When X.224 Connection Confirm failure is found, convert to specific NegotiationFailure error type instead of generic RDCleanPath error * Enable clients to show meaningful error messages like "CredSSP authentication required" instead of generic connection failures * Maintain backward compatibility - existing proxies sending empty x224_connection_pdu continue working as before * Helper for proxies creating an RDCleanPath error with server response --- crates/ironrdp-client/src/rdp.rs | 43 +++++++++++++++++++- crates/ironrdp-rdcleanpath/src/lib.rs | 57 +++++++++++++++++++++++++-- crates/ironrdp-web/src/session.rs | 25 +++++++++++- 3 files changed, 120 insertions(+), 5 deletions(-) diff --git a/crates/ironrdp-client/src/rdp.rs b/crates/ironrdp-client/src/rdp.rs index 77687cd5..6b131e92 100644 --- a/crates/ironrdp-client/src/rdp.rs +++ b/crates/ironrdp-client/src/rdp.rs @@ -410,6 +410,26 @@ where debug!(message = ?rdcleanpath_res, "Received RDCleanPath PDU"); + // Check for negotiation failure data before consuming the PDU + if let Some(x224_data) = &rdcleanpath_res.x224_connection_pdu { + if let Some(_error) = &rdcleanpath_res.error { + // Try to decode as X.224 Connection Confirm to extract negotiation failure details. + if let Ok(x224_confirm) = ironrdp_core::decode::< + ironrdp::pdu::x224::X224, + >(x224_data.as_bytes()) + { + if let ironrdp::pdu::nego::ConnectionConfirm::Failure { code } = x224_confirm.0 { + // Convert to negotiation failure instead of generic RDCleanPath error. + let negotiation_failure = connector::NegotiationFailure::from(code); + return Err(connector::ConnectorError::new( + "RDP negotiation failed", + connector::ConnectorErrorKind::Negotiation(negotiation_failure), + )); + } + } + } + } + let (x224_connection_response, server_cert_chain) = match rdcleanpath_res .into_enum() .map_err(|e| connector::custom_err!("invalid RDCleanPath PDU", e))? @@ -424,9 +444,30 @@ where server_cert_chain, server_addr: _, } => (x224_connection_response, server_cert_chain), - ironrdp_rdcleanpath::RDCleanPath::Err(error) => { + ironrdp_rdcleanpath::RDCleanPath::GeneralErr(error) => { + // Fallback to generic RDCleanPath error if no negotiation failure data found return Err(connector::custom_err!("received an RDCleanPath error", error)); } + ironrdp_rdcleanpath::RDCleanPath::NegotiationErr { + x224_connection_response, + } => { + // Try to decode as X.224 Connection Confirm to extract negotiation failure details. + if let Ok(x224_confirm) = ironrdp_core::decode::< + ironrdp::pdu::x224::X224, + >(&x224_connection_response) + { + if let ironrdp::pdu::nego::ConnectionConfirm::Failure { code } = x224_confirm.0 { + // Convert to negotiation failure instead of generic RDCleanPath error. + let negotiation_failure = connector::NegotiationFailure::from(code); + return Err(connector::ConnectorError::new( + "RDP negotiation failed", + connector::ConnectorErrorKind::Negotiation(negotiation_failure), + )); + } + } + // Fallback to generic error if we can't decode the negotiation failure. + return Err(connector::general_err!("received an RDCleanPath negotiation error")); + } }; let connector::ClientConnectorState::ConnectionInitiationWaitConfirm { .. } = connector.state else { diff --git a/crates/ironrdp-rdcleanpath/src/lib.rs b/crates/ironrdp-rdcleanpath/src/lib.rs index a495f766..90a0f70e 100644 --- a/crates/ironrdp-rdcleanpath/src/lib.rs +++ b/crates/ironrdp-rdcleanpath/src/lib.rs @@ -13,6 +13,7 @@ pub const BASE_VERSION: u64 = 3389; pub const VERSION_1: u64 = BASE_VERSION + 1; pub const GENERAL_ERROR_CODE: u16 = 1; +pub const NEGOTIATION_ERROR_CODE: u16 = 2; #[derive(Clone, Debug, Eq, PartialEq, der::Sequence)] #[asn1(tag_mode = "EXPLICIT")] @@ -258,6 +259,34 @@ impl RDCleanPathPdu { } } + /// Create a negotiation error response that includes the server's X.224 negotiation response. + /// + /// This allows clients to extract specific negotiation failure details + /// (like "CredSSP required") from the server's original response. + /// + /// # Example + /// ```rust + /// use ironrdp_rdcleanpath::RDCleanPathPdu; + /// + /// // Server rejected connection with "CredSSP required" - preserve this info + /// let server_response = vec![/* X.224 Connection Confirm with failure code */]; + /// let error_pdu = RDCleanPathPdu::new_negotiation_error(server_response)?; + /// # Ok::<(), der::Error>(()) + /// ``` + pub fn new_negotiation_error(server_x224_response: Vec) -> der::Result { + Ok(Self { + version: VERSION_1, + error: Some(RDCleanPathErr { + error_code: NEGOTIATION_ERROR_CODE, + http_status_code: None, + wsa_last_error: None, + tls_alert_code: None, + }), + x224_connection_pdu: Some(OctetString::new(server_x224_response)?), + ..Self::default() + }) + } + pub fn to_der(&self) -> der::Result> { der::Encode::to_der(self) } @@ -278,7 +307,10 @@ pub enum RDCleanPath { server_cert_chain: Vec, server_addr: String, }, - Err(RDCleanPathErr), + GeneralErr(RDCleanPathErr), + NegotiationErr { + x224_connection_response: Vec, + }, } impl RDCleanPath { @@ -323,7 +355,13 @@ impl TryFrom for RDCleanPath { server_addr, } } else { - Self::Err(pdu.error.ok_or(MissingRDCleanPathField("error"))?) + let error = pdu.error.ok_or(MissingRDCleanPathField("error"))?; + match (error.error_code, pdu.x224_connection_pdu) { + (NEGOTIATION_ERROR_CODE, Some(x224_pdu)) => Self::NegotiationErr { + x224_connection_response: x224_pdu.as_bytes().to_vec(), + }, + _ => Self::GeneralErr(error), + } }; Ok(rdcleanpath) @@ -359,11 +397,24 @@ impl From for RDCleanPathPdu { server_addr: Some(server_addr), ..Default::default() }, - RDCleanPath::Err(error) => Self { + RDCleanPath::GeneralErr(error) => Self { version: VERSION_1, error: Some(error), ..Default::default() }, + RDCleanPath::NegotiationErr { + x224_connection_response, + } => Self { + version: VERSION_1, + error: Some(RDCleanPathErr { + error_code: NEGOTIATION_ERROR_CODE, + http_status_code: None, + wsa_last_error: None, + tls_alert_code: None, + }), + x224_connection_pdu: Some(OctetString::new(x224_connection_response).unwrap()), + ..Default::default() + }, } } } diff --git a/crates/ironrdp-web/src/session.rs b/crates/ironrdp-web/src/session.rs index ffd16ef9..fd79f5a8 100644 --- a/crates/ironrdp-web/src/session.rs +++ b/crates/ironrdp-web/src/session.rs @@ -1089,12 +1089,35 @@ where server_cert_chain, server_addr: _, } => (x224_connection_response, server_cert_chain), - ironrdp_rdcleanpath::RDCleanPath::Err(error) => { + ironrdp_rdcleanpath::RDCleanPath::GeneralErr(error) => { return Err( IronError::from(anyhow::Error::new(error).context("received an RDCleanPath error")) .with_kind(IronErrorKind::RDCleanPath), ); } + ironrdp_rdcleanpath::RDCleanPath::NegotiationErr { + x224_connection_response, + } => { + // Try to decode as X.224 Connection Confirm to extract negotiation failure details. + if let Ok(x224_confirm) = ironrdp_core::decode::< + ironrdp::pdu::x224::X224, + >(&x224_connection_response) + { + if let ironrdp::pdu::nego::ConnectionConfirm::Failure { code } = x224_confirm.0 { + // Convert to negotiation failure instead of generic RDCleanPath error. + let negotiation_failure = connector::NegotiationFailure::from(code); + return Err(IronError::from( + anyhow::Error::new(negotiation_failure).context("RDP negotiation failed"), + ) + .with_kind(IronErrorKind::NegotiationFailure)); + } + } + // Fallback to generic error if we can't decode the negotiation failure. + return Err( + IronError::from(anyhow::Error::msg("received an RDCleanPath negotiation error")) + .with_kind(IronErrorKind::RDCleanPath), + ); + } }; let connector::ClientConnectorState::ConnectionInitiationWaitConfirm { .. } = connector.state else {