Assert the server's own refusal and the second ClientHello's message_seq, relates to github #1468.

The re-review of the previous fixes found two comments claiming more than the
code does and two test assertions weaker than their messages.

The notification comment said the call was placed exactly as
TlsServerProtocol does it. The version gating is mirrored; the ordering
relative to the other TlsServer callbacks is not, and cannot be, because
DTLS selects the version in generateServerHello, by which point
establishClientSigAlgs and processClientExtensions have already run. Say
both, rather than claiming a wholesale mirror.

DTLSVerifier produces a DTLSRequest for any ClientHello carrying a cookie it
has verified, so it need not have sent the HelloVerifyRequest during that
call. The invariant is that the ClientHello arrived through the cookie
exchange.

The server-refusal test asserted the internal_error the client receives,
which is not specific to this cause, so the harness now records what the
server itself threw and the test asserts that diagnostic. The harness also
stops printing a stack trace for a server abort a test expects, which
otherwise makes a passing test look like a failing one.

Counting datagrams that carry a ClientHello cannot tell a new message from a
retransmission of the first, so the scripted transport records each
ClientHello's message_seq and the test requires it to advance to 1, per
RFC 6347 4.2.2.

Author: mondain

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

@@ -134,8 +134,9 @@ protected DTLSTransport serverHandshake(ServerHandshakeState state, DTLSRequest
         {
             /*
              * Reached through accept(TlsServer, DatagramTransport, DTLSRequest), i.e. behind DTLSVerifier,
-             * which has already sent a DTLS 1.2 HelloVerifyRequest on this connection - see the check in
-             * generateServerHello.
+             * so this ClientHello arrived through the DTLS 1.2 cookie exchange: DTLSVerifier only produces a
+             * DTLSRequest for a ClientHello carrying a cookie it has verified, though it need not have sent
+             * the HelloVerifyRequest itself during this call - see the check in generateServerHello.
              */
             state.afterHelloVerifyRequest = true;
 
@@ -1677,12 +1678,18 @@ protected void processClientHello(ServerHandshakeState state, ClientHello client
 
         /*
          * NOTE: server.notifySecureRenegotiation is called from generateServerHello, in the
-         * DTLS-1.2-and-below portion past the point where the DTLS 1.3 path has returned, exactly as
-         * TlsServerProtocol.generateServerHello does it. RFC 8446 / RFC 9147 remove renegotiation, so a
-         * client offering only DTLS 1.3 (or later) legitimately sends neither the "renegotiation_info"
-         * extension nor the SCSV - see the matching 'offeringDTLSv12Minus' gate in
-         * DTLSClientProtocol.generateClientHello - and gating on the selected version rather than on the
-         * client's offer is what keeps a {1.3, 1.2} offer with neither of them acceptable.
+         * DTLS-1.2-and-below portion past the point where the DTLS 1.3 path has returned. That mirrors the
+         * version gating of TlsServerProtocol.generateServerHello, which likewise notifies only once a
+         * version at or below 1.2 has been selected. It does NOT mirror its ordering relative to the other
+         * TlsServer callbacks: DTLS selects the version in generateServerHello, so by the time the
+         * notification is made here establishClientSigAlgs and server.processClientExtensions have already
+         * run from processClientHello, whereas TlsServerProtocol notifies before both.
+         *
+         * RFC 8446 / RFC 9147 remove renegotiation, so a client offering only DTLS 1.3 (or later)
+         * legitimately sends neither the "renegotiation_info" extension nor the SCSV - see the matching
+         * 'offeringDTLSv12Minus' gate in DTLSClientProtocol.generateClientHello - and gating on the selected
+         * version rather than on the client's offer is what keeps a {1.3, 1.2} offer with neither of them
+         * acceptable.
          */
 
         if (clientHelloExtensions != null)

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

@@ -97,6 +97,15 @@ public void testClientRefusesDTLSv13SelectedAfterAHelloVerifyRequest() throws Ex
 
         assertEquals("the client must have answered the HelloVerifyRequest", 2,
             transport.clientHellosSeen());
+
+        /*
+         * A retransmission of the first ClientHello would also be a second datagram carrying a ClientHello,
+         * so check the message_seq advanced: RFC 6347 4.2.2 requires the ClientHello answering a
+         * HelloVerifyRequest to be a new message, at message_seq 1.
+         */
+        assertEquals("the first ClientHello is at message_seq 0", 0, transport.clientHelloSeq(0));
+        assertEquals("the second ClientHello must be a new message, not a retransmission", 1,
+            transport.clientHelloSeq(1));
     }
 
     private short connectAndExpectFatalAlert(byte[] selectedVersion) throws Exception
@@ -250,6 +259,8 @@ private static class ScriptedServerHelloTransport
     {
         private final byte[][] script;
 
+        private final int[] clientHelloSeqs = new int[8];
+
         private int clientHellosSeen = 0;
         private byte[] pending = null;
 
@@ -268,6 +279,16 @@ int clientHellosSeen()
             return clientHellosSeen;
         }
 
+        /**
+         * The DTLS handshake message_seq of the ClientHello at the given index, so that a retransmission of
+         * an earlier ClientHello can be told apart from a genuinely new one - both are datagrams carrying a
+         * ClientHello, and only the message_seq distinguishes them.
+         */
+        int clientHelloSeq(int index)
+        {
+            return clientHelloSeqs[index];
+        }
+
         public int getReceiveLimit()
         {
             return MTU;
@@ -284,6 +305,13 @@ public void send(byte[] buf, int off, int len) throws IOException
             if (clientHellosSeen < script.length && len > 13 && (buf[off] & 0xFF) == ContentType.handshake
                 && (buf[off + 13] & 0xFF) == HandshakeType.client_hello)
             {
+                // Handshake message header: msg_type, length, then message_seq at offsets 4 and 5
+                if (clientHellosSeen < clientHelloSeqs.length)
+                {
+                    clientHelloSeqs[clientHellosSeen] = ((buf[off + 17] & 0xFF) << 8)
+                        | (buf[off + 18] & 0xFF);
+                }
+
                 this.pending = script[clientHellosSeen++];
             }
         }

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

@@ -786,6 +786,7 @@ public void testServerRefusesToSelectDTLSv13BehindAHelloVerifyRequest() throws E
     {
         Harness harness = new Harness();
         harness.helloVerifyRequestFrontEnd = true;
+        harness.expectServerAbort = true;
         harness.clientVersions = ProtocolVersion.DTLSv13.downTo(ProtocolVersion.DTLSv12);
         harness.serverVersions = ProtocolVersion.DTLSv13.downTo(ProtocolVersion.DTLSv12);
 
@@ -809,6 +810,16 @@ public void testServerRefusesToSelectDTLSv13BehindAHelloVerifyRequest() throws E
         assertEquals("the version the server refused to go on with", ProtocolVersion.DTLSv13,
             harness.serverVersion);
 
+        /*
+         * The client only ever sees the resulting alert, and internal_error is not specific to this cause.
+         * Assert on what the SERVER threw, so the test cannot pass because something else on the server went
+         * wrong at the same point.
+         */
+        assertNotNull("the server must be the side that aborted", harness.serverAbort);
+        assertEquals("the server's own diagnostic",
+            "internal_error(80); DTLS 1.3 cannot be negotiated behind a HelloVerifyRequest front end",
+            harness.serverAbort.getMessage());
+
         assertEquals("no application data can have been exchanged", null, harness.echo);
     }
 
@@ -1750,6 +1761,9 @@ static class Harness
         boolean replaySecondHelloRetryRequest = false;
         int mangleSecondClientHello = MANGLE_NONE;
         boolean helloVerifyRequestFrontEnd = false;
+        boolean expectServerAbort = false;
+
+        Exception serverAbort = null;
 
         /*
          * RFC 5746 3.4. Strips the TLS_EMPTY_RENEGOTIATION_INFO_SCSV from the ClientHello on its way out, so
@@ -2194,7 +2208,7 @@ public void notifyHandshakeComplete() throws IOException
             DTLSServerProtocol serverProtocol = new DTLSServerProtocol();
 
             ServerThread serverThread = new ServerThread(serverProtocol, server, network.getServer(),
-                helloVerifyRequestFrontEnd);
+                helloVerifyRequestFrontEnd, expectServerAbort);
             serverThread.setDaemon(true);
             serverThread.start();
 
@@ -2257,6 +2271,8 @@ public void notifyHandshakeComplete() throws IOException
                 this.mangled = recording.getMangled();
 
                 serverThread.shutdown();
+
+                this.serverAbort = serverThread.getCaught();
             }
         }
 
@@ -2414,15 +2430,28 @@ static class ServerThread
         private final TlsServer server;
         private final DatagramTransport serverTransport;
         private final boolean helloVerifyRequestFrontEnd;
+        private final boolean expectAbort;
         private volatile boolean isShutdown = false;
+        private volatile Exception caught = null;
 
         ServerThread(DTLSServerProtocol serverProtocol, TlsServer server, DatagramTransport serverTransport,
-            boolean helloVerifyRequestFrontEnd)
+            boolean helloVerifyRequestFrontEnd, boolean expectAbort)
         {
             this.serverProtocol = serverProtocol;
             this.server = server;
             this.serverTransport = serverTransport;
             this.helloVerifyRequestFrontEnd = helloVerifyRequestFrontEnd;
+            this.expectAbort = expectAbort;
+        }
+
+        /**
+         * Whatever aborted the server, for a test whose subject is the server's own refusal: the client only
+         * ever sees the alert that results, so asserting on this is what distinguishes the server raising the
+         * refusal from the client inferring something from an alert.
+         */
+        Exception getCaught()
+        {
+            return caught;
         }
 
         public void run()
@@ -2452,7 +2481,16 @@ public void run()
             }
             catch (Exception e)
             {
-                e.printStackTrace();
+                this.caught = e;
+
+                /*
+                 * A test whose subject IS the server's refusal aborts here on every run, so printing would
+                 * make a passing test look like a failing one. It asserts on getCaught() instead.
+                 */
+                if (!expectAbort)
+                {
+                    e.printStackTrace();
+                }
             }
         }