From 0954f42519fd6942f7e04fb692467a4f1b387038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Cortier?= Date: Fri, 8 Dec 2023 10:59:48 -0500 Subject: [PATCH] fix(connector): better security protocol selection (#328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the bitflag-based config API with a boolean-based one: - `enable_tls`: set the PROTOCOL_SSL flag - `enable_credssp`: set the PROTOCOL_HYBRID and PROTOCOL_HYBRID_EX flags The `--security_protocol` argument was removed from the native client CLI, and instead it’s possible to disable specific protocols with `--no-tls` and `--no-credssp`. By default, both protocols are enabled for maximum compatibility with most RDP servers. We may change the defaults in the future. --- crates/ironrdp-async/src/connector.rs | 52 ++++---- crates/ironrdp-blocking/src/connector.rs | 50 ++++---- crates/ironrdp-client/src/config.rs | 46 +++---- crates/ironrdp-connector/src/connection.rs | 118 +++++++++--------- crates/ironrdp-connector/src/credssp.rs | 76 +++++------ crates/ironrdp-connector/src/lib.rs | 60 ++++++++- crates/ironrdp-pdu/src/macros.rs | 4 +- crates/ironrdp-pdu/src/nego.rs | 117 +++++++++++++---- crates/ironrdp-pdu/src/rdp/suppress_output.rs | 2 +- .../ironrdp-testsuite-core/src/core_data.rs | 6 +- .../ironrdp-testsuite-core/tests/pdu/x224.rs | 14 ++- crates/ironrdp-web/src/session.rs | 6 +- crates/ironrdp/examples/screenshot.rs | 4 +- 13 files changed, 338 insertions(+), 217 deletions(-) diff --git a/crates/ironrdp-async/src/connector.rs b/crates/ironrdp-async/src/connector.rs index dcb31862..bf5fc48e 100644 --- a/crates/ironrdp-async/src/connector.rs +++ b/crates/ironrdp-async/src/connector.rs @@ -120,30 +120,12 @@ where { assert!(connector.should_perform_credssp()); - let mut credssp_sequence = CredsspSequence::new(connector, server_name, server_public_key, kerberos_config)?; - - while !credssp_sequence.is_done() { - buf.clear(); - - if let Some(next_pdu_hint) = credssp_sequence.next_pdu_hint() { - debug!( - connector.state = connector.state.name(), - hint = ?next_pdu_hint, - "Wait for PDU" - ); - - let pdu = framed - .read_by_hint(next_pdu_hint) - .await - .map_err(|e| ironrdp_connector::custom_err!("read frame by hint", e))?; - - trace!(length = pdu.len(), "PDU received"); - - credssp_sequence.read_request_from_server(&pdu)?; - } + let (mut sequence, mut ts_request) = + CredsspSequence::init(connector, server_name, server_public_key, kerberos_config)?; + loop { let client_state = { - let mut generator = credssp_sequence.process(); + let mut generator = sequence.process_ts_request(ts_request); if let Some(network_client_ref) = network_client.as_deref_mut() { trace!("resolving network"); @@ -155,7 +137,8 @@ where } }; // drop generator - let written = credssp_sequence.handle_process_result(client_state, buf)?; + buf.clear(); + let written = sequence.handle_process_result(client_state, buf)?; if let Some(response_len) = written.size() { let response = &buf[..response_len]; @@ -165,6 +148,29 @@ where .await .map_err(|e| ironrdp_connector::custom_err!("write all", e))?; } + + let Some(next_pdu_hint) = sequence.next_pdu_hint() else { + break; + }; + + debug!( + connector.state = connector.state.name(), + hint = ?next_pdu_hint, + "Wait for PDU" + ); + + let pdu = framed + .read_by_hint(next_pdu_hint) + .await + .map_err(|e| ironrdp_connector::custom_err!("read frame by hint", e))?; + + trace!(length = pdu.len(), "PDU received"); + + if let Some(next_request) = sequence.decode_server_message(&pdu)? { + ts_request = next_request; + } else { + break; + } } connector.mark_credssp_as_done(); diff --git a/crates/ironrdp-blocking/src/connector.rs b/crates/ironrdp-blocking/src/connector.rs index 2fddf56d..30300c08 100644 --- a/crates/ironrdp-blocking/src/connector.rs +++ b/crates/ironrdp-blocking/src/connector.rs @@ -125,33 +125,17 @@ where { assert!(connector.should_perform_credssp()); - let mut credssp_sequence = CredsspSequence::new(connector, server_name, server_public_key, kerberos_config)?; - - while !credssp_sequence.is_done() { - buf.clear(); - - if let Some(next_pdu_hint) = credssp_sequence.next_pdu_hint() { - debug!( - connector.state = connector.state.name(), - hint = ?next_pdu_hint, - "Wait for PDU" - ); - - let pdu = framed - .read_by_hint(next_pdu_hint) - .map_err(|e| ironrdp_connector::custom_err!("read frame by hint", e))?; - - trace!(length = pdu.len(), "PDU received"); - - credssp_sequence.read_request_from_server(&pdu)?; - } + let (mut sequence, mut ts_request) = + CredsspSequence::init(connector, server_name, server_public_key, kerberos_config)?; + loop { let client_state = { - let mut generator = credssp_sequence.process(); + let mut generator = sequence.process_ts_request(ts_request); resolve_generator(&mut generator, network_client)? }; // drop generator - let written = credssp_sequence.handle_process_result(client_state, buf)?; + buf.clear(); + let written = sequence.handle_process_result(client_state, buf)?; if let Some(response_len) = written.size() { let response = &buf[..response_len]; @@ -160,6 +144,28 @@ where .write_all(response) .map_err(|e| ironrdp_connector::custom_err!("write all", e))?; } + + let Some(next_pdu_hint) = sequence.next_pdu_hint() else { + break; + }; + + debug!( + connector.state = connector.state.name(), + hint = ?next_pdu_hint, + "Wait for PDU" + ); + + let pdu = framed + .read_by_hint(next_pdu_hint) + .map_err(|e| ironrdp_connector::custom_err!("read frame by hint", e))?; + + trace!(length = pdu.len(), "PDU received"); + + if let Some(next_request) = sequence.decode_server_message(&pdu)? { + ts_request = next_request; + } else { + break; + } } connector.mark_credssp_as_done(); diff --git a/crates/ironrdp-client/src/config.rs b/crates/ironrdp-client/src/config.rs index 2d809c46..5ee73bf3 100644 --- a/crates/ironrdp-client/src/config.rs +++ b/crates/ironrdp-client/src/config.rs @@ -5,9 +5,8 @@ use std::str::FromStr; use anyhow::Context as _; use clap::clap_derive::ValueEnum; use clap::{crate_name, Parser}; -use ironrdp::connector::Credentials; +use ironrdp::connector::{self, Credentials}; use ironrdp::pdu::rdp::capability_sets::MajorPlatformType; -use ironrdp::{connector, pdu}; use tap::prelude::*; const DEFAULT_WIDTH: u16 = 1920; @@ -20,23 +19,6 @@ pub struct Config { pub connector: connector::Config, } -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)] -enum SecurityProtocol { - Ssl, - Hybrid, - HybridEx, -} - -impl SecurityProtocol { - fn parse(security_protocol: SecurityProtocol) -> pdu::nego::SecurityProtocol { - match security_protocol { - SecurityProtocol::Ssl => pdu::nego::SecurityProtocol::SSL, - SecurityProtocol::Hybrid => pdu::nego::SecurityProtocol::HYBRID, - SecurityProtocol::HybridEx => pdu::nego::SecurityProtocol::HYBRID_EX, - } - } -} - #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)] enum KeyboardType { IbmPcXt, @@ -168,10 +150,6 @@ struct Args { #[clap(short, long, value_parser)] password: Option, - /// Specify the security protocols to use - #[clap(long, value_enum, value_parser, default_value_t = SecurityProtocol::Hybrid)] - security_protocol: SecurityProtocol, - /// The keyboard type #[clap(long, value_enum, value_parser, default_value_t = KeyboardType::IbmEnhanced)] keyboard_type: KeyboardType, @@ -222,10 +200,25 @@ struct Args { #[clap(long, value_parser = parse_hex, default_value_t = 0)] capabilities: u32, - /// Automatically logon to the server by passing the INFO_AUTOLOGON flag. This flag is - /// ignored if CredSSP is used (SecurityProtocol::Hybrid | SecurityProtocol::HybridEx). + /// Automatically logon to the server by passing the INFO_AUTOLOGON flag + /// + /// This flag is ignored if CredSSP authentication is used. + /// You can use `--no-credssp` to ensure it’s not. #[clap(long)] autologon: bool, + + /// Disable TLS + Graphical login (legacy authentication method) + /// + /// Disabling this in order to enforce usage of CredSSP (NLA) is recommended. + #[clap(long)] + no_tls: bool, + + /// Disable TLS + Network Level Authentication (NLA) using CredSSP + /// + /// NLA is used to authenticates RDP clients and servers before sending credentials over the network. + /// It’s not recommended to disable this. + #[clap(long, alias = "no-nla")] + no_credssp: bool, } impl Config { @@ -284,7 +277,8 @@ impl Config { let connector = connector::Config { credentials: Credentials::UsernamePassword { username, password }, domain: args.domain, - security_protocol: SecurityProtocol::parse(args.security_protocol), + enable_tls: !args.no_tls, + enable_credssp: !args.no_credssp, keyboard_type: KeyboardType::parse(args.keyboard_type), keyboard_subtype: args.keyboard_subtype, keyboard_functional_keys_count: args.keyboard_functional_keys_count, diff --git a/crates/ironrdp-connector/src/connection.rs b/crates/ironrdp-connector/src/connection.rs index 051f6592..0299ac43 100644 --- a/crates/ironrdp-connector/src/connection.rs +++ b/crates/ironrdp-connector/src/connection.rs @@ -35,7 +35,9 @@ pub enum ClientConnectorState { Consumed, ConnectionInitiationSendRequest, - ConnectionInitiationWaitConfirm, + ConnectionInitiationWaitConfirm { + requested_protocol: nego::SecurityProtocol, + }, EnhancedSecurityUpgrade { selected_protocol: nego::SecurityProtocol, }, @@ -47,18 +49,11 @@ pub enum ClientConnectorState { }, BasicSettingsExchangeWaitResponse { connect_initial: mcs::ConnectInitial, - selected_protocol: nego::SecurityProtocol, }, ChannelConnection { - selected_protocol: nego::SecurityProtocol, io_channel_id: u16, channel_connection: ChannelConnectionSequence, }, - RdpSecurityCommencement { - selected_protocol: nego::SecurityProtocol, - io_channel_id: u16, - user_channel_id: u16, - }, SecureSettingsExchange { io_channel_id: u16, user_channel_id: u16, @@ -96,13 +91,12 @@ impl State for ClientConnectorState { match self { Self::Consumed => "Consumed", Self::ConnectionInitiationSendRequest => "ConnectionInitiationSendRequest", - Self::ConnectionInitiationWaitConfirm => "ConnectionInitiationWaitResponse", + Self::ConnectionInitiationWaitConfirm { .. } => "ConnectionInitiationWaitResponse", Self::EnhancedSecurityUpgrade { .. } => "EnhancedSecurityUpgrade", Self::Credssp { .. } => "Credssp", Self::BasicSettingsExchangeSendInitial { .. } => "BasicSettingsExchangeSendInitial", Self::BasicSettingsExchangeWaitResponse { .. } => "BasicSettingsExchangeWaitResponse", Self::ChannelConnection { .. } => "ChannelConnection", - Self::RdpSecurityCommencement { .. } => "RdpSecurityCommencement", Self::SecureSettingsExchange { .. } => "SecureSettingsExchange", Self::ConnectTimeAutoDetection { .. } => "ConnectTimeAutoDetection", Self::LicensingExchange { .. } => "LicensingExchange", @@ -196,13 +190,12 @@ impl Sequence for ClientConnector { match &self.state { ClientConnectorState::Consumed => None, ClientConnectorState::ConnectionInitiationSendRequest => None, - ClientConnectorState::ConnectionInitiationWaitConfirm => Some(&ironrdp_pdu::X224_HINT), + ClientConnectorState::ConnectionInitiationWaitConfirm { .. } => Some(&ironrdp_pdu::X224_HINT), ClientConnectorState::EnhancedSecurityUpgrade { .. } => None, ClientConnectorState::Credssp { .. } => None, ClientConnectorState::BasicSettingsExchangeSendInitial { .. } => None, ClientConnectorState::BasicSettingsExchangeWaitResponse { .. } => Some(&ironrdp_pdu::X224_HINT), ClientConnectorState::ChannelConnection { channel_connection, .. } => channel_connection.next_pdu_hint(), - ClientConnectorState::RdpSecurityCommencement { .. } => None, ClientConnectorState::SecureSettingsExchange { .. } => None, ClientConnectorState::ConnectTimeAutoDetection { .. } => None, ClientConnectorState::LicensingExchange { license_exchange, .. } => license_exchange.next_pdu_hint(), @@ -231,10 +224,36 @@ impl Sequence for ClientConnector { // Exchange supported security protocols and a few other connection flags. ClientConnectorState::ConnectionInitiationSendRequest => { debug!("Connection Initiation"); + + let mut security_protocol = nego::SecurityProtocol::empty(); + + if self.config.enable_tls { + security_protocol.insert(nego::SecurityProtocol::SSL); + } + + if self.config.enable_credssp { + // https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/902b090b-9cb3-4efc-92bf-ee13373371e3 + // The spec is stating that `PROTOCOL_SSL` "SHOULD" also be set when using `PROTOCOL_HYBRID`. + // > PROTOCOL_HYBRID (0x00000002) + // > Credential Security Support Provider protocol (CredSSP) (section 5.4.5.2). + // > If this flag is set, then the PROTOCOL_SSL (0x00000001) flag SHOULD also be set + // > because Transport Layer Security (TLS) is a subset of CredSSP. + // However, crucially, it’s not strictly required (not "MUST"). + // In fact, we purposefully choose to not set `PROTOCOL_SSL` unless `enable_winlogon` is `true`. + // This tells the server that we are not going to accept downgrading NLA to TLS security. + security_protocol.insert(nego::SecurityProtocol::HYBRID | nego::SecurityProtocol::HYBRID_EX); + } + + if security_protocol.is_standard_rdp_security() { + return Err(reason_err!("Initiation", "standard RDP security is not supported",)); + } + let connection_request = nego::ConnectionRequest { - nego_data: Some(nego::NegoRequestData::cookie(self.config.credentials.username().into())), + nego_data: Some(nego::NegoRequestData::cookie( + self.config.credentials.username().to_owned(), + )), flags: nego::RequestFlags::empty(), - protocol: self.config.security_protocol, + protocol: security_protocol, }; debug!(message = ?connection_request, "Send"); @@ -243,10 +262,12 @@ impl Sequence for ClientConnector { ( Written::from_size(written)?, - ClientConnectorState::ConnectionInitiationWaitConfirm, + ClientConnectorState::ConnectionInitiationWaitConfirm { + requested_protocol: security_protocol, + }, ) } - ClientConnectorState::ConnectionInitiationWaitConfirm => { + ClientConnectorState::ConnectionInitiationWaitConfirm { requested_protocol } => { let connection_confirm = ironrdp_pdu::decode::(input).map_err(ConnectorError::pdu)?; @@ -256,15 +277,16 @@ impl Sequence for ClientConnector { nego::ConnectionConfirm::Response { flags, protocol } => (flags, protocol), nego::ConnectionConfirm::Failure { code } => { error!(?code, "Received connection failure code"); - return Err(general_err!("connection failed")); + return Err(reason_err!("Initiation", "{code}")); } }; info!(?selected_protocol, ?flags, "Server confirmed connection"); - if !self.config.security_protocol.contains(selected_protocol) { - return Err(general_err!( - "server selected a security protocol that is unsupported by this client", + if !selected_protocol.intersects(requested_protocol) { + return Err(reason_err!( + "Initiation", + "client advertised {requested_protocol}, but server selected {selected_protocol}", )); } @@ -278,12 +300,13 @@ impl Sequence for ClientConnector { // NOTE: we assume the selected protocol is never the standard RDP security (RC4). // User code should match this variant and perform the appropriate upgrade (TLS handshake, etc). ClientConnectorState::EnhancedSecurityUpgrade { selected_protocol } => { - let next_state = if selected_protocol.contains(nego::SecurityProtocol::HYBRID) - || selected_protocol.contains(nego::SecurityProtocol::HYBRID_EX) + let next_state = if selected_protocol + .intersects(nego::SecurityProtocol::HYBRID | nego::SecurityProtocol::HYBRID_EX) { + debug!("Begin NLA using CredSSP"); ClientConnectorState::Credssp { selected_protocol } } else { - debug!("Skipped CredSSP"); + debug!("CredSSP is disabled, skipping NLA"); ClientConnectorState::BasicSettingsExchangeSendInitial { selected_protocol } }; @@ -300,6 +323,7 @@ impl Sequence for ClientConnector { // Exchange basic settings including Core Data, Security Data and Network Data. ClientConnectorState::BasicSettingsExchangeSendInitial { selected_protocol } => { debug!("Basic Settings Exchange"); + let client_gcc_blocks = create_gcc_blocks(&self.config, selected_protocol, self.static_channels.values()); @@ -311,16 +335,10 @@ impl Sequence for ClientConnector { ( Written::from_size(written)?, - ClientConnectorState::BasicSettingsExchangeWaitResponse { - connect_initial, - selected_protocol, - }, + ClientConnectorState::BasicSettingsExchangeWaitResponse { connect_initial }, ) } - ClientConnectorState::BasicSettingsExchangeWaitResponse { - connect_initial, - selected_protocol, - } => { + ClientConnectorState::BasicSettingsExchangeWaitResponse { connect_initial } => { let connect_response = legacy::decode_x224_packet::(input)?; debug!(message = ?connect_response, "Received"); @@ -361,7 +379,6 @@ impl Sequence for ClientConnector { ( Written::Nothing, ClientConnectorState::ChannelConnection { - selected_protocol, io_channel_id, channel_connection: ChannelConnectionSequence::new(io_channel_id, static_channel_ids), }, @@ -371,7 +388,6 @@ impl Sequence for ClientConnector { //== Channel Connection ==// // Connect every individual channel. ClientConnectorState::ChannelConnection { - selected_protocol, io_channel_id, mut channel_connection, } => { @@ -382,14 +398,12 @@ impl Sequence for ClientConnector { { debug_assert!(channel_connection.state.is_terminal()); - ClientConnectorState::RdpSecurityCommencement { - selected_protocol, + ClientConnectorState::SecureSettingsExchange { io_channel_id, user_channel_id, } } else { ClientConnectorState::ChannelConnection { - selected_protocol, io_channel_id, channel_connection, } @@ -400,25 +414,9 @@ impl Sequence for ClientConnector { //== RDP Security Commencement ==// // When using standard RDP security (RC4), a Security Exchange PDU is sent at this point. - // NOTE: IronRDP does not support RC4 security. - ClientConnectorState::RdpSecurityCommencement { - selected_protocol, - io_channel_id, - user_channel_id, - } => { - debug!("RDP Security Commencement"); - if selected_protocol == nego::SecurityProtocol::RDP { - return Err(general_err!("standard RDP Security (RC4 encryption) is not supported")); - } - - ( - Written::Nothing, - ClientConnectorState::SecureSettingsExchange { - io_channel_id, - user_channel_id, - }, - ) - } + // However, IronRDP does not support this unsecure security protocol (purposefully) and + // this part of the sequence is not implemented. + //==============================// //== Secure Settings Exchange ==// // Send Client Info PDU (information about supported types of compression, username, password, etc). @@ -427,6 +425,7 @@ impl Sequence for ClientConnector { user_channel_id, } => { debug!("Secure Settings Exchange"); + let routing_addr = self .server_addr .as_ref() @@ -459,7 +458,7 @@ impl Sequence for ClientConnector { user_channel_id, license_exchange: LicenseExchangeSequence::new( io_channel_id, - self.config.credentials.username().into(), + self.config.credentials.username().to_owned(), self.config.domain.clone(), ), }, @@ -474,6 +473,7 @@ impl Sequence for ClientConnector { mut license_exchange, } => { debug!("Licensing Exchange"); + let written = license_exchange.step(input, output)?; let next_state = if license_exchange.state.is_terminal() { @@ -512,6 +512,7 @@ impl Sequence for ClientConnector { user_channel_id, } => { debug!("Capabilities Exchange"); + let send_data_indication_ctx = legacy::decode_send_data_indication(input)?; let share_control_ctx = legacy::decode_share_control(send_data_indication_ctx)?; @@ -592,6 +593,7 @@ impl Sequence for ClientConnector { mut connection_finalization, } => { debug!("Connection Finalization"); + let written = connection_finalization.step(input, output)?; let next_state = if connection_finalization.state.is_terminal() { @@ -748,8 +750,8 @@ fn create_client_info_pdu(config: &Config, routing_addr: &SocketAddr) -> rdp::Cl let client_info = ClientInfo { credentials: Credentials { - username: config.credentials.username().into(), - password: config.credentials.secret().into(), + username: config.credentials.username().to_owned(), + password: config.credentials.secret().to_owned(), domain: config.domain.clone(), }, code_page: 0, // ignored if the keyboardLayout field of the Client Core Data is set to zero diff --git a/crates/ironrdp-connector/src/credssp.rs b/crates/ironrdp-connector/src/credssp.rs index 67b8547b..750c0255 100644 --- a/crates/ironrdp-connector/src/credssp.rs +++ b/crates/ironrdp-connector/src/credssp.rs @@ -68,36 +68,33 @@ pub type CredsspProcessGenerator<'a> = Generator<'a, NetworkRequest, sspi::Resul #[derive(Debug)] pub struct CredsspSequence { client: CredSspClient, - next_request: Option, state: CredsspState, selected_protocol: nego::SecurityProtocol, } #[derive(Debug, PartialEq)] pub(crate) enum CredsspState { - CredsspInitial, - CredsspReplyNeeded, - CredsspEarlyUserAuthResult, + Ongoing, + EarlyUserAuthResult, Finished, } impl CredsspSequence { pub fn next_pdu_hint(&self) -> Option<&dyn PduHint> { match self.state { - CredsspState::CredsspInitial => None, - CredsspState::CredsspReplyNeeded => Some(&CREDSSP_TS_REQUEST_HINT), - CredsspState::CredsspEarlyUserAuthResult => Some(&CREDSSP_EARLY_USER_AUTH_RESULT_HINT), + CredsspState::Ongoing => Some(&CREDSSP_TS_REQUEST_HINT), + CredsspState::EarlyUserAuthResult => Some(&CREDSSP_EARLY_USER_AUTH_RESULT_HINT), CredsspState::Finished => None, } } /// `server_name` must be the actual target server hostname (as opposed to the proxy) - pub fn new( + pub fn init( connector: &ClientConnector, server_name: ServerName, server_public_key: Vec, kerberos_config: Option, - ) -> ConnectorResult { + ) -> ConnectorResult<(Self, credssp::TsRequest)> { let config = &connector.config; if let crate::Credentials::SmartCard { .. } = config.credentials { return Err(general_err!( @@ -139,36 +136,41 @@ impl CredsspSequence { .map_err(|e| ConnectorError::new("CredSSP", ConnectorErrorKind::Credssp(e)))?; match connector.state { - ClientConnectorState::Credssp { selected_protocol } => Ok(Self { - client, - next_request: Some(credssp::TsRequest::default()), - state: CredsspState::CredsspInitial, - selected_protocol, - }), + ClientConnectorState::Credssp { selected_protocol } => { + let sequence = Self { + client, + state: CredsspState::Ongoing, + selected_protocol, + }; + + let initial_request = credssp::TsRequest::default(); + + Ok((sequence, initial_request)) + } _ => Err(general_err!("invalid connector state for CredSSP sequence")), } } - pub fn is_done(&self) -> bool { - self.state == CredsspState::Finished - } - - pub fn read_request_from_server(&mut self, input: &[u8]) -> ConnectorResult<()> { + /// Returns Some(ts_request) when a TS request is received from server, + /// and None when an early user auth result PDU is received instead. + pub fn decode_server_message(&mut self, input: &[u8]) -> ConnectorResult> { match self.state { - CredsspState::CredsspReplyNeeded => { + CredsspState::Ongoing => { let message = credssp::TsRequest::from_buffer(input).map_err(|e| custom_err!("TsRequest", e))?; debug!(?message, "Received"); - self.next_request = Some(message); - Ok(()) + Ok(Some(message)) } - CredsspState::CredsspEarlyUserAuthResult => { + CredsspState::EarlyUserAuthResult => { let early_user_auth_result = credssp::EarlyUserAuthResult::from_buffer(input) .map_err(|e| custom_err!("EarlyUserAuthResult", e))?; debug!(message = ?early_user_auth_result, "Received"); match early_user_auth_result { - credssp::EarlyUserAuthResult::Success => Ok(()), + credssp::EarlyUserAuthResult::Success => { + self.state = CredsspState::Finished; + Ok(None) + } credssp::EarlyUserAuthResult::AccessDenied => { Err(ConnectorError::new("CredSSP", ConnectorErrorKind::AccessDenied)) } @@ -180,31 +182,19 @@ impl CredsspSequence { } } - pub fn process(&mut self) -> CredsspProcessGenerator<'_> { - let request = self.next_request.take().expect("next request"); // FIXME: error handling + pub fn process_ts_request(&mut self, request: credssp::TsRequest) -> CredsspProcessGenerator<'_> { self.client.process(request) } pub fn handle_process_result(&mut self, result: ClientState, output: &mut WriteBuf) -> ConnectorResult { let (size, next_state) = match self.state { - CredsspState::CredsspInitial => { + CredsspState::Ongoing => { let (ts_request_from_client, next_state) = match result { - ClientState::ReplyNeeded(ts_request) => (ts_request, CredsspState::CredsspReplyNeeded), - ClientState::FinalMessage(ts_request) => (ts_request, CredsspState::Finished), - }; - debug!(message = ?ts_request_from_client, "Send"); - - let written = write_credssp_request(ts_request_from_client, output)?; - self.next_request = None; - Ok((Written::from_size(written)?, next_state)) - } - CredsspState::CredsspReplyNeeded => { - let (ts_request_from_client, next_state) = match result { - credssp::ClientState::ReplyNeeded(ts_request) => (ts_request, CredsspState::CredsspReplyNeeded), + credssp::ClientState::ReplyNeeded(ts_request) => (ts_request, CredsspState::Ongoing), credssp::ClientState::FinalMessage(ts_request) => ( ts_request, if self.selected_protocol.contains(nego::SecurityProtocol::HYBRID_EX) { - CredsspState::CredsspEarlyUserAuthResult + CredsspState::EarlyUserAuthResult } else { CredsspState::Finished }, @@ -214,10 +204,10 @@ impl CredsspSequence { debug!(message = ?ts_request_from_client, "Send"); let written = write_credssp_request(ts_request_from_client, output)?; - self.next_request = None; + Ok((Written::from_size(written)?, next_state)) } - CredsspState::CredsspEarlyUserAuthResult => Ok((Written::Nothing, CredsspState::Finished)), + CredsspState::EarlyUserAuthResult => Ok((Written::Nothing, CredsspState::Finished)), CredsspState::Finished => Err(general_err!("CredSSP sequence is already done")), }?; diff --git a/crates/ironrdp-connector/src/lib.rs b/crates/ironrdp-connector/src/lib.rs index eecfe4c2..c8cca110 100644 --- a/crates/ironrdp-connector/src/lib.rs +++ b/crates/ironrdp-connector/src/lib.rs @@ -24,7 +24,7 @@ pub use connection::{ClientConnector, ClientConnectorState, ConnectionResult}; pub use connection_finalization::{ConnectionFinalizationSequence, ConnectionFinalizationState}; use ironrdp_pdu::rdp::capability_sets; use ironrdp_pdu::write_buf::WriteBuf; -use ironrdp_pdu::{gcc, nego, PduHint}; +use ironrdp_pdu::{gcc, PduHint}; pub use license_exchange::{LicenseExchangeSequence, LicenseExchangeState}; pub use server_name::ServerName; pub use sspi; @@ -78,13 +78,61 @@ impl Credentials { #[derive(Debug, Clone)] #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] pub struct Config { + /// The initial desktop size to request pub desktop_size: DesktopSize, - pub security_protocol: nego::SecurityProtocol, + /// TLS + Graphical login (legacy) + /// + /// Also called SSL or TLS security protocol. + /// The PROTOCOL_SSL flag will be set. + /// + /// When this security protocol is negotiated, the RDP server will show a graphical login screen. + /// For Windows, it means that the login subsystem (winlogon.exe) and the GDI graphics subsystem + /// will be initiated and the user will authenticate himself using LogonUI.exe, as if + /// using the physical machine directly. + /// + /// This security protocol is being phased out because it’s not great security-wise. + /// Indeed, the whole RDP connection sequence will be performed, allowing anyone to effectively + /// open a RDP session session with all static channels joined and active (e.g.: I/O, clipboard, + /// sound, drive redirection, etc). This exposes a wide attack surface with many impacts on both + /// the client and the server. + /// + /// - Man-in-the-middle (MITM) + /// - Server-side takeover + /// - Client-side file stealing + /// - Client-side takeover + /// + /// Recommended reads on this topic: + /// + /// - + /// - + /// - + /// - + /// + /// By setting this option to `false`, it’s possible to effectively enforce usage of NLA on client side. + pub enable_tls: bool, + /// TLS + Network Level Authentication (NLA) using CredSSP + /// + /// The PROTOCOL_HYBRID and PROTOCOL_HYBRID_EX flags will be set. + /// + /// NLA is allowing authentication to be performed before session establishement. + /// + /// This option includes the extended CredSSP early user authorization result PDU. + /// This PDU is used by the server to deny access before any credentials (except for the username) + /// have been submitted, e.g.: typically if the user does not have the necessary remote access + /// privileges. + /// + /// The attack surface is considerably reduced in comparison to the legacy "TLS" security protocol. + /// For this reason, it is recommended to set `enable_tls` to `false` when connecting to NLA-capable + /// computers. + #[doc(alias("enable_nla", "nla"))] + pub enable_credssp: bool, pub credentials: Credentials, pub domain: Option, /// The build number of the client. pub client_build: u32, - /// Name of the client computer. Truncated to the 15 first characters. + /// Name of the client computer + /// + /// The name will be truncated to the 15 first characters. pub client_name: String, pub keyboard_type: gcc::KeyboardType, pub keyboard_subtype: u32, @@ -95,9 +143,11 @@ pub struct Config { pub dig_product_id: String, pub client_dir: String, pub platform: capability_sets::MajorPlatformType, - pub no_server_pointer: bool, - /// If true, the INFO_AUTOLOGON flag is set in the [`ironrdp_pdu::rdp::ClientInfoPdu`]. + /// If true, the INFO_AUTOLOGON flag is set in the [`ClientInfoPdu`](ironrdp_pdu::rdp::ClientInfoPdu) pub autologon: bool, + + // FIXME(@CBenoit): these are client-only options, not part of the connector. + pub no_server_pointer: bool, pub pointer_software_rendering: bool, } diff --git a/crates/ironrdp-pdu/src/macros.rs b/crates/ironrdp-pdu/src/macros.rs index a5728cbc..29c3cd5a 100644 --- a/crates/ironrdp-pdu/src/macros.rs +++ b/crates/ironrdp-pdu/src/macros.rs @@ -2,10 +2,10 @@ //! //! Some are exported and available to external crates -/// Automatically returns the full pathname to a -/// function. Taken from https://stackoverflow.com/a/40234666. +/// Finds the name of the function in which this macro is expanded #[macro_export] macro_rules! function { + // Taken from https://stackoverflow.com/a/40234666 () => {{ fn f() {} fn type_name_of(_: T) -> &'static str { diff --git a/crates/ironrdp-pdu/src/nego.rs b/crates/ironrdp-pdu/src/nego.rs index 0ec88a55..5884ea84 100644 --- a/crates/ironrdp-pdu/src/nego.rs +++ b/crates/ironrdp-pdu/src/nego.rs @@ -1,5 +1,7 @@ //! PDUs used during the Connection Initiation stage +use core::fmt; + use bitflags::bitflags; use tap::prelude::*; @@ -10,24 +12,48 @@ use crate::x224::X224Pdu; use crate::{Pdu as _, PduError, PduErrorExt as _, PduResult}; bitflags! { - /// A 32-bit, unsigned integer that contains flags indicating the supported - /// security protocols. - /// The client and server agree on it during the Connection Initiation phase. + /// A 32-bit, unsigned integer that contains flags indicating the supported security protocols. /// - /// # MSDN - /// - /// * [RDP Negotiation Request (RDP_NEG_REQ)](https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/902b090b-9cb3-4efc-92bf-ee13373371e3) + /// Used to negotiate the security protocol to use during the Connection Initiation phase using + /// the [`ConnectionConfirm`] and [`ConnectionRequest`] messages. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SecurityProtocol: u32 { - const RDP = 0x0000_0000; + /// PROTOCOL_SSL, TLS + login subsystem (winlogon.exe) const SSL = 0x0000_0001; + /// PROTOCOL_HYBRID, TLS + Credential Security Support Provider protocol (CredSSP) const HYBRID = 0x0000_0002; + /// PROTOCOL_RDSTLS, RDSTLS protocol const RDSTLS = 0x0000_0004; + /// PROTOCOL_HYBRID_EX, TLS + Credential Security Support Provider protocol (CredSSP) coupled with the Early User Authorization Result PDU const HYBRID_EX = 0x0000_0008; + /// PROTOCOL_RDSAAD, RDS-AAD-Auth Security const RDSAAD = 0x0000_0010; } } +impl SecurityProtocol { + /// Returns true if no enhanced security protocol is enabled + /// + /// The PROTOCOL_RDP bitmask is defined as 0x00000000. + /// Hence, this is logically equivalent to `SecurityProtocol::is_empty()`, but more explicit in the intention. + /// + /// As a server, to convey that the standard RDP security protocol has been chosen, no flag must be set. + /// As a client, the standard RDP security is always implied because there is no flag to set or unset. + pub fn is_standard_rdp_security(self) -> bool { + self.is_empty() + } +} + +impl fmt::Display for SecurityProtocol { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.is_standard_rdp_security() { + write!(f, "STANDARD_RDP_SECURITY") + } else { + bitflags::parser::to_writer(self, f) + } + } +} + bitflags! { /// Holds the negotiation protocol flags of the *request* message. /// @@ -58,17 +84,32 @@ bitflags! { } } -/// The type of the negotiation error. May be contained in ResponseData. +/// A 32-bit, unsigned integer that specifies the negotiation failure code #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct FailureCode(u32); impl FailureCode { + /// The server requires that the client support Enhanced RDP Security (section 5.4) + /// with either TLS 1.0, 1.1 or 1.2 (section 5.4.5.1) or CredSSP (section 5.4.5.2). + /// If only CredSSP was requested then the server only supports TLS. pub const SSL_REQUIRED_BY_SERVER: Self = Self(1); + /// The server is configured to only use Standard RDP Security mechanisms (section + /// 5.3) and does not support any External Security Protocols (section 5.4.5). pub const SSL_NOT_ALLOWED_BY_SERVER: Self = Self(2); + /// The server does not possess a valid authentication certificate and cannot + /// initialize the External Security Protocol Provider (section 5.4.5). pub const SSL_CERT_NOT_ON_SERVER: Self = Self(3); + /// The list of requested security protocols is not consistent with the current + /// security protocol in effect. This error is only possible when the Direct + /// Approach (sections 5.4.2.2 and 1.3.1.2) is used and an External Security + /// Protocol (section 5.4.5) is already being used. pub const INCONSISTENT_FLAGS: Self = Self(4); + /// The server requires that the client support Enhanced RDP Security (section 5.4) + /// with CredSSP (section 5.4.5.2). pub const HYBRID_REQUIRED_BY_SERVER: Self = Self(5); - /// Used when the failure caused by ResponseFailure. + /// The server requires that the client support Enhanced RDP Security (section + /// 5.4) with TLS 1.0, 1.1 or 1.2 (section 5.4.5.1) and certificate-based client + /// authentication. pub const SSL_WITH_USER_AUTH_REQUIRED_BY_SERVER: Self = Self(6); } @@ -84,6 +125,32 @@ impl From for u32 { } } +impl fmt::Display for FailureCode { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self { + Self::SSL_REQUIRED_BY_SERVER => { + write!(f, "enhanced RDP security required by server") + } + Self::SSL_NOT_ALLOWED_BY_SERVER => { + write!(f, "enhanced RDP security not allowed by server") + } + Self::SSL_CERT_NOT_ON_SERVER => { + write!(f, "no valid TLS authentication certificate on server") + } + Self::INCONSISTENT_FLAGS => { + write!(f, "inconsistent flags for security protocols") + } + Self::HYBRID_REQUIRED_BY_SERVER => { + write!(f, "CredSSP enhanced RDP security required by server") + } + Self::SSL_WITH_USER_AUTH_REQUIRED_BY_SERVER => { + write!(f, "TLS certificate-based client authentication required by server") + } + _ => write!(f, "unknown failure code: {}", self.0), + } + } +} + /// The kind of the negotiation request message. /// /// # MSDN @@ -208,11 +275,20 @@ impl<'de> X224Pdu<'de> for ConnectionRequest { nego_data.write(dst)?; } - if self.protocol != SecurityProtocol::RDP { - dst.write_u8(u8::from(NegoMsgType::REQUEST)); - dst.write_u8(self.flags.bits()); - dst.write_u16(Self::RDP_NEG_REQ_SIZE); - dst.write_u32(self.protocol.bits()); + // [MS-RDPBCGR] mentions the following payload as optional, but it appears that on recent + // versions of Windows, the server always expect to find this payload. + dst.write_u8(u8::from(NegoMsgType::REQUEST)); + dst.write_u8(self.flags.bits()); + dst.write_u16(Self::RDP_NEG_REQ_SIZE); + dst.write_u32(self.protocol.bits()); + + if self.flags.contains(RequestFlags::CORRELATION_INFO_PRESENT) { + // TODO(#111): support for RDP_NEG_CORRELATION_INFO + return Err(PduError::invalid_message( + Self::NAME, + "flags", + "CORRECTION_INFO_PRESENT flag is set, but not supported by IronRDP", + )); } Ok(()) @@ -266,21 +342,14 @@ impl<'de> X224Pdu<'de> for ConnectionRequest { Ok(Self { nego_data, flags: RequestFlags::empty(), - protocol: SecurityProtocol::RDP, + protocol: SecurityProtocol::empty(), }) } } fn tpdu_header_variable_part_size(&self) -> usize { let optional_nego_data_size = self.nego_data.as_ref().map(|data| data.size()).unwrap_or(0); - - let rdp_neg_req_size = if self.protocol == SecurityProtocol::RDP { - 0 - } else { - usize::from(Self::RDP_NEG_REQ_SIZE) - }; - - optional_nego_data_size + rdp_neg_req_size + optional_nego_data_size + usize::from(Self::RDP_NEG_REQ_SIZE) } fn tpdu_user_data_size(&self) -> usize { @@ -359,7 +428,7 @@ impl<'de> X224Pdu<'de> for ConnectionConfirm { } else { Ok(Self::Response { flags: ResponseFlags::empty(), - protocol: SecurityProtocol::RDP, + protocol: SecurityProtocol::empty(), }) } } diff --git a/crates/ironrdp-pdu/src/rdp/suppress_output.rs b/crates/ironrdp-pdu/src/rdp/suppress_output.rs index 86a8c487..98f7e4b5 100644 --- a/crates/ironrdp-pdu/src/rdp/suppress_output.rs +++ b/crates/ironrdp-pdu/src/rdp/suppress_output.rs @@ -34,7 +34,7 @@ impl AllowDisplayUpdatesType { /// restored. Server support for this PDU is indicated in the General Capability /// Set [2.2.7.1.1]. /// -/// [2.2.11.3.1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/0be71491-0b01-402c-947d-080706ccf91b +/// [2.2.11.3.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/0be71491-0b01-402c-947d-080706ccf91b #[derive(Debug, Clone, PartialEq, Eq)] pub struct SuppressOutputPdu { pub desktop_rect: Option, diff --git a/crates/ironrdp-testsuite-core/src/core_data.rs b/crates/ironrdp-testsuite-core/src/core_data.rs index 6205b5c7..606c5b03 100644 --- a/crates/ironrdp-testsuite-core/src/core_data.rs +++ b/crates/ironrdp-testsuite-core/src/core_data.rs @@ -82,7 +82,7 @@ lazy_static! { data.optional_data.early_capability_flags = Some(ClientEarlyCapabilityFlags::SUPPORT_ERR_INFO_PDU); data.optional_data.dig_product_id = Some(String::from("69712-783-0357974-42714")); data.optional_data.connection_type = Some(ConnectionType::NotUsed); - data.optional_data.server_selected_protocol = Some(SecurityProtocol::RDP); + data.optional_data.server_selected_protocol = Some(SecurityProtocol::empty()); data }; pub static ref CLIENT_CORE_DATA_WITH_ALL_OPTIONAL_FIELDS: ClientCoreData = { @@ -163,14 +163,14 @@ lazy_static! { pub static ref SERVER_CORE_DATA_TO_FLAGS: ServerCoreData = ServerCoreData { version: RdpVersion::V5_PLUS, optional_data: ServerCoreOptionalData { - client_requested_protocols: Some(SecurityProtocol::RDP), + client_requested_protocols: Some(SecurityProtocol::empty()), early_capability_flags: None, }, }; pub static ref SERVER_CORE_DATA_WITH_ALL_OPTIONAL_FIELDS: ServerCoreData = ServerCoreData { version: RdpVersion::V5_PLUS, optional_data: ServerCoreOptionalData { - client_requested_protocols: Some(SecurityProtocol::RDP), + client_requested_protocols: Some(SecurityProtocol::empty()), early_capability_flags: Some(ServerEarlyCapabilityFlags::EDGE_ACTIONS_SUPPORTED_V1), }, }; diff --git a/crates/ironrdp-testsuite-core/tests/pdu/x224.rs b/crates/ironrdp-testsuite-core/tests/pdu/x224.rs index ede4e4f3..288b203d 100644 --- a/crates/ironrdp-testsuite-core/tests/pdu/x224.rs +++ b/crates/ironrdp-testsuite-core/tests/pdu/x224.rs @@ -78,35 +78,36 @@ encode_decode_test! { ConnectionRequest { nego_data: None, flags: RequestFlags::empty(), - protocol: SecurityProtocol::RDP, + protocol: SecurityProtocol::empty(), }, [ // tpkt header 0x03, // version 0x00, // reserved - 0x00, 0x0B, // length in BE + 0x00, 0x13, // length in BE // tpdu header - 0x06, // length + 0x0E, // length 0xE0, // code 0x00, 0x00, // dst_ref 0x00, 0x00, // src_ref 0x00, // class // variable part + 0x01, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, // RDP_NEG_REQ ]; nego_connection_request_rdp_security_with_cookie: ConnectionRequest { nego_data: Some(NegoRequestData::Cookie(Cookie("User".to_owned()))), flags: RequestFlags::empty(), - protocol: SecurityProtocol::RDP, + protocol: SecurityProtocol::empty(), }, [ // tpkt header 0x03, // version 0x00, // reserved - 0x00, 0x22, // length in BE + 0x00, 0x2A, // length in BE // tpdu header - 0x1D, // length + 0x25, // length 0xE0, // code 0x00, 0x00, // dst_ref 0x00, 0x00, // src_ref @@ -114,6 +115,7 @@ encode_decode_test! { // variable part 0x43, 0x6F, 0x6F, 0x6B, 0x69, 0x65, 0x3A, 0x20, 0x6D, 0x73, 0x74, 0x73, 0x68, 0x61, 0x73, 0x68, 0x3D, 0x55, 0x73, 0x65, 0x72, 0x0D, 0x0A, // cookie + 0x01, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, // RDP_NEG_REQ ]; nego_connection_request_ssl_security_with_cookie: diff --git a/crates/ironrdp-web/src/session.rs b/crates/ironrdp-web/src/session.rs index 4cc0d61c..04164354 100644 --- a/crates/ironrdp-web/src/session.rs +++ b/crates/ironrdp-web/src/session.rs @@ -704,7 +704,9 @@ fn build_config( connector::Config { credentials: Credentials::UsernamePassword { username, password }, domain, - security_protocol: ironrdp::pdu::nego::SecurityProtocol::HYBRID, + // TODO(#327): expose these options from the WASM module. + enable_tls: true, + enable_credssp: true, keyboard_type: ironrdp::pdu::gcc::KeyboardType::IbmEnhanced, keyboard_subtype: 0, keyboard_functional_keys_count: 12, @@ -898,7 +900,7 @@ where connector.attach_server_addr(server_addr); - let ironrdp::connector::ClientConnectorState::ConnectionInitiationWaitConfirm = connector.state else { + let ironrdp::connector::ClientConnectorState::ConnectionInitiationWaitConfirm { .. } = connector.state else { return Err(anyhow::Error::msg("invalid connector state (wait confirm)").into()); }; diff --git a/crates/ironrdp/examples/screenshot.rs b/crates/ironrdp/examples/screenshot.rs index bc4e985e..e2c7d163 100644 --- a/crates/ironrdp/examples/screenshot.rs +++ b/crates/ironrdp/examples/screenshot.rs @@ -28,7 +28,6 @@ use ironrdp::connector; use ironrdp::connector::sspi::network_client::reqwest_network_client::ReqwestNetworkClient; use ironrdp::connector::ConnectionResult; use ironrdp::pdu::gcc::KeyboardType; -use ironrdp::pdu::nego::SecurityProtocol; use ironrdp::pdu::rdp::capability_sets::MajorPlatformType; use ironrdp::session::image::DecodedImage; use ironrdp::session::{ActiveStage, ActiveStageOutput}; @@ -177,7 +176,8 @@ fn build_config(username: String, password: String, domain: Option) -> c connector::Config { credentials: Credentials::UsernamePassword { username, password }, domain, - security_protocol: SecurityProtocol::HYBRID, + enable_tls: false, // This example does not expose any frontend. + enable_credssp: true, keyboard_type: KeyboardType::IbmEnhanced, keyboard_subtype: 0, keyboard_functional_keys_count: 12,