State the peer-keyed nonce reuse precisely and document the lock order, relates to github #1468.

The scoped re-review of the previous commit found that its corrected comment
was still false in one reachable state, and that state is the one the new
test creates. deriveNextWriteEpoch advances the local traffic secret when a
KeyUpdate is sent, while installPendingWriteEpoch swaps the epoch in only
when the acknowledgement arrives. So a peer read epoch derived while our own
KeyUpdate is outstanding carries an encrypt side keyed like the PENDING
write epoch, not the current one.

The conclusion is unchanged, since the key is live either way and both
sequence counters start at zero, but the sentence a maintainer would reason
from was wrong, and checking it against the outstanding-KeyUpdate case would
have suggested the guard was over-cautious.

The lock order between DTLS13PostHandshake's monitor and the record layer's
write lock was held by construction and documented nowhere. It is now stated
at both ends: the monitor is taken first, and a call into that class from
inside a write-lock block would invert the order.

Two test comments named mutations that no longer do what they claim. The
epoch ordering moved out of the test-facing view into getLiveReadEpoch, so
reversing the view proves nothing; and relaxing only the first of the two
guards on a second KeyUpdate fails on the exception raised by the second,
not on the datagram count.

The acknowledgement probe's settle period had been trimmed along with the
key update deadlines it does not share. Its claim is a count of records that
did not arrive, so the margin is restored under its own name.

Author: mondain

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

@@ -22,6 +22,14 @@
  * reducing to "waiting for an ACK and retransmitting the original message". The receiving side, implemented
  * here, is common to all of them: reassemble, dispatch by message type, acknowledge the record.
  * </p>
+ * <p>
+ * Thread safety: a send thread reaches this class through {@code checkKeyUpdateBeforeSend} while a receive
+ * thread reaches it through the ACK listener, the record dispatch and the timers, so every non-private method
+ * is synchronized on this object. LOCK ORDER: this monitor is taken FIRST and the record layer's write lock
+ * second, because the work done here calls back into the record layer. A call into this class from inside a
+ * {@code synchronized (writeLock)} block would take the two in the opposite order and can deadlock; see the
+ * note on that field.
+ * </p>
  */
 class DTLS13PostHandshake
     implements DTLSAckListener
@@ -584,7 +592,13 @@ private void handleMessage(short msg_type, byte[] body)
         }
     }
 
