aes-gcm: avoid exposing plaintext on tag verification failure (#551)

In #409, for whatever reason I moved the application of the keystream
from after the tag check to before. This means the keystream is applied
unilaterally, instead of only when tag verification is successful.

Sadly, there was a TODO to test for this. A test has been added to
ensure the buffer is unmodified on tag verification failure. It was
red/green tested to ensure it caught the previous bug, and that the fix
corrects it.

This is being tracked as GHSA-423w-p2w9-r7vq (currently embargoed).

aes-gcm/src/lib.rs

@@ -300,10 +300,10 @@ where
         // TODO(tarcieri): interleave encryption with GHASH
         // See: <https://github.com/RustCrypto/AEADs/issues/74>
         let expected_tag = self.compute_tag(mask, associated_data, buffer);
-        ctr.apply_keystream_partial(buffer.into());
 
         use subtle::ConstantTimeEq;
         if expected_tag[..TagSize::to_usize()].ct_eq(tag).into() {
+            ctr.apply_keystream_partial(buffer.into());
             Ok(())
         } else {
             Err(Error)

aes-gcm/tests/aes128gcm.rs

@@ -4,7 +4,7 @@
 mod common;
 
 use self::common::TestVector;
-use aes_gcm::aead::{generic_array::GenericArray, Aead, KeyInit, Payload};
+use aes_gcm::aead::{generic_array::GenericArray, Aead, AeadInPlace, KeyInit, Payload};
 use aes_gcm::Aes128Gcm;
 use hex_literal::hex;
 

aes-gcm/tests/aes256gcm.rs

@@ -4,7 +4,7 @@
 mod common;
 
 use self::common::TestVector;
-use aes_gcm::aead::{generic_array::GenericArray, Aead, KeyInit, Payload};
+use aes_gcm::aead::{generic_array::GenericArray, Aead, AeadInPlace, KeyInit, Payload};
 use aes_gcm::Aes256Gcm;
 use hex_literal::hex;
 

aes-gcm/tests/common/mod.rs

@@ -71,8 +71,27 @@ macro_rules! tests {
 
             let cipher = <$aead>::new(key);
             assert!(cipher.decrypt(nonce, payload).is_err());
+        }
+
+        #[test]
+        fn decrypt_in_place_detached_modified() {
+            let vector = &$vectors.iter().last().unwrap();
+            let key = GenericArray::from_slice(vector.key);
+            let nonce = GenericArray::from_slice(vector.nonce);
+
+            let mut buffer = Vec::from(vector.ciphertext);
+            assert!(!buffer.is_empty());
+
+            // Tweak the first byte
+            let mut tag = GenericArray::clone_from_slice(vector.tag);
+            tag[0] ^= 0xaa;
+
+            let cipher = <$aead>::new(key);
+            assert!(cipher
+                .decrypt_in_place_detached(nonce, &[], &mut buffer, &tag)
+                .is_err());
 
-            // TODO(tarcieri): test ciphertext is unmodified in in-place API
+            assert_eq!(vector.ciphertext, buffer);
         }
     };
 }