fix(ntlm): preserve RC4 sealing state across mechListMIC (#753)

CredSSP can wrap `pubKeyAuth` after the initiator sends its mechListMIC
but before it verifies the acceptor's MIC. Resetting both NTLM RC4
handles during verification loses the advanced send state and breaks the
next wrapped message.

Snapshot and restore only the sealing handle used for each MIC,
including when verification fails. Remove the obsolete reset helper and
add regression coverage for both directions, the CredSSP ordering, and
invalid signatures. Sequence numbers remain unchanged.

Tests: `cargo test -p sspi --lib ntlm::test:: --quiet` (36 passed);
`cargo test -p sspi --features network_client,__test-data --test sspi
ntlm --quiet` (14 passed); `cargo check -p sspi --quiet`; `cargo fmt
--all --check`.

Fixes: #752

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: mamoreau-devolutions

src/ntlm/mod.rs

@@ -99,8 +99,6 @@ pub struct Ntlm {
     // If the NTLM is used as server, then our_seq_number is the server sequence number and remote seq_number is the client sequence number.
     our_seq_number: u32,
     remote_seq_number: u32,
-    // This flag is needed to correctly reset cipher state after MIC token generation/verification.
-    is_client: bool,
 
     session_key: Option<[u8; SESSION_KEY_SIZE]>,
 }
@@ -162,7 +160,6 @@ impl Ntlm {
 
             our_seq_number: 0,
             remote_seq_number: 0,
-            is_client: true,
         }
     }
 
@@ -194,7 +191,6 @@ impl Ntlm {
 
             our_seq_number: 0,
             remote_seq_number: 0,
-            is_client: true,
         }
     }
 
@@ -226,7 +222,6 @@ impl Ntlm {
 
             our_seq_number: 0,
             remote_seq_number: 0,
-            is_client: true,
         }
     }
 
@@ -252,47 +247,6 @@ impl Ntlm {
         });
     }
 
