Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 32 additions & 4 deletions crates/ironrdp-acceptor/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,16 @@ pub struct Acceptor {
client_cluster_data: Option<gcc::ClientClusterData>,
password_cookie: Option<PasswordCookie>,
client_info_flags: Option<ClientInfoFlags>,
alternate_shell: String,
work_dir: String,
performance_flags: PerformanceFlags,
remote_app_capabilities: Option<RemoteAppCapabilities>,
server_capabilities: Vec<CapabilitySet>,
static_channels: StaticChannelSet,
pending_input_events: InputEventBuffer,
saved_for_reactivation: AcceptorState,
pub(crate) creds: Option<Credentials>,
/// Accepted credentials; empty accepts any (ClientInfo) and fails CredSSP.
pub(crate) creds: Vec<Credentials>,
pub(crate) credssp_backend: crate::CredsspBackend,
received_credentials: Option<ReceivedCredentials>,
credentials_handled: bool,
Expand Down Expand Up @@ -458,6 +461,13 @@ pub struct AcceptorResult {
///
/// [2.2.1.11.1.1]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/732394f5-e2b5-4ac5-8a0a-35345386b0d1
pub client_info_flags: ClientInfoFlags,
/// The program the client asked the session to run instead of the shell,
/// from its Client Info PDU ([MS-RDPBCGR] 2.2.1.11.1.1, `AlternateShell`).
/// Empty when it asked for none. A server without such programs may read
/// it as a client preference.
pub alternate_shell: String,
/// The working directory for [`Self::alternate_shell`] (`WorkingDir`); empty when none.
pub work_dir: String,
Comment on lines +464 to +470

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[rust-style] alternate_shell/work_dir docs cite MS-RDPBCGR 2.2.1.11.1.1 without a reference-style link — low 🟡 — The added doc comments cite the section inline as ([MS-RDPBCGR] 2.2.1.11.1.1, AlternateShell) and (WorkingDir) with no link target, while the neighboring pre-existing client_info_flags doc (453-462) already defines the reference-style [2.2.1.11.1.1] link for the same Info Packet section. Reuse that link in the new field docs per the doc-comment convention.

}

impl Acceptor {
Expand Down Expand Up @@ -488,13 +498,15 @@ impl Acceptor {
client_cluster_data: None,
password_cookie: None,
client_info_flags: None,
alternate_shell: String::new(),
work_dir: String::new(),
performance_flags: PerformanceFlags::empty(),
remote_app_capabilities: None,
server_capabilities: capabilities,
static_channels: StaticChannelSet::new(),
pending_input_events: InputEventBuffer::default(),
saved_for_reactivation: Default::default(),
creds,
creds: creds.into_iter().collect(),
credssp_backend: crate::CredsspBackend::default(),
received_credentials: None,
credentials_handled: false,
Expand Down Expand Up @@ -561,6 +573,15 @@ impl Acceptor {
self.honor_client_desktop_size = max;
}

/// Accept any of `creds`, replacing the credentials given to [`Self::new`].
///
/// CredSSP checks the client's password against the entry with its user
/// name; ClientInfo credentials must match one of them. An empty list
/// accepts any ClientInfo credentials and fails CredSSP, as `None` does.
pub fn set_credential_list(&mut self, creds: Vec<Credentials>) {
self.creds = creds;
}

/// Selects the CredSSP authentication provider before negotiation begins.
pub fn set_credssp_backend(&mut self, backend: crate::CredsspBackend) {
self.credssp_backend = backend;
Expand Down Expand Up @@ -748,6 +769,8 @@ impl Acceptor {
client_cluster_data: consumed.client_cluster_data,
password_cookie: consumed.password_cookie,
client_info_flags: consumed.client_info_flags,
alternate_shell: consumed.alternate_shell,
work_dir: consumed.work_dir,
performance_flags: consumed.performance_flags,
remote_app_capabilities: None,
server_capabilities: consumed.server_capabilities,
Expand Down Expand Up @@ -1003,6 +1026,8 @@ impl Acceptor {
client_cluster_data: self.client_cluster_data.clone(),
password_cookie: self.password_cookie.clone(),
client_info_flags: self.client_info_flags.unwrap_or_else(ClientInfoFlags::empty),
alternate_shell: self.alternate_shell.clone(),
work_dir: self.work_dir.clone(),
}),
previous_state => {
self.state = previous_state;
Expand Down Expand Up @@ -1509,6 +1534,9 @@ impl Sequence for Acceptor {
let client_info: rdp::ClientInfoPdu =
decode(data.user_data.as_ref()).map_err(ConnectorError::decode)?;
self.client_info_flags = Some(client_info.client_info.flags);
self.alternate_shell
.clone_from(&client_info.client_info.alternate_shell);
self.work_dir.clone_from(&client_info.client_info.work_dir);
self.performance_flags = client_info
.client_info
.extra_info
Expand All @@ -1534,8 +1562,8 @@ impl Sequence for Acceptor {
let creds = client_info.client_info.credentials;

if !self.allow_unverified_credentials
&& let Some(expected) = &self.creds
&& !credentials_match(expected, &creds)
&& !self.creds.is_empty()
&& !self.creds.iter().any(|expected| credentials_match(expected, &creds))
Comment on lines +1565 to +1566

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[test] Empty-credential-list semantics (accept any ClientInfo, fail CredSSP) are documented but untested — medium 🟠 — The PR makes an empty creds list skip ClientInfo validation entirely (1565-1566, accepting arbitrary client-supplied credentials on non-CredSSP paths) while lib.rs fails CredSSP on an empty list, and the doc comments state both halves as intended (mirroring the old None). No added or existing test exercises set_credential_list(vec![]): neither that arbitrary Client Info credentials are accepted, nor that CredSSP fails. This is security-relevant acceptance behavior introduced by this diff; each half needs a named test pinning it (the author's documented intent is the apparent source).

{
self.encode_access_denied(output)?;
return Err(ConnectorError::general("invalid credentials"));
Expand Down
32 changes: 26 additions & 6 deletions crates/ironrdp-acceptor/src/credssp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,11 @@ pub struct CredsspSequence<'a> {

#[derive(Debug)]
struct CredentialsProxyImpl<'a> {
credentials: &'a AuthIdentity,
credentials: &'a [AuthIdentity],
}

impl<'a> CredentialsProxyImpl<'a> {
fn new(credentials: &'a AuthIdentity) -> Self {
fn new(credentials: &'a [AuthIdentity]) -> Self {
Self { credentials }
}
}
Expand All @@ -93,18 +93,22 @@ impl CredentialsProxy for CredentialsProxyImpl<'_> {
type AuthenticationData = AuthIdentity;

fn auth_data_by_user(&mut self, username: &Username) -> std::io::Result<Self::AuthenticationData> {
if !usernames_match(&self.credentials.username, username) {
let Some(credentials) = self
.credentials
.iter()
.find(|credentials| usernames_match(&credentials.username, username))
else {
return Err(std::io::Error::other("invalid username"));
}
};

let mut data = self.credentials.clone();
let mut data = credentials.clone();
// keep the original user/domain
data.username = username.clone();
Ok(data)
}

fn auth_data(&mut self) -> Result<Vec<Self::AuthenticationData>, std::io::Error> {
Ok(vec![self.credentials.clone()])
Ok(self.credentials.to_vec())
}
}

Expand Down Expand Up @@ -222,6 +226,22 @@ impl<'a> CredsspSequence<'a> {
client_computer_name: ServerName,
public_key: Vec<u8>,
krb_config: Option<KerberosServerConfig>,
) -> ConnectorResult<Self> {
Self::init_with_identities(
core::slice::from_ref(creds),
client_computer_name,
public_key,
krb_config,
)
}

/// Like [`Self::init`], accepting any of several identities: the client's
/// user name picks the one its password is checked against.
pub fn init_with_identities(
creds: &'a [AuthIdentity],
client_computer_name: ServerName,
public_key: Vec<u8>,
krb_config: Option<KerberosServerConfig>,
) -> ConnectorResult<Self> {
let client_computer_name = client_computer_name.into_inner();
let new_server = |server_mode| {
Expand Down
41 changes: 25 additions & 16 deletions crates/ironrdp-acceptor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,23 +483,32 @@ where
}
}

let creds = acceptor
.creds
.as_ref()
.ok_or_else(|| general_err!("no credentials while doing credssp"))?;
let username = if creds.domain.is_none() && creds.username.contains(['\\', '@']) {
Username::parse(&creds.username)
} else {
Username::new(&creds.username, creds.domain.as_deref())
if acceptor.creds.is_empty() {
return Err(general_err!("no credentials while doing credssp"));
}
.map_err(|e| custom_err!("invalid username", e))?;
let identity = AuthIdentity {
username,
password: creds.password.clone().into(),
};

let mut sequence =
credssp::CredsspSequence::init(&identity, client_computer_name, public_key, kerberos_config)?;
let identities = acceptor
.creds
.iter()
.map(|creds| {
let username = if creds.domain.is_none() && creds.username.contains(['\\', '@']) {
Username::parse(&creds.username)
} else {
Username::new(&creds.username, creds.domain.as_deref())
}
.map_err(|e| custom_err!("invalid username", e))?;
Ok(AuthIdentity {
username,
password: creds.password.clone().into(),
})
})
.collect::<ConnectorResult<Vec<_>>>()?;

let mut sequence = credssp::CredsspSequence::init_with_identities(
&identities,
client_computer_name,
public_key,
kerberos_config,
)?;

loop {
let Some(next_pdu_hint) = sequence.next_pdu_hint()? else {
Expand Down
16 changes: 13 additions & 3 deletions crates/ironrdp-rdpdr/src/pdu/efs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,16 @@ impl CapabilityMessage {
}
}

/// Whether this is a general capability set whose `extendedPDU` has
/// `RDPDR_USER_LOGGEDON_PDU`: the client takes the Server User Logged On PDU
/// ([MS-RDPEFS] 2.2.2.7.1).
pub fn user_logged_on_supported(&self) -> bool {
match &self.capability_data {
CapabilityData::General(general) => general.extended_pdu.contains(ExtendedPdu::RDPDR_USER_LOGGEDON_PDU),
_ => false,
}
}

/// Creates a new [`SMARTCARD_CAPS_SET`].
///
/// [`SMARTCARD_CAPS_SET`]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpefs/e02de60a-4d32-4dc7-ab17-9d591129eb93
Expand Down Expand Up @@ -1525,7 +1535,7 @@ impl DeviceAnnounceHeader {
self.preferred_dos_name.0.strip_prefix("PRN")?.parse().ok()
}

pub(crate) fn device_type(&self) -> DeviceType {
pub fn device_type(&self) -> DeviceType {
self.device_type
}

Expand Down Expand Up @@ -5593,9 +5603,9 @@ impl FileAllocationInformation {
/// [2.2.3.4.9]: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpefs/16b893d5-5d8b-49d1-8dcb-ee21e7612970
#[derive(Debug, PartialEq, Clone)]
pub struct ClientDriveSetInformationResponse {
pub(crate) device_io_reply: DeviceIoResponse,
pub device_io_reply: DeviceIoResponse,
/// This field MUST be equal to the Length field in the Server Drive Set Information Request (section 2.2.3.3.9).
pub(crate) length: u32,
pub length: u32,
}

impl ClientDriveSetInformationResponse {
Expand Down
15 changes: 14 additions & 1 deletion crates/ironrdp-rdpdr/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,20 @@ impl RdpdrServer {
"received ClientCoreCapabilityResponse"
);
self.state = RdpdrServerState::Active;
Ok(Vec::new())
// [MS-RDPEFS] 3.3.5.1.5: the server sends Server User Logged On once the
// user has logged on, to a client whose general capability set has
// RDPDR_USER_LOGGEDON_PDU; clients announce their drives only after it
// (3.1.5.1.5). The user was authenticated with the connection, so the
// PDU follows the capability exchange at once.
if response
.capabilities
.iter()
.any(CapabilityMessage::user_logged_on_supported)
{
Ok(vec![SvcMessage::from(RdpdrPdu::UserLoggedon)])
} else {
Ok(Vec::new())
}
}

fn handle_device_list_announce(&mut self, src: &mut ReadCursor<'_>) -> PduResult<Vec<SvcMessage>> {
Expand Down
3 changes: 1 addition & 2 deletions crates/ironrdp-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ test = false

[features]
default = ["rayon", "qoi", "qoiz"]
helper = ["dep:x509-cert", "dep:rustls-pemfile"]
helper = ["dep:x509-cert"]
rayon = ["dep:rayon"]
qoi = ["dep:qoicoubeh", "ironrdp-pdu/qoi"]
qoiz = ["dep:zstd-safe", "qoi", "ironrdp-pdu/qoiz"]
Expand Down Expand Up @@ -66,7 +66,6 @@ ironrdp-rdpeudp = { path = "../ironrdp-rdpeudp", version = "0.1" } # public
ironrdp-rdpeudp-tokio = { path = "../ironrdp-rdpeudp-tokio", version = "0.2" } # public
tracing = { version = "0.1", features = ["log"] }
x509-cert = { version = "0.3", optional = true }
rustls-pemfile = { version = "2.2", optional = true }
rayon = { version = "1.12", optional = true }
bytes = "1"
visibility = { version = "0.1", optional = true }
Expand Down
5 changes: 5 additions & 0 deletions crates/ironrdp-server/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ pub enum MouseButton {
pub trait RdpServerInputHandler: Send {
fn keyboard(&mut self, event: KeyboardEvent);
fn mouse(&mut self, event: MouseEvent);

/// The client opened the advanced input channel (`FreeRDP::Advanced::Input`,
/// [`ironrdp_ainput`]), which FreeRDP-based clients have and the Microsoft
/// clients do not. Its mouse events arrive through [`Self::mouse`].
fn advanced_input_opened(&mut self) {}
}

impl From<(u8, fast_path::KeyboardFlags)> for KeyboardEvent {
Expand Down
11 changes: 5 additions & 6 deletions crates/ironrdp-server/src/helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ use std::io::BufReader;
use std::path::Path;
use std::sync::Arc;

use rustls_pemfile::{certs, pkcs8_private_keys};
use tokio_rustls::rustls::pki_types::pem::PemObject as _;
use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer};
use tokio_rustls::rustls::pki_types::{CertificateDer, PrivateKeyDer, PrivatePkcs8KeyDer};
use tokio_rustls::{TlsAcceptor, rustls};

use crate::error::{ServerError, ServerErrorExt as _, ServerResult};
Expand All @@ -27,23 +26,23 @@ impl TlsIdentityCtx {
.collect::<Result<Vec<_>, _>>()
.map_err(|e| ServerError::custom("collecting server cert", e))?
} else {
certs(&mut BufReader::new(
CertificateDer::pem_reader_iter(BufReader::new(
File::open(cert_path).map_err(|e| ServerError::io("opening server cert", e))?,
))
.collect::<Result<Vec<_>, _>>()
.map_err(|e| ServerError::io("collecting server cert", e))?
.map_err(|e| ServerError::custom("collecting server cert", e))?
};

let priv_key = if key_path.extension().is_some_and(|ext| ext == "pem") {
PrivateKeyDer::from_pem_file(key_path).map_err(|e| ServerError::custom("reading server key", e))?
} else {
pkcs8_private_keys(&mut BufReader::new(
PrivatePkcs8KeyDer::pem_reader_iter(BufReader::new(
File::open(key_path).map_err(|e| ServerError::io("opening server key", e))?,
))
.next()
.ok_or_else(|| ServerError::reason("server key", "no private key"))?
.map(PrivateKeyDer::from)
.map_err(|e| ServerError::io("reading server key", e))?
.map_err(|e| ServerError::custom("reading server key", e))?
};

let pub_key = {
Expand Down
Loading
Loading