fix(credssp): complete the SPNEGO mechListMIC exchange in CredSspServer

.github/workflows/ci.yml

@@ -101,16 +101,18 @@ jobs:
           - manifest: crates/dpapi-native-transport/Cargo.toml
             crate-name: dpapi-native-transport
 
-          # Per-OS feature overrides for specific manifests
+          # Per-OS feature overrides for specific manifests.
+          # `__test-data` enables the `tests/sspi/client_server` suite (gated on
+          # `all(network_client, __test-data)`), which otherwise never runs in CI.
           - os: win
             manifest: Cargo.toml
-            additional-args: --features network_client,dns_resolver,scard,tsssp
+            additional-args: --features network_client,dns_resolver,scard,tsssp,__test-data
           - os: osx
             manifest: Cargo.toml
-            additional-args: --features network_client,scard
+            additional-args: --features network_client,scard,__test-data
           - os: linux
             manifest: Cargo.toml
-            additional-args: --features network_client,dns_resolver,scard
+            additional-args: --features network_client,dns_resolver,scard,__test-data
 
           - os: win
             manifest: ffi/Cargo.toml

src/credssp/mod.rs

@@ -454,6 +454,10 @@ pub struct CredSspServer<C: CredentialsProxy<AuthenticationData = AuthIdentity>>
     credentials_handle: Option<CredentialsBuffers>,
     ts_request_version: u32,
     context_config: Option<ServerMode>,
+    /// Set when the security context completed but the final SPNEGO token (accept-completed +
+    /// `mechListMIC`) still had to be sent to the client: the client cannot send `pubKeyAuth`
+    /// until it has verified that token, so `pubKeyAuth` arrives on the *next* leg.
+    awaiting_pub_key_auth: bool,
 }
 
 impl<C: CredentialsProxy<AuthenticationData = AuthIdentity> + Send> CredSspServer<C> {
@@ -466,6 +470,7 @@ impl<C: CredentialsProxy<AuthenticationData = AuthIdentity> + Send> CredSspServe
             credentials_handle: None,
             ts_request_version: TS_REQUEST_VERSION,
             context_config: Some(client_mode),
+            awaiting_pub_key_auth: false,
         })
     }
 
@@ -483,9 +488,39 @@ impl<C: CredentialsProxy<AuthenticationData = AuthIdentity> + Send> CredSspServe
             credentials_handle: None,
             ts_request_version,
             context_config: Some(client_mode),
+            awaiting_pub_key_auth: false,
         })
     }
 
+    /// Decrypts and verifies the client's `pubKeyAuth`, returning the server-side `pubKeyAuth`
+    /// to send back.
+    fn exchange_pub_key_auth(
+        &mut self,
+        pub_key_auth: Vec<u8>,
+        client_nonce: &Option<[u8; NONCE_SIZE]>,
+    ) -> crate::Result<Vec<u8>> {
+        let peer_version = self
+            .context
+            .as_ref()
+            .unwrap()
+            .peer_version
+            .expect("an decrypt public key server function cannot be fired without any incoming TSRequest");
+        let context = self.context.as_mut().unwrap();
+        context.decrypt_public_key(
+            self.public_key.as_ref(),
+            pub_key_auth.as_ref(),
+            EndpointType::Server,
+            client_nonce,
+            peer_version,
+        )?;
+        context.encrypt_public_key(
+            self.public_key.as_ref(),
+            EndpointType::Server,
+            client_nonce,
+            peer_version,
+        )
+    }
+
     #[instrument(fields(state = ?self.state), skip_all)]
     pub fn process(
         &mut self,
@@ -576,6 +611,32 @@ impl<C: CredentialsProxy<AuthenticationData = AuthIdentity> + Send> CredSspServe
                 Ok(ServerState::Finished(auth_identity))
             }
             CredSspState::NegoToken => {
+                if self.awaiting_pub_key_auth {
+                    // The SPNEGO `mechListMIC` exchange deferred the client's `pubKeyAuth` by one
+                    // leg: the security context is already established and the final SPNEGO token
+                    // has been sent, so this TSRequest carries `pubKeyAuth` alone. Calling the
+                    // acceptor again would be out-of-sequence.
+                    let pub_key_auth = try_cred_ssp_server!(
+                        ts_request.pub_key_auth.take().ok_or_else(|| {
+                            Error::new(
+                                ErrorKind::InvalidToken,
+                                String::from("CredSSP server expected an encrypted public key"),
+                            )
+                        }),
+                        ts_request
+                    );
+                    let client_nonce = ts_request.client_nonce;
+                    let pub_key_auth =
+                        try_cred_ssp_server!(self.exchange_pub_key_auth(pub_key_auth, &client_nonce), ts_request);
+                    ts_request.nego_tokens = None;
+                    ts_request.pub_key_auth = Some(pub_key_auth);
+
+                    self.awaiting_pub_key_auth = false;
+                    self.state = CredSspState::AuthInfo;
+
+                    return Ok(ServerState::ReplyNeeded(ts_request));
+                }
+
                 let input = ts_request.nego_tokens.take().unwrap_or_default();
                 let mut input_token = vec![SecurityBuffer::new(input, BufferType::Token)];
                 let mut output_token = vec![SecurityBuffer::new(Vec::with_capacity(1024), BufferType::Token)];
@@ -628,42 +689,38 @@ impl<C: CredentialsProxy<AuthenticationData = AuthIdentity> + Send> CredSspServe
                             self.context.as_mut().unwrap().sspi_context.complete_auth_token(&mut []),
                             ts_request
                         );