-    /// Resets the cipher state.
-    ///
-    /// According to the specification, we need to reset ciphers before and after MIC token generation/verification.
-    /// [3.2.5.1 NTLM RC4 Key State for MechListMIC and First Signed Message](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-spng/f38ae8e3-847d-4829-b933-5ac1911a00ba):
-    /// > When NTLM is negotiated, the SPNG server MUST set OriginalHandle to ServerHandle before generating the mechListMIC,
-    /// > then set ServerHandle to OriginalHandle after generating the mechListMIC. This results in the RC4 key state
-    /// > being the same for the mechListMIC and for the first message signed by the application.
-    /// >
-    /// > Because the RC4 key state is the same for the mechListMIC and for the first message signed by the application,
-    /// > the SPNEGO Extension server MUST set OriginalHandle to ClientHandle before validating the mechListMIC and then
-    /// > set ClientHandle to OriginalHandle after validating the mechListMIC.
-    fn reset_cipher_state(&mut self) -> crate::Result<()> {
-        use crate::ntlm::messages::computations::generate_signing_key;
-        use crate::ntlm::messages::{CLIENT_SEAL_MAGIC, CLIENT_SIGN_MAGIC, SERVER_SEAL_MAGIC, SERVER_SIGN_MAGIC};
-
-        let session_key = self.session_key.as_ref().ok_or_else(|| {
-            Error::new(
-                ErrorKind::OutOfSequence,
-                "the session key is not established, cannot reset cipher state",
-            )
-        })?;
-
-        if self.is_client {
-            self.send_signing_key = generate_signing_key(session_key.as_ref(), CLIENT_SIGN_MAGIC);
-            self.recv_signing_key = generate_signing_key(session_key.as_ref(), SERVER_SIGN_MAGIC);
-            self.send_sealing_key = Some(Rc4::new(
-                generate_signing_key(session_key.as_ref(), CLIENT_SEAL_MAGIC).as_ref(),
-            ));
-            self.recv_sealing_key = Some(Rc4::new(
-                generate_signing_key(session_key.as_ref(), SERVER_SEAL_MAGIC).as_ref(),
-            ));
-        } else {
-            self.send_signing_key = generate_signing_key(session_key, SERVER_SIGN_MAGIC);
-            self.recv_signing_key = generate_signing_key(session_key, CLIENT_SIGN_MAGIC);
-            self.send_sealing_key = Some(Rc4::new(generate_signing_key(session_key, SERVER_SEAL_MAGIC).as_ref()));
-            self.recv_sealing_key = Some(Rc4::new(generate_signing_key(session_key, CLIENT_SEAL_MAGIC).as_ref()));
-        }
-
-        Ok(())
-    }
-
     /// Returns the next sequence number for outgoing messages and increments the internal counter.
     fn our_seq_num(&mut self) -> u32 {
         let seq_num = self.our_seq_number;
@@ -370,8 +324,6 @@ impl Ntlm {
         &mut self,
         builder: FilledAcceptSecurityContext<'_, <Self as SspiImpl>::CredentialsHandle>,
     ) -> crate::Result<AcceptSecurityContextResult> {
-        self.is_client = false;
-
         let input = builder
             .input
             .ok_or_else(|| Error::new(ErrorKind::InvalidToken, "Input buffers must be specified"))?;
@@ -417,8 +369,6 @@ impl Ntlm {
         &mut self,
         builder: &mut FilledInitializeSecurityContext<'_, '_, <Self as SspiImpl>::CredentialsHandle>,
     ) -> crate::Result<InitializeSecurityContextResult> {
-        self.is_client = true;
-
         trace!(?builder);
 
         let status = match self.state {
@@ -803,49 +753,24 @@ impl SspiEx for Ntlm {
     }
 
     fn verify_mic_token(&mut self, signature: &[u8], data: &[u8], _: crate::private::Sealed) -> crate::Result<()> {
-        // We reset the cipher state before and after MIC token verification.
-        //
-        // [3.2.5.1 NTLM RC4 Key State for MechListMIC and First Signed Message](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-spng/f38ae8e3-847d-4829-b933-5ac1911a00ba):
-        // > When NTLM is negotiated, the SPNG server MUST set OriginalHandle to ServerHandle before generating the mechListMIC,
-        // > then set ServerHandle to OriginalHandle after generating the mechListMIC. This results in the RC4 key state
-        // > being the same for the mechListMIC and for the first message signed by the application.
-        // >
-        // > Because the RC4 key state is the same for the mechListMIC and for the first message signed by the application,
-        // > the SPNEGO Extension server MUST set OriginalHandle to ClientHandle before validating the mechListMIC and then
-        // > set ClientHandle to OriginalHandle after validating the mechListMIC.
-
         if self.recv_sealing_key.is_none() {
             self.complete_auth_token(&mut [])?;
-        } else {
-            self.reset_cipher_state()?;
         }
 
         let seq_number = self.remote_seq_num();
 
         let digest = compute_digest(self.recv_signing_key.as_ref(), seq_number, data)?;
-        self.check_signature(seq_number, &digest, signature)?;
-
-        self.reset_cipher_state()?;
 
-        Ok(())
+        // MS-SPNG 3.2.5.1 / 3.3.5.1: the MIC must not advance the application's RC4 handle.
+        let original_recv_sealing_key = self.recv_sealing_key.clone();
+        let result = self.check_signature(seq_number, &digest, signature);
+        self.recv_sealing_key = original_recv_sealing_key;
+        result
     }
 
     fn generate_mic_token(&mut self, data: &[u8], _: crate::private::Sealed) -> crate::Result<Vec<u8>> {
-        // We reset the cipher state before and after MIC token generation.
-        //
-        // [3.2.5.1 NTLM RC4 Key State for MechListMIC and First Signed Message](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-spng/f38ae8e3-847d-4829-b933-5ac1911a00ba):
-        // > When NTLM is negotiated, the SPNG server MUST set OriginalHandle to ServerHandle before generating the mechListMIC,
-        // > then set ServerHandle to OriginalHandle after generating the mechListMIC. This results in the RC4 key state
-        // > being the same for the mechListMIC and for the first message signed by the application.
-        // >
-        // > Because the RC4 key state is the same for the mechListMIC and for the first message signed by the application,
-        // > the SPNEGO Extension server MUST set OriginalHandle to ClientHandle before validating the mechListMIC and then
-        // > set ClientHandle to OriginalHandle after validating the mechListMIC.
-
         if self.send_sealing_key.is_none() {
             self.complete_auth_token(&mut [])?;
-        } else {
-            self.reset_cipher_state()?;
         }
 
         let seq_number = self.our_seq_num();
@@ -854,9 +779,11 @@ impl SspiEx for Ntlm {
 
         let mut mic_token = vec![0; SIGNATURE_SIZE];
         let mut message = [SecurityBufferRef::token_buf(&mut mic_token)];
-        self.compute_checksum(&mut message, seq_number, &digest)?;
-
-        self.reset_cipher_state()?;
+        // MS-SPNG 3.2.5.1 / 3.3.5.1: restore only the handle used to sign the MIC.
+        let original_send_sealing_key = self.send_sealing_key.clone();
+        let result = self.compute_checksum(&mut message, seq_number, &digest);
+        self.send_sealing_key = original_send_sealing_key;
+        result?;
 
         Ok(mic_token)
     }

src/ntlm/test.rs

@@ -1,5 +1,7 @@
 use crate::crypto::{HASH_SIZE, Rc4};
+use crate::ntlm::messages::computations::generate_signing_key;
 use crate::ntlm::messages::test::TEST_CREDENTIALS;
+use crate::ntlm::messages::{CLIENT_SEAL_MAGIC, CLIENT_SIGN_MAGIC, SERVER_SEAL_MAGIC, SERVER_SIGN_MAGIC};
 use crate::ntlm::{
     AuthenticateMessage, CHALLENGE_SIZE, ChallengeMessage, Mic, NegotiateFlags, NegotiateMessage, Ntlm, NtlmState,
     SIGNATURE_SIZE,
@@ -331,6 +333,152 @@ fn verify_signature_fails_on_invalid_signature() {
     );
 }
 
+fn mic_test_pair() -> (Ntlm, Ntlm) {
+    let mut client = Ntlm::new();
+    let mut server = Ntlm::new();
+    client.flags = NegotiateFlags::NTLM_SSP_NEGOTIATE_KEY_EXCH;
+    server.flags = NegotiateFlags::NTLM_SSP_NEGOTIATE_KEY_EXCH;
+    client.session_key = Some(SEALING_KEY);
+    server.session_key = Some(SEALING_KEY);
+
+    client.send_signing_key = generate_signing_key(&SEALING_KEY, CLIENT_SIGN_MAGIC);
+    server.recv_signing_key = client.send_signing_key.clone();
+    server.send_signing_key = generate_signing_key(&SEALING_KEY, SERVER_SIGN_MAGIC);
+    client.recv_signing_key = server.send_signing_key.clone();
+
+    client.send_sealing_key = Some(Rc4::new(generate_signing_key(&SEALING_KEY, CLIENT_SEAL_MAGIC).as_ref()));
+    server.recv_sealing_key = client.send_sealing_key.clone();
+    server.send_sealing_key = Some(Rc4::new(generate_signing_key(&SEALING_KEY, SERVER_SEAL_MAGIC).as_ref()));
+    client.recv_sealing_key = server.send_sealing_key.clone();
+
+    (client, server)
+}
+
+fn assert_wrapped_message(sender: &mut Ntlm, receiver: &mut Ntlm, plaintext: &[u8]) {
+    let mut token = [0; SIGNATURE_SIZE];
+    let mut data = plaintext.to_vec();
+    {
+        let mut buffers = [
+            SecurityBufferRef::token_buf(&mut token),
+            SecurityBufferRef::data_buf(&mut data),
+        ];
+        sender.encrypt_message(EncryptionFlags::empty(), &mut buffers).unwrap();
+        assert_ne!(buffers[1].data(), plaintext);
+    }
+
+    let mut buffers = [
+        SecurityBufferRef::data_buf(&mut data),
+        SecurityBufferRef::token_buf(&mut token),
+    ];
+    receiver.decrypt_message(&mut buffers).unwrap();
+    assert_eq!(buffers[0].data(), plaintext);
+}
+
+#[test]
+fn generate_mic_token_preserves_both_sealing_handles() {
+    let (client, server) = mic_test_pair();
+    for mut context in [client, server] {
+        let _ = context
+            .send_sealing_key
+            .as_mut()
+            .unwrap()
+            .process(b"earlier outgoing data");
+        let _ = context
+            .recv_sealing_key
+            .as_mut()
+            .unwrap()
+            .process(b"earlier incoming data");
+        let mut send_before = context.send_sealing_key.clone().unwrap();
+        let mut recv_before = context.recv_sealing_key.clone().unwrap();
+
+        let mic = context.generate_mic_token(b"mech types", private::Sealed).unwrap();
+
+        assert_eq!(mic.len(), SIGNATURE_SIZE);
+        assert_eq!(
+            context.send_sealing_key.as_mut().unwrap().process(TEST_DATA),
+            send_before.process(TEST_DATA)
+        );
+        assert_eq!(
+            context.recv_sealing_key.as_mut().unwrap().process(TEST_DATA),
+            recv_before.process(TEST_DATA)
+        );
+    }
+}
+
+#[test]
+fn verify_mic_token_after_pub_key_auth_preserves_send_state() {
+    let (mut client, mut server) = mic_test_pair();
+    let mech_types = b"mech types";
+    let server_mic = server.generate_mic_token(mech_types, private::Sealed).unwrap();
+
+    assert_wrapped_message(&mut client, &mut server, b"client pubKeyAuth");
+    let mut reply_token = [0; SIGNATURE_SIZE];
+    let mut reply_data = b"server pubKeyAuth".to_vec();
+    server
+        .encrypt_message(
+            EncryptionFlags::empty(),
+            &mut [
+                SecurityBufferRef::token_buf(&mut reply_token),
+                SecurityBufferRef::data_buf(&mut reply_data),
+            ],
+        )
+        .unwrap();
+
+    client
+        .verify_mic_token(&server_mic, mech_types, private::Sealed)
+        .unwrap();
+    client
+        .decrypt_message(&mut [
+            SecurityBufferRef::data_buf(&mut reply_data),
+            SecurityBufferRef::token_buf(&mut reply_token),
+        ])
+        .unwrap();
+    assert_eq!(reply_data, b"server pubKeyAuth");
+
+    assert_wrapped_message(&mut client, &mut server, b"following client payload");
+}
+
+#[test]
+fn verify_mic_token_preserves_advanced_receive_state() {
+    let (mut client, mut server) = mic_test_pair();
+    assert_wrapped_message(&mut server, &mut client, b"earlier server payload");
+
+    let mech_types = b"mech types";
+    let server_mic = server.generate_mic_token(mech_types, private::Sealed).unwrap();
+    client
+        .verify_mic_token(&server_mic, mech_types, private::Sealed)
+        .unwrap();
+
+    assert_wrapped_message(&mut server, &mut client, b"following server payload");
+}
+
+#[test]
+fn verify_mic_token_restores_receive_state_on_invalid_signature() {
+    let (mut client, mut server) = mic_test_pair();
+    assert_wrapped_message(&mut server, &mut client, b"earlier server payload");
+
+    let mut mic = server.generate_mic_token(b"mech types", private::Sealed).unwrap();
+    mic[4] ^= 0xff;
+    let mut recv_before = client.recv_sealing_key.clone().unwrap();
+    let mut send_before = client.send_sealing_key.clone().unwrap();
+
+    assert_eq!(
+        client
+            .verify_mic_token(&mic, b"mech types", private::Sealed)
+            .unwrap_err()
+            .error_type,
+        ErrorKind::MessageAltered
+    );
+    assert_eq!(
+        client.recv_sealing_key.as_mut().unwrap().process(TEST_DATA),
+        recv_before.process(TEST_DATA)
+    );
+    assert_eq!(
+        client.send_sealing_key.as_mut().unwrap().process(TEST_DATA),
+        send_before.process(TEST_DATA)
+    );
+}
+
 #[test]
 fn initialize_security_context_wrong_state_negotiate() {
     let mut context = Ntlm::new();