From b587b27270cc300d39c496a1ab06be80d72ac107 Mon Sep 17 00:00:00 2001
From: Tony Arcieri <bascule@gmail.com>
Date: Thu, 21 Sep 2023 16:20:57 -0600
Subject: [PATCH] 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          |  2 +-
 aes-gcm/tests/aes128gcm.rs  |  2 +-
 aes-gcm/tests/aes256gcm.rs  |  2 +-
 aes-gcm/tests/common/mod.rs | 21 ++++++++++++++++++++-
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/aes-gcm/src/lib.rs b/aes-gcm/src/lib.rs
index c6a67e5e..4006c368 100644
--- a/aes-gcm/src/lib.rs
+++ b/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)
diff --git a/aes-gcm/tests/aes128gcm.rs b/aes-gcm/tests/aes128gcm.rs
index 7d4e5db0..8834636f 100644
--- a/aes-gcm/tests/aes128gcm.rs
+++ b/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;
 
diff --git a/aes-gcm/tests/aes256gcm.rs b/aes-gcm/tests/aes256gcm.rs
index 82c9f1f6..dd701d5f 100644
--- a/aes-gcm/tests/aes256gcm.rs
+++ b/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;
 
diff --git a/aes-gcm/tests/common/mod.rs b/aes-gcm/tests/common/mod.rs
index d8f2eb51..6255bb3a 100644
--- a/aes-gcm/tests/common/mod.rs
+++ b/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);
         }
     };
 }