From ebff930726273a2b72f7eb5195379165c6da71fa Mon Sep 17 00:00:00 2001
From: Alessandro Ghedini <alessandro@ghedini.me>
Date: Thu, 23 Jan 2025 10:51:43 +0000
Subject: [PATCH] crypto/boringssl: do dummy seal on key update too

Follow-up to b17904e522777746b2a6b12667d85283e1df368c.

Currently the dummy seal operation done to prime the AEAD context is
done in `Seal::from_secret()`, but really it needs to be done as part of
`PacketKey::from_secret()` to make sure every code path is covered,
including the key update one which doesn't call `Seal::from_secret()`.

It's not entirely clear why, but the problem (i.e. seal operation fail
when trying to encrypt a packet) seems to only appear after several
packets have already been encrypted with the new key, which is why this
wasn't caught by the existing key update test.

To cover that, the test is expanded by making the server send additional
packets which from local testing seems to trigger the issue.
---
 quiche/src/crypto/boringssl.rs | 12 +++++++++++-
 quiche/src/crypto/mod.rs       | 14 ++------------
 quiche/src/lib.rs              | 16 ++++++++++++++--
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/quiche/src/crypto/boringssl.rs b/quiche/src/crypto/boringssl.rs
index c6264d5ec7..8eaed89d8a 100644
--- a/quiche/src/crypto/boringssl.rs
+++ b/quiche/src/crypto/boringssl.rs
@@ -65,7 +65,17 @@ impl PacketKey {
         derive_pkt_key(aead, secret, &mut key)?;
         derive_pkt_iv(aead, secret, &mut iv)?;
 
-        Self::new(aead, key, iv, enc)
+        let pkt_key = Self::new(aead, key, iv, enc)?;
+
+        // Dummy seal operation to prime the AEAD context with the nonce mask.
+        //
+        // This is needed because BoringCrypto requires the first counter (i.e.
+        // packet number) to be zero, which would not be the case for packet
+        // number spaces after Initial as the same packet number sequence is
+        // shared.
+        let _ = pkt_key.seal_with_u64_counter(0, b"", &mut [0_u8; 16], 0, None);
+
+        Ok(pkt_key)
     }
 
     pub fn open_with_u64_counter(
diff --git a/quiche/src/crypto/mod.rs b/quiche/src/crypto/mod.rs
index a06eb37fb9..367215ae60 100644
--- a/quiche/src/crypto/mod.rs
+++ b/quiche/src/crypto/mod.rs
@@ -235,7 +235,7 @@ impl Seal {
     }
 
     pub fn from_secret(aead: Algorithm, secret: &[u8]) -> Result<Seal> {
-        let seal = Seal {
+        Ok(Seal {
             alg: aead,
 
             secret: secret.to_vec(),
@@ -243,17 +243,7 @@ impl Seal {
             header: HeaderProtectionKey::from_secret(aead, secret)?,
 
             packet: PacketKey::from_secret(aead, secret, Self::ENCRYPT)?,
-        };
-
-        // Dummy seal operation to prime the AEAD context with the nonce mask.
-        //
-        // This is needed because BoringCrypto requires the first counter (i.e.
-        // packet number) to be zero, which would not be the case for packet
-        // number spaces after Initial as the same packet number sequence is
-        // shared.
-        let _ = seal.seal_with_u64_counter(0, b"", &mut [0_u8; 16], 0, None);
-
-        Ok(seal)
+        })
     }
 
     pub fn new_mask(&self, sample: &[u8]) -> Result<[u8; 5]> {
diff --git a/quiche/src/lib.rs b/quiche/src/lib.rs
index 6bb7790017..83d1651ad1 100644
--- a/quiche/src/lib.rs
+++ b/quiche/src/lib.rs
@@ -10246,15 +10246,27 @@ mod tests {
         );
 
         // Server sends message with the new key.
-        assert_eq!(pipe.server.stream_send(4, b"world", true), Ok(5));
+        assert_eq!(pipe.server.stream_send(4, b"world", false), Ok(5));
         assert_eq!(pipe.advance(), Ok(()));
 
         // Ensure update key is completed and client can decrypt packet.
         let mut r = pipe.client.readable();
         assert_eq!(r.next(), Some(4));
         assert_eq!(r.next(), None);
-        assert_eq!(pipe.client.stream_recv(4, &mut b), Ok((5, true)));
+        assert_eq!(pipe.client.stream_recv(4, &mut b), Ok((5, false)));
         assert_eq!(&b[..5], b"world");
+
+        // Server keeps sending packets to ensure encryption still works.
+        for _ in 0..10 {
+            assert_eq!(pipe.server.stream_send(4, b"world", false), Ok(5));
+            assert_eq!(pipe.advance(), Ok(()));
+
+            let mut r = pipe.client.readable();
+            assert_eq!(r.next(), Some(4));
+            assert_eq!(r.next(), None);
+            assert_eq!(pipe.client.stream_recv(4, &mut b), Ok((5, false)));
+            assert_eq!(&b[..5], b"world");
+        }
     }
 
     #[test]