-    /** RFC 9147 7.2. The outstanding post-handshake messages awaiting acknowledgement. */
+    /**
+     * RFC 9147 7.2. The outstanding post-handshake messages awaiting acknowledgement.
+     * <p>
+     * Synchronized only so the reference is published safely. It confers nothing on the caller, who reads and
+     * mutates the tracker outside this monitor. Test-only; there is no production caller.
+     * </p>
+     */
     synchronized DTLS13FlightTracker getFlightTracker()
     {
         return flightTracker;

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

@@ -20,14 +20,19 @@ class DTLSEpoch
      * for BOTH directions: TlsAEADCipher's (D)TLS 1.3 constructor calls rekeyCipher once for the decrypt side,
      * keyed from the peer's traffic secret, and once for the encrypt side, keyed from the LOCAL one.
      * updatePeerReadEpoch updates only the peer's secret (TlsUtils.update13TrafficSecretPeer), so the epoch it
-     * builds carries an encrypt side keyed IDENTICALLY to the current write epoch's, paired with a sequence
+     * builds carries an encrypt side keyed from the LOCAL secret's current value, paired with a sequence
      * number counter of its own that starts at zero.
      *
+     * Which epoch that key belongs to depends on when the derivation happens. Normally it is the current write
+     * epoch's key. If a KeyUpdate of ours is already outstanding it is the PENDING write epoch's, because
+     * deriveNextWriteEpoch advances the local secret when the KeyUpdate is sent and installPendingWriteEpoch
+     * only swaps the epoch in when the ACK arrives. Either way it is a key that is live, or about to be.
+     *
      * So allocating a record from this epoch would not produce something the peer cannot read. It would
-     * produce records encrypted under the SAME AEAD key, at nonces already used for records sent at the
-     * current write epoch. That is AEAD nonce reuse: silent, with the connection still working, and for GCM it
-     * is enough to recover the authentication key. It is the worst outcome an epoch lookup can have, and
-     * nothing observable would report it.
+     * produce records encrypted under the SAME AEAD key as another epoch, at nonces that epoch either has
+     * already used or will use, because both counters start at zero. That is AEAD nonce reuse: silent, with
+     * the connection still working, and for GCM it is enough to recover the authentication key. It is the
+     * worst outcome an epoch lookup can have, and nothing observable would report it.
      *
      * The structural cause, which is why this flag has to exist at all: updatePeerReadEpoch builds a full
      * bidirectional cipher when it needs only the read direction, and so leaves a correctly-keyed encryptor in

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

@@ -127,6 +127,15 @@ private static void sendDatagram(DatagramSender sender, byte[] buf, int off, int
     private final DatagramTransport transport;
 
     private final ByteQueue recordQueue = new ByteQueue();
+
+    /*
+     * Guards the write path, which a send and a receive thread may reach at once.
+     *
+     * LOCK ORDER: every DTLS13PostHandshake method is synchronized on that object, and its work reaches back
+     * into this class and takes writeLock. So the only permitted order is the post-handshake monitor first,
+     * writeLock second. Never call into postHandshake from inside a synchronized (writeLock) block: that is
+     * the reverse order and two threads taking the two orders at once would deadlock.
+     */
     private final Object writeLock = new Object();
 
     // github #1487. While a flight is open, records are packed into as few datagrams as the MTU allows.
@@ -2269,11 +2278,12 @@ private DTLSEpoch getEpochForRetransmit(int epoch)
          * (DTLSEpoch.isPeerKeyed, set only by updatePeerReadEpoch) may be read at and must never be written
          * at. Its encrypt side is NOT the peer's - TlsUtils.initCipher keys both directions, the decrypt side
          * from the peer's traffic secret and the encrypt side from the local one, and updatePeerReadEpoch
-         * updates only the peer's secret - so that epoch's encryptor is keyed identically to the current write
-         * epoch's, while its sequence number counter starts again at zero. Writing at it would therefore put
-         * records on the wire under the SAME AEAD key at nonces the current write epoch has already used, and
-         * a peer would read them perfectly well. See DTLSEpoch.peerKeyed for why that is worse than a
-         * decryption failure would be.
+         * updates only the peer's secret - so that epoch's encryptor is keyed from the local secret as it
+         * stands, which is the current write epoch's key, or the pending write epoch's if a KeyUpdate of ours
+         * is already outstanding. Its sequence number counter starts again at zero either way. Writing at it
+         * would therefore put records on the wire under the SAME AEAD key as another epoch, at nonces that
+         * epoch has used or will use, and a peer would read them perfectly well. See DTLSEpoch.peerKeyed for
+         * why that is worse than a decryption failure would be.
          *
          * Before post-handshake key updates every held epoch was one both directions shared, so this could not
          * arise; once the read side advances on its own it can, and the epoch numbers of the two directions

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

@@ -816,10 +816,12 @@ public void testASecondKeyUpdateIsNotStartedWhileOneIsOutstanding() throws Excep
      * </p>
      * <p>
      * Mutations this test is built to catch: remove the outstanding-KeyUpdate latch at the top of
-     * {@code checkKeyUpdateBeforeSend} - i.e. follow RFC 8446 instead - and the deferral half fails, because
-     * a second KeyUpdate goes out while one is unacknowledged; drop the {@code keyUpdatePendingSend} latch
-     * (clear it in {@code handleMessage} instead of recording it) and the discharge half fails, because the
-     * answering KeyUpdate is never sent at all.
+     * {@code checkKeyUpdateBeforeSend} - i.e. try to follow RFC 8446 instead - and the deferral half fails on
+     * the {@code IllegalStateException} raised by {@code sendKeyUpdate}'s own RFC 9147 5.8.4 guard, which is
+     * the second of the two. Relax both, so that the implementation conforms to RFC 8446, and it fails
+     * instead on the datagram count, because a second KeyUpdate goes out while one is unacknowledged. Drop
+     * the {@code keyUpdatePendingSend} latch (clear it in {@code handleMessage} instead of recording it) and
+     * the discharge half fails, because the answering KeyUpdate is never sent at all.
      * </p>
      */
     public void testAnUpdateRequestedIsDeferredWhileOurOwnKeyUpdateIsOutstanding() throws Exception

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

@@ -10,8 +10,9 @@
 import junit.framework.TestCase;
 
 /**
- * RFC 9147 4.2.2 and 8. The record layer resolves a received record's epoch from one ordered collection of
- * live read epochs, iterated most recent first, and writes at an epoch through the same collection.
+ * RFC 9147 4.2.2 and 8. The record layer resolves a received record's epoch from one ordered set of live read
+ * epochs, walked most recent first by {@code getLiveReadEpoch(int)}, and writes at an epoch through the same
+ * walk. {@code getLiveReadEpochs()} is a view over that walk for tests; the record path does not use it.
  * <p>
  * The load-bearing test here is {@link #testAliasingEpochResolvesToTheMostRecentMatch()}: only the low 2
  * epoch bits are on the wire, so two live epochs can alias, and the RFC resolves that to the most recent of
@@ -119,9 +120,10 @@ public void testLiveReadEpochsAreOrderedMostRecentFirst() throws Exception
      * bits that are on the wire, and both are live and able to decode the record, so the only thing that
      * decides which one the record is attributed to is the order the collection is iterated in.
      * <p>
-     * Mutation this test is built to catch: reverse the iteration in {@code getLiveReadEpochs} (or in its
-     * consumer in {@code processDTLS13Record}) and the record resolves to epoch 2 instead, failing every
-     * assertion below.
+     * Mutation this test is built to catch: reverse the slot order in {@code getLiveReadEpoch(int)} (or in
+     * its consumer {@code resolveReadEpochByHeaderBits}) and the record resolves to epoch 2 instead, failing
+     * every assertion below. Reversing {@code getLiveReadEpochs()} alone would not, since that is only the
+     * test-facing view.
      * </p>
      */
     public void testAliasingEpochResolvesToTheMostRecentMatch() throws Exception

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

@@ -142,8 +142,14 @@ public class DTLS13ProtocolTest
      * that. The settle period is spent on every run, and is what lets a peer's answer to the last record
      * arrive, so it has to stay long enough to be reliable and short enough not to dominate the suite.
      */
-    private static final int KEY_UPDATE_DEADLINE_MILLIS = 8000;
-    private static final int KEY_UPDATE_SETTLE_MILLIS = 1000;
+    private static final int POST_HANDSHAKE_DEADLINE_MILLIS = 8000;
+    private static final int POST_HANDSHAKE_SETTLE_MILLIS = 1000;
+
+    /*
+     * The ACK probe waits longer before concluding that nothing more is coming, because its whole claim is a
+     * count of records that did NOT arrive. Trimming this trades a real margin for no measurable runtime.
+     */
+    private static final int ACK_PROBE_SETTLE_MILLIS = 1500;
 
     /** The application payloads of the exchanges after the handshake, distinct in both length and content. */
     private static final byte[] REQUEST_2 = new byte[24];
@@ -2663,8 +2669,8 @@ private void runKeyUpdate(DTLSTransport dtlsClient, ServerThread serverThread) t
              */
             int target = clientEpoch3Before + (dropKeyUpdateDatagram ? 2 : 1);
 
-            pumpUntilClientRecordsAtEpoch(dtlsClient, 3, target, KEY_UPDATE_DEADLINE_MILLIS,
-                KEY_UPDATE_SETTLE_MILLIS, earlier);
+            pumpUntilClientRecordsAtEpoch(dtlsClient, 3, target, POST_HANDSHAKE_DEADLINE_MILLIS,
+                POST_HANDSHAKE_SETTLE_MILLIS, earlier);
 
             this.clientKeyUpdateCopies = countRecordsAtEpoch(clientRecords(), 3) - clientEpoch3Before;
 
@@ -2817,7 +2823,7 @@ private void probeServerAckPath(DTLSTransport dtlsClient) throws IOException
             int expected = holdFirstClientEpoch2Datagram ? 2 : 1;
 
             this.serverEpoch3AfterHandshake = drainServerEpoch3(dtlsClient, expected,
-                KEY_UPDATE_DEADLINE_MILLIS, KEY_UPDATE_SETTLE_MILLIS);
+                POST_HANDSHAKE_DEADLINE_MILLIS, ACK_PROBE_SETTLE_MILLIS);
 
             injectPlaintextHandshakeRecord();