Derive the retained handshake epoch and document the DTLS 1.3 epoch retention, relates to github #1468.

Author: mondain

tls/src/main/java/org/bouncycastle/tls/DTLSRecordLayer.java

@@ -141,6 +141,22 @@ private static void sendDatagram(DatagramSender sender, byte[] buf, int off, int
      * plaintext at epoch 0 while the rest of the same flight is protected at the handshake epoch - so
      * answering a retransmission of it means being able to read the plaintext epoch too. Handshake records
      * only, exactly as for retransmitEpoch.
+     *
+     * Epoch 0 is unauthenticated, so anyone able to put a datagram on the path can forge a record at it, and
+     * retaining it for reading would otherwise be a way to draw an answer out of a completed client. It is
+     * harmless because of three separate gates, all of which have to hold for it to stay harmless:
+     *
+     * 1. DTLSReliableHandshake.processRecord's DTLS 1.3 'expectedEpoch' check (RFC 9147 6.1). At epoch 0
+     *    only a client_hello or server_hello msg_type is accepted, so a forgery cannot stand in for anything
+     *    later in the flight; on the client, which is the only side that retains epoch 0, that leaves
+     *    server_hello fragments.
+     * 2. DTLSReliableHandshake.processRecord only answers a retransmission once
+     *    checkAll(previousInboundFlight) finds that flight complete, and the rest of that flight is
+     *    protected at the handshake epoch, which cannot be forged. A plaintext-only forgery therefore never
+     *    completes a flight and never draws a retransmission.
+     * 3. Post-handshake, processRecord is called with a 'windowSize' of 0 (see the retransmit callback in
+     *    DTLSReliableHandshake.finish), so every message_seq is 'too far ahead' and no DTLSReassembler is
+     *    ever allocated: the forgery cannot make us buffer anything either.
      */
     private DTLSEpoch retransmitEpochPlaintext = null;
     private Timeout retransmitTimeout = null;
@@ -345,12 +361,37 @@ private void commitPendingEpochIfCurrent()
     {
         if (readEpoch == pendingEpoch && writeEpoch == pendingEpoch)
         {
-            this.retiredEpoch = currentEpoch;
+            /*
+             * DTLS 1.2 never reads 'retiredEpoch': handshakeSuccessful retains 'currentEpoch' there, because
+             * in DTLS 1.2 the epoch being superseded is still the current one at that point. Assigning it
+             * only for DTLS 1.3 keeps the DTLS 1.2 path's state untouched by this field.
+             *
+             * TODO[dtls13] This holds only the MOST RECENT retired epoch, which is all RFC 9147 5.8.1 needs
+             * while the only epoch change after the handshake epoch is the one to the application epoch. Once
+             * post-handshake key update (RFC 9147 8) lands, several epochs can be retired in succession and
+             * which of them is still readable becomes load-bearing.
+             */
+            if (dtls13)
+            {
+                this.retiredEpoch = currentEpoch;
+            }
+
             this.currentEpoch = pendingEpoch;
             this.pendingEpoch = null;
         }
     }
 
+    /**
+     * RFC 9147 5.8.1. DTLS 1.3 only: the epoch that {@link #handshakeSuccessful(DTLSHandshakeRetransmit)} is
+     * about to retain for reading, i.e. the handshake epoch the peer's final flight was protected under, or
+     * -1 if no epoch has been retired. Read by {@link DTLSReliableHandshake#finish()}, so that the epoch a
+     * retransmission of that flight has to arrive at is taken from the record layer rather than assumed.
+     */
+    int getRetiredEpoch()
+    {
+        return (null != retiredEpoch) ? retiredEpoch.getEpoch() : -1;
+    }
+
     void handshakeSuccessful(DTLSHandshakeRetransmit retransmit)
     {
         if (!dtls13 && (readEpoch == currentEpoch || writeEpoch == currentEpoch))

tls/src/main/java/org/bouncycastle/tls/DTLSReliableHandshake.java

@@ -408,6 +408,13 @@ void finish()
                 final Vector finalFlightAck = ackRecordNumbers;
                 final Hashtable finalFlight = summarizeFlight(currentInboundFlight);
 
+                /*
+                 * RFC 9147 5.8.1. The epoch that flight was protected under, taken from the record layer -
+                 * which is about to retain exactly that epoch for reading - rather than assumed to be the
+                 * handshake epoch's current number.
+                 */
+                final int finalFlightEpoch = recordLayer.getRetiredEpoch();
+
                 sendPendingAck();
 
                 /*
@@ -426,7 +433,7 @@ void finish()
                     public void receivedHandshakeRecord(int epoch, byte[] buf, int off, int len)
                         throws IOException
                     {
-                        if (matchesFlight(finalFlight, epoch, buf, off, len))
+                        if (matchesFlight(finalFlight, finalFlightEpoch, epoch, buf, off, len))
                         {
                             sendAck(finalFlightAck);
                         }
@@ -508,13 +515,15 @@ static Hashtable summarizeFlight(Hashtable inboundFlight)
      * that retains epoch 0 for reading.
      * </p>
      */
-    static boolean matchesFlight(Hashtable flight, int epoch, byte[] buf, int off, int len)
+    static boolean matchesFlight(Hashtable flight, int flightEpoch, int epoch, byte[] buf, int off, int len)
     {
         /*
-         * RFC 9147 6.1. The flight this answers is the peer's final flight, every message of which is
-         * protected under the handshake traffic keys at epoch 2.
+         * RFC 9147 6.1. The flight this answers is the peer's final flight, and 'flightEpoch' is the epoch it
+         * was protected under - the handshake epoch, which the record layer has retained for reading. A
+         * record at any other epoch carries none of it, the unauthenticated epoch 0 included; a flightEpoch
+         * of -1 means no epoch was retired, so there is nothing to recognise.
          */
-        if (2 != epoch || len < MESSAGE_HEADER_LENGTH)
+        if (flightEpoch < 0 || flightEpoch != epoch || len < MESSAGE_HEADER_LENGTH)
         {
             return false;
         }

tls/src/main/java/org/bouncycastle/tls/DTLSServerProtocol.java

@@ -570,7 +570,7 @@ protected byte[] send13ServerHelloCoda(ServerHandshakeState state, DTLSReliableH
         recordLayer.enablePendingEpochRead();
 
         handshake.sendMessage(HandshakeType.encrypted_extensions,
-            generate13EncryptedExtensions(state.serverExtensions));
+            generate13EncryptedExtensions(state, state.serverExtensions));
 
         if (state.selectedPSK13)
         {
@@ -1030,7 +1030,7 @@ protected void processClientHelloRetry(ServerHandshakeState state, byte[] body)
     /**
      * Mirrors TlsServerProtocol.send13EncryptedExtensionsMessage.
      */
-    protected byte[] generate13EncryptedExtensions(Hashtable serverExtensions)
+    protected byte[] generate13EncryptedExtensions(ServerHandshakeState state, Hashtable serverExtensions)
         throws IOException
     {
         byte[] extBytes = TlsProtocol.writeExtensionsData(serverExtensions);

tls/src/main/java/org/bouncycastle/tls/ServerHello.java

@@ -22,9 +22,13 @@ public class ServerHello
     private final int cipherSuite;
     private final Hashtable extensions;
 
+    /**
+     * A TLS 1.3 HelloRetryRequest, i.e. the 4-argument constructor with the 0x0303 'legacy_version' RFC 8446
+     * 4.1.3 gives it.
+     */
     public ServerHello(byte[] sessionID, int cipherSuite, Hashtable extensions)
     {
-        this(ProtocolVersion.TLSv12, Arrays.clone(HELLO_RETRY_REQUEST_MAGIC), sessionID, cipherSuite, extensions);
+        this(ProtocolVersion.TLSv12, sessionID, cipherSuite, extensions);
     }
 
     /**

tls/src/test/java/org/bouncycastle/tls/DTLS13RetransmissionTest.java

@@ -182,13 +182,13 @@ public void testRetransmissionOfTheFinalFlightIsRecognised()
             FINISHED_LENGTH);
 
         assertTrue("a retransmission of the final flight must be answered",
-            DTLSReliableHandshake.matchesFlight(flight, 2, record, 0, record.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, record, 0, record.length));
 
         byte[] firstHalf = handshakeRecord(HandshakeType.finished, FINISHED_LENGTH, FINAL_FLIGHT_SEQ, 0,
             FINISHED_LENGTH / 2);
 
         assertTrue("a fragment of the final flight must be answered",
-            DTLSReliableHandshake.matchesFlight(flight, 2, firstHalf, 0, firstHalf.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, firstHalf, 0, firstHalf.length));
     }
 
     public void testUnauthenticatedEpochIsNotTakenForTheFinalFlight()
@@ -199,9 +199,12 @@ public void testUnauthenticatedEpochIsNotTakenForTheFinalFlight()
             FINISHED_LENGTH);
 
         assertFalse("epoch 0 is unauthenticated and carries none of the final flight",
-            DTLSReliableHandshake.matchesFlight(flight, 0, record, 0, record.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 0, record, 0, record.length));
         assertFalse("the application epoch carries no handshake flight",
-            DTLSReliableHandshake.matchesFlight(flight, 3, record, 0, record.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 3, record, 0, record.length));
+
+        assertFalse("with no epoch retired there is nothing to recognise",
+            DTLSReliableHandshake.matchesFlight(flight, -1, -1, record, 0, record.length));
     }
 
     public void testForeignHandshakeRecordIsNotTakenForTheFinalFlight()
@@ -211,30 +214,30 @@ public void testForeignHandshakeRecordIsNotTakenForTheFinalFlight()
         byte[] otherSeq = handshakeRecord(HandshakeType.finished, FINISHED_LENGTH, FINAL_FLIGHT_SEQ + 1, 0,
             FINISHED_LENGTH);
         assertFalse("a message_seq the flight never contained",
-            DTLSReliableHandshake.matchesFlight(flight, 2, otherSeq, 0, otherSeq.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, otherSeq, 0, otherSeq.length));
 
         byte[] otherType = handshakeRecord(HandshakeType.key_update, FINISHED_LENGTH, FINAL_FLIGHT_SEQ, 0,
             FINISHED_LENGTH);
         assertFalse("a different message under the flight's message_seq",
-            DTLSReliableHandshake.matchesFlight(flight, 2, otherType, 0, otherType.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, otherType, 0, otherType.length));
 
         byte[] otherLength = handshakeRecord(HandshakeType.finished, FINISHED_LENGTH + 1, FINAL_FLIGHT_SEQ, 0,
             FINISHED_LENGTH + 1);
         assertFalse("a different length under the flight's message_seq",
-            DTLSReliableHandshake.matchesFlight(flight, 2, otherLength, 0, otherLength.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, otherLength, 0, otherLength.length));
 
         byte[] record = handshakeRecord(HandshakeType.finished, FINISHED_LENGTH, FINAL_FLIGHT_SEQ, 0,
             FINISHED_LENGTH);
         assertFalse("a truncated record",
-            DTLSReliableHandshake.matchesFlight(flight, 2, record, 0, record.length - 1));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, record, 0, record.length - 1));
         assertFalse("a record too short to hold a message header",
-            DTLSReliableHandshake.matchesFlight(flight, 2, record, 0, 4));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, record, 0, 4));
 
         byte[] pair = new byte[record.length + otherSeq.length];
         System.arraycopy(record, 0, pair, 0, record.length);
         System.arraycopy(otherSeq, 0, pair, record.length, otherSeq.length);
         assertFalse("one fragment of the flight does not excuse a foreign one beside it",
-            DTLSReliableHandshake.matchesFlight(flight, 2, pair, 0, pair.length));
+            DTLSReliableHandshake.matchesFlight(flight, 2, 2, pair, 0, pair.length));
     }
 
     public void testUnacknowledgedFlightStillRetransmitsEverything() throws Exception