Fix decryption after locally initiated key update

When a locally-initiated key update took place, the next packet number
to be sent was incorrectly used as the remote packet number above
which the new key was in use. This caused packets already in flight
from the peer that coincidentally had a higher packet number to be
interpreted as an additional key update, causing decryption to fail.

quinn-proto/src/connection.rs

@@ -539,10 +539,9 @@ where
                     // Might have a key update to discard too
                     self.set_key_discard_timer(now);
                 } else if let Some(ref prev) = self.prev_crypto {
-                    if prev
-                        .update_ack_time
-                        .map_or(false, |x| instant_saturating_sub(now, x) >= self.pto() * 3)
-                    {
+                    if prev.end_packet.map_or(false, |(_, recvd)| {
+                        instant_saturating_sub(now, recvd) >= self.pto() * 3
+                    }) {
                         self.prev_crypto = None;
                     } else {
                         self.set_key_discard_timer(now);
@@ -568,8 +567,8 @@ where
             // gains access to an endpoint's internal state shortly after the handshake completes to
             // decrypt packets sent previously within the same connection.
             now + self.pto() * 3 * 100
-        } else if let Some(time) = self.prev_crypto.as_ref().and_then(|x| x.update_ack_time) {
-            time + self.pto() * 3
+        } else if let Some((_, recvd)) = self.prev_crypto.as_ref().and_then(|x| x.end_packet) {
+            recvd + self.pto() * 3
         } else {
             return;
         };
@@ -2647,17 +2646,30 @@ where
         } else if key_phase == self.key_phase || space != SpaceId::Data {
             &self.spaces[space as usize].crypto.as_mut().unwrap().packet
         } else if let Some(prev) = self.prev_crypto.as_ref().and_then(|crypto| {
-            if number < crypto.end_packet {
+            // If this packet comes prior to acknowledgment of the key update by the peer,
+            if crypto.end_packet.map_or(true, |(pn, _)| number < pn) {
+                // use the previous keys.
                 Some(crypto)
             } else {
+                // Otherwise, this must be a remotely-initiated key update, so fall through to the
+                // final case.
                 None
             }
         }) {
             &prev.crypto
         } else {
+            // We're in the Data space with a key phase mismatch and either there is no locally
+            // initiated key update or the locally initiated key update was acknowledged by a
+            // lower-numbered packet. The key phase mismatch must therefore represent a new
+            // remotely-initiated key update.
             crypto_update = Some(
-                self.tls
-                    .update_keys(&self.spaces[space as usize].crypto.as_ref().unwrap().packet),
+                self.tls.update_keys(
+                    &self.spaces[SpaceId::Data as usize]
+                        .crypto
+                        .as_ref()
+                        .unwrap()
+                        .packet,
+                ),
             );
             crypto_update.as_ref().unwrap()
         };
@@ -2674,9 +2686,9 @@ where
             })?;
 
         if let Some(ref mut prev) = self.prev_crypto {
-            if prev.update_ack_time.is_none() && key_phase == self.key_phase {
+            if prev.end_packet.is_none() && key_phase == self.key_phase {
                 // Key update newly acknowledged
-                prev.update_ack_time = Some(now);
+                prev.end_packet = Some((number, now));
                 self.set_key_discard_timer(now);
             }
         }
@@ -2703,9 +2715,7 @@ where
                 )));
             }
             trace!(self.log, "key update authenticated");
-            self.update_keys(crypto, number, true);
-            // No need to wait for confirmation of a remotely-initiated key update
-            self.prev_crypto.as_mut().unwrap().update_ack_time = Some(now);
+            self.update_keys(crypto, Some((number, now)), true);
             self.set_key_discard_timer(now);
         }
 
@@ -2713,11 +2723,10 @@ where
     }
 
     #[doc(hidden)]
-    pub fn force_key_update(&mut self) {
+    pub fn initiate_key_update(&mut self) {
         let space = self.space(SpaceId::Data);
         let update = self.tls.update_keys(&space.crypto.as_ref().unwrap().packet);
-        let number = space.next_packet_number;
-        self.update_keys(update, number, false);
+        self.update_keys(update, None, false);
     }
 
     /// Send data on the given stream
@@ -2786,7 +2795,7 @@ where
         Ok(n)
     }
 
-    fn update_keys(&mut self, crypto: S::Keys, number: u64, remote: bool) {
+    fn update_keys(&mut self, crypto: S::Keys, end_packet: Option<(u64, Instant)>, remote: bool) {
         let old = mem::replace(
             &mut self.spaces[SpaceId::Data as usize]
                 .crypto
@@ -2797,8 +2806,7 @@ where
         );
         self.prev_crypto = Some(PrevCrypto {
             crypto: old,
-            end_packet: number,
-            update_ack_time: None,
+            end_packet,
             update_unacked: remote,
         });
         self.key_phase = !self.key_phase;
@@ -3170,10 +3178,16 @@ struct PrevCrypto<K>
 where
     K: crypto::Keys,
 {
+    /// The keys used for the previous key phase, temporarily retained to decrypt packets sent by
+    /// the peer prior to its own key update.
     crypto: K,
-    end_packet: u64,
-    /// Time at which a packet using the following key phase was received
-    update_ack_time: Option<Instant>,
+    /// The incoming packet that ends the interval for which these keys are applicable, and the time
+    /// of its receipt.
+    ///
+    /// Incoming packets should be decrypted using these keys iff this is `None` or their packet
+    /// number is lower. `None` indicates that we have not yet received a packet using newer keys,
+    /// which implies that the update was locally initiated.
+    end_packet: Option<(u64, Instant)>,
     /// Whether the following key phase is from a remotely initiated update that we haven't acked
     update_unacked: bool,
 }

quinn-proto/src/tests/mod.rs

@@ -554,7 +554,7 @@ fn key_update() {
         Ok(Some((ref data, 0))) if data == MSG1
     );
 
-    pair.client_conn_mut(client_ch).force_key_update();
+    pair.client_conn_mut(client_ch).initiate_key_update();
 
     const MSG2: &[u8] = b"hello2";
     pair.client_conn_mut(client_ch).write(s, MSG2).unwrap();
@@ -586,7 +586,7 @@ fn key_update_reordered() {
     assert!(!pair.client.outbound.is_empty());
     pair.client.delay_outbound();
 
-    pair.client_conn_mut(client_ch).force_key_update();
+    pair.client_conn_mut(client_ch).initiate_key_update();
     info!(pair.log, "updated keys");
 
     const MSG2: &[u8] = b"two";

quinn/src/connection.rs

@@ -246,7 +246,7 @@ impl Connection {
     // Update traffic keys spontaneously for testing purposes.
     #[doc(hidden)]
     pub fn force_key_update(&self) {
-        self.0.lock().unwrap().inner.force_key_update()
+        self.0.lock().unwrap().inner.initiate_key_update()
     }
 
     /// Replace the diagnostic logger