-                        ts_request.nego_tokens = None;
-
-                        let pub_key_auth = try_cred_ssp_server!(
-                            ts_request.pub_key_auth.take().ok_or_else(|| {
-                                Error::new(
-                                    ErrorKind::InvalidToken,
-                                    String::from("CredSSP server expected an encrypted public key"),
-                                )
-                            }),
-                            ts_request
-                        );
-                        let peer_version = self.context.as_ref().unwrap().peer_version.expect(
-                            "an decrypt public key server function cannot be fired without any incoming TSRequest",
-                        );
-                        try_cred_ssp_server!(
-                            self.context.as_mut().unwrap().decrypt_public_key(
-                                self.public_key.as_ref(),
-                                pub_key_auth.as_ref(),
-                                EndpointType::Server,
-                                &ts_request.client_nonce,
-                                peer_version,
-                            ),
-                            ts_request
-                        );
-                        let pub_key_auth = try_cred_ssp_server!(
-                            self.context.as_mut().unwrap().encrypt_public_key(
-                                self.public_key.as_ref(),
-                                EndpointType::Server,
-                                &ts_request.client_nonce,
-                                peer_version,
-                            ),
-                            ts_request
-                        );
-                        ts_request.pub_key_auth = Some(pub_key_auth);
 
-                        self.state = CredSspState::AuthInfo;
+                        let final_token = output_token.remove(0).buffer;
+                        if ts_request.pub_key_auth.is_none() && !final_token.is_empty() {
+                            // SPNEGO `mechListMIC` exchange ([MS-SPNG] 3.3.5.5 / RFC 4178 §5): the
+                            // security context is established, but the client cannot send
+                            // `pubKeyAuth` until it has received and verified our final SPNEGO
+                            // token (accept-completed + `mechListMIC`), so that token must reach
+                            // the wire instead of being dropped. `pubKeyAuth` arrives on the next
+                            // leg.
+                            ts_request.nego_tokens = Some(final_token);
+                            self.awaiting_pub_key_auth = true;
+                        } else {
+                            ts_request.nego_tokens = None;
+
+                            let pub_key_auth = try_cred_ssp_server!(
+                                ts_request.pub_key_auth.take().ok_or_else(|| {
+                                    Error::new(
+                                        ErrorKind::InvalidToken,
+                                        String::from("CredSSP server expected an encrypted public key"),
+                                    )
+                                }),
+                                ts_request
+                            );
+                            let client_nonce = ts_request.client_nonce;
+                            let pub_key_auth = try_cred_ssp_server!(
+                                self.exchange_pub_key_auth(pub_key_auth, &client_nonce),
+                                ts_request
+                            );
+                            ts_request.pub_key_auth = Some(pub_key_auth);
+
+                            self.state = CredSspState::AuthInfo;
+                        }
                     }
                     result => {
                         try_cred_ssp_server!(

tests/sspi/client_server/credssp.rs

@@ -109,6 +109,54 @@ fn credssp_ntlm() {
     run_credssp(&mut client, &mut server, &auth_identity, &mut network_client);
 }
 
+#[test]
+fn credssp_negotiate_ntlm() {
+    // NTLM wrapped in SPNEGO on both sides — the pairing RDP requires (Windows RDP servers
+    // reject bare NTLM inside CredSSP). Because the client always includes an NTLM MIC, the
+    // SPNEGO `mechListMIC` exchange is mandatory ([MS-SPNG]): the client defers `pubKeyAuth`
+    // until it has verified the server's final SPNEGO token, so the handshake takes four
+    // client legs (NEGOTIATE, AUTHENTICATE+mechListMIC, pubKeyAuth, authInfo) instead of
+    // NTLM-only's three. Regression test for #687: the server used to drop its final SPNEGO
+    // token and demand `pubKeyAuth` on the AUTHENTICATE leg, failing the handshake.
+    let auth_identity = AuthIdentity {
+        username: Username::parse("test_user").unwrap(),
+        password: Secret::from("test_password".to_owned()),
+    };
+    let credentials = Credentials::AuthIdentity(auth_identity.clone());
+
+    let mut client = CredSspClient::new(
+        PUBLIC_KEY.to_vec(),
+        credentials.clone(),
+        CredSspMode::WithCredentials,
+        ClientMode::Negotiate(NegotiateConfig::new(
+            Box::new(NtlmConfig {
+                client_computer_name: Some("DESKTOP-3D83IAN.example.com".to_owned()),
+            }),
+            Some("ntlm,!kerberos,!pku2u".to_owned()),
+            "DESKTOP-3D83IAN.example.com".to_owned(),
+        )),
+        TARGET_NAME.to_owned(),
+    )
+    .unwrap();
+
+    let mut server = CredSspServer::new(
+        PUBLIC_KEY.to_vec(),
+        CredentialsProxyImpl::new(&auth_identity),
+        ServerMode::Negotiate(NegotiateConfig::new(
+            Box::new(NtlmConfig {
+                client_computer_name: Some("DESKTOP-3D83IAN.example.com".to_owned()),
+            }),
+            Some("ntlm,!kerberos,!pku2u".to_owned()),
+            "SERVER.example.com".to_owned(),
+        )),
+    )
+    .unwrap();
+
+    let mut network_client = NetworkClientMock { kdc: KdcMock::empty() };
+
+    run_credssp(&mut client, &mut server, &auth_identity, &mut network_client);
+}
+
 #[test]
 fn credssp_kerberos() {
     // CredSSP with Kerberos inside requires SPNEGO. We cannot use Kerberos inside CredSSP without SPNEGO.