Repository navigation
feat(server): several credentials, client alternate shell and RDPDR drive redirection #2357
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 8 commits
6c1fbc9
403ddde
03deb38
9202f91
2ae0442
373b807
0873ecd
4b788d1
58492e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -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, | ||
| } | ||
|
|
||
| impl Acceptor { | ||
|
|
@@ -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, | ||
|
|
@@ -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; | ||
|
|
@@ -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, | ||
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
@@ -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
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] 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")); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -545,7 +545,10 @@ impl RdpdrServer { | |
| "received ClientCoreCapabilityResponse" | ||
| ); | ||
| self.state = RdpdrServerState::Active; | ||
| Ok(Vec::new()) | ||
| // [MS-RDPEFS] 3.1.5.1.5: clients announce their drives only after the | ||
| // Server User Logged On PDU. The user was authenticated when the | ||
| // connection was, so it follows the capability exchange at once. | ||
| Ok(vec![SvcMessage::from(RdpdrPdu::UserLoggedon)]) | ||
|
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] Server User Logged On sent without checking the client's RDPDR_USER_LOGGEDON_PDU capability bit — medium 🟠 — MS-RDPEFS 3.3.5.1.5 requires sending the Server User Logged On PDU only when the client's general capability set advertises RDPDR_USER_LOGGEDON_PDU (0x00000004) in extendedPDU (else 3.3.5.1.7 re-sends Client ID Confirm on logon). The new code returns RdpdrPdu::UserLoggedon unconditionally after every ClientCoreCapabilityResponse, although response.capabilities — including the already-modeled ExtendedPdu::RDPDR_USER_LOGGEDON_PDU flag (efs.rs 987) — is in scope for the guard. Non-advertising clients receive an unsolicited PDU, and the comment cites 3.1.5.1.5 (client side) rather than the governing server-side 3.3.5.1.5. |
||
| } | ||
|
|
||
| fn handle_device_list_announce(&mut self, src: &mut ReadCursor<'_>) -> PduResult<Vec<SvcMessage>> { | ||
|
|
||
There was a problem hiding this comment.
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.