Stateful nonce desync fix

There was a logic bug where unauthenticated payloads could
still cause a nonce increment in snow. This could in turn allow a
denial-of-service type attack where an attacker could prevent
message delivery by sending garbage data.

This only affects those who are using TransportState, not those
using StatelessTransportState.

Author: mcginty

src/cipherstate.rs

@@ -59,12 +59,12 @@ impl CipherState {
         }
 
         validate_nonce(self.n)?;
-        let len = self.cipher.decrypt(self.n, authtext, ciphertext, out);
+        let len = self.cipher.decrypt(self.n, authtext, ciphertext, out)?;
 
         // We have validated this will not wrap around.
         self.n += 1;
 
-        len
+        Ok(len)
     }
 
     pub fn encrypt(&mut self, plaintext: &[u8], out: &mut [u8]) -> Result<usize, Error> {

src/resolvers/default.rs

@@ -1,10 +1,7 @@
 use blake2::{Blake2b, Blake2b512, Blake2s, Blake2s256};
 #[cfg(feature = "xchachapoly")]
 use chacha20poly1305::XChaCha20Poly1305;
-use chacha20poly1305::{
-    aead::AeadInPlace,
-    KeyInit, ChaCha20Poly1305,
-};
+use chacha20poly1305::{aead::AeadInPlace, ChaCha20Poly1305, KeyInit};
 use curve25519_dalek::montgomery::MontgomeryPoint;
 #[cfg(feature = "pqclean_kyber1024")]
 use pqcrypto_kyber::kyber1024;
@@ -611,7 +608,7 @@ mod tests {
         keypair.dh(&public, &mut output).unwrap();
         assert_eq!(
             hex::encode(output),
-                "c3da55379de9c6908e94ea4df28d084f32eccf03491c71f754b4075577a28552"
+            "c3da55379de9c6908e94ea4df28d084f32eccf03491c71f754b4075577a28552"
         );
     }
 

tests/general.rs

@@ -417,6 +417,7 @@ fn test_rekey() {
     h_i.rekey_outgoing();
     let len = h_i.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     assert!(h_r.read_message(&buffer_msg[..len], &mut buffer_out).is_err());
+    h_r.set_receiving_nonce(h_i.sending_nonce());
 
     // rekey incoming on responder
     h_r.rekey_incoming();
@@ -428,6 +429,7 @@ fn test_rekey() {
     h_r.rekey_outgoing();
     let len = h_r.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     assert!(h_i.read_message(&buffer_msg[..len], &mut buffer_out).is_err());
+    h_i.set_receiving_nonce(h_r.sending_nonce());
 
     // rekey incoming on initiator
     h_i.rekey_incoming();
@@ -444,37 +446,45 @@ fn test_rekey_manually() {
 
     let mut buffer_msg = [0u8; 200];
     let mut buffer_out = [0u8; 200];
+
+    // Do a handshake, and transition to stateful transport mode.
     let len = h_i.write_message(b"abc", &mut buffer_msg).unwrap();
     h_r.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
-
     let len = h_r.write_message(b"defg", &mut buffer_msg).unwrap();
     h_i.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
-
     let mut h_i = h_i.into_transport_mode().unwrap();
     let mut h_r = h_r.into_transport_mode().unwrap();
 
-    // test message initiator->responder before rekeying initiator
+    // test sanity message initiator->responder before rekeying initiator
     let len = h_i.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     let len = h_r.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
     assert_eq!(&buffer_out[..len], b"hack the planet");
 
-    // rekey initiator (on initiator)
+    // rekey initiator-side initiator key to K1 without rekeying the responder,
+    // expecting a decryption failure.
+    //
+    // The message *should* have failed to read, so we also force nonce re-sync.
     h_i.rekey_manually(Some(&[1u8; 32]), None);
     let len = h_i.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     assert!(h_r.read_message(&buffer_msg[..len], &mut buffer_out).is_err());
+    h_r.set_receiving_nonce(h_i.sending_nonce());
 
-    // rekey initiator (on responder)
+    // rekey responder-side responder key to K1, expecting a successful decryption.
     h_r.rekey_manually(Some(&[1u8; 32]), None);
     let len = h_i.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     let len = h_r.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
     assert_eq!(&buffer_out[..len], b"hack the planet");
 
-    // rekey responder (on responder)
+    // rekey responder-side responder key to K1 without rekeying the initiator,
+    // expecting a decryption failure.
+    //
+    // The message *should* have failed to read, so we also force nonce re-sync.
     h_r.rekey_manually(None, Some(&[1u8; 32]));
     let len = h_r.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     assert!(h_i.read_message(&buffer_msg[..len], &mut buffer_out).is_err());
+    h_i.set_receiving_nonce(h_r.sending_nonce());
 
-    // rekey responder (on initiator)
+    // rekey intiator-side responder key to K1, expecting a successful decryption.
     h_i.rekey_manually(None, Some(&[1u8; 32]));
     let len = h_r.write_message(b"hack the planet", &mut buffer_msg).unwrap();
     let len = h_i.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
@@ -870,3 +880,36 @@ fn test_stateless_nonce_maximum_behavior() {
         Err(snow::Error::State(snow::error::StateProblem::Exhausted))
     ));
 }
+
+#[test]
+fn test_stateful_nonce_increment_behavior() {
+    let params: NoiseParams = "Noise_NN_25519_ChaChaPoly_SHA256".parse().unwrap();
+    let mut h_i = Builder::new(params.clone()).build_initiator().unwrap();
+    let mut h_r = Builder::new(params).build_responder().unwrap();
+
+    let mut buffer_msg = [0u8; 200];
+    let mut buffer_out = [0u8; 200];
+    let len = h_i.write_message(b"abc", &mut buffer_msg).unwrap();
+    h_r.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
+
+    let len = h_r.write_message(b"defg", &mut buffer_msg).unwrap();
+    h_i.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
+
+    let mut h_i = h_i.into_transport_mode().unwrap();
+    let mut h_r = h_r.into_transport_mode().unwrap();
+
+    let len = h_i.write_message(b"xyz", &mut buffer_msg).unwrap();
+
+    // Corrupt the message by incrementing a byte in the payload
+    let mut corrupted = buffer_msg[..len].to_owned();
+    corrupted[0] = corrupted[0].wrapping_add(1);
+
+    // This should result in an error, but should not change any internal state
+    assert!(h_r.read_message(&corrupted, &mut buffer_out).is_err());
+
+    // This should now succeed as the nonce counter should have remained the same
+    h_r.read_message(&buffer_msg[..len], &mut buffer_out).unwrap();
+
+    // This should now fail again as the nonce counter should have incremented
+    assert!(h_r.read_message(&buffer_msg[..len], &mut buffer_out).is_err());
+}