First draft of more correct check of tls version

Author: friedbyalice

src/java.base/share/classes/sun/security/ssl/ClientHello.java

@@ -1144,6 +1144,11 @@ public void consume(ConnectionContext context,
                         "The ClientHello.legacy_version field is not TLS 1.2");
             }
 
+            if (shc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {
+                throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
+                                    "CLIENT_HELLO messages must align with a record boundary");
+            }
+
             // The client may send a dummy change_cipher_spec record
             // immediately after the first ClientHello.
             shc.conContext.consumers.putIfAbsent(

src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java

@@ -170,7 +170,7 @@ Plaintext[] decode(ByteBuffer packet) throws SSLProtocolException {
 
         // Buffer next epoch message if necessary.
         if (this.readEpoch < recordEpoch) {
-            // Discard the record younger than the current epcoh if:
+            // Discard the record younger than the current epoch if:
             // 1. it is not a handshake message, or
             // 3. it is not of next epoch.
             if ((contentType != ContentType.HANDSHAKE.id &&
@@ -310,6 +310,12 @@ int bytesInCompletePacket(
         return bytesInCompletePacket(srcs[srcsOffset]);
     }
 
+    @Override
+    public boolean t13keyChangeHsExceedsRecordBoundary() {
+        // DTLS 1.3 is not supported
+        return false;
+    }
+
     private int bytesInCompletePacket(ByteBuffer packet) throws SSLException {
 
         // DTLS length field is in bytes 11/12

src/java.base/share/classes/sun/security/ssl/Finished.java

@@ -935,6 +935,11 @@ private void onConsumeFinished(ClientHandshakeContext chc,
                         "Consuming server Finished handshake message", fm);
             }
 
+            if (chc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {
+                throw chc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
+                        "FINISHED messages must align with a record boundary");
+            }
+
             // Save client verify data for secure renegotiation.
             if (chc.conContext.secureRenegotiation) {
                 chc.conContext.serverVerifyData = fm.verifyData;
@@ -1078,6 +1083,11 @@ private void onConsumeFinished(ServerHandshakeContext shc,
                         "Consuming client Finished handshake message", fm);
             }
 
+            if (shc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {
+                throw shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
+                        "FINISHED messages must align with a record boundary");
+            }
+
             if (shc.conContext.secureRenegotiation) {
                 shc.conContext.clientVerifyData = fm.verifyData;
             }

src/java.base/share/classes/sun/security/ssl/InputRecord.java

@@ -150,6 +150,8 @@ int bytesInCompletePacket() throws IOException {
         throw new UnsupportedOperationException();
     }
 
+    public abstract boolean t13keyChangeHsExceedsRecordBoundary();
+
     // apply to SSLSocket only
     void setReceiverStream(InputStream inputStream) {
         throw new UnsupportedOperationException();

src/java.base/share/classes/sun/security/ssl/KeyUpdate.java

@@ -196,6 +196,11 @@ public void consume(ConnectionContext context,
                         "Consuming KeyUpdate post-handshake message", km);
             }
 
+            if (hc.negotiatedProtocol.useTLS13PlusSpec() && hc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {
+                throw hc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
+                        "KEYUPDATE messages must align with a record boundary");
+            }
+
             // Update read key and IV.
             SSLTrafficKeyDerivation kdg =
                 SSLTrafficKeyDerivation.valueOf(hc.conContext.protocolVersion);

src/java.base/share/classes/sun/security/ssl/SSLEngineInputRecord.java

@@ -44,6 +44,9 @@ final class SSLEngineInputRecord extends InputRecord implements SSLRecord {
     // Cache for incomplete handshake messages.
     private ByteBuffer handshakeBuffer = null;
 
+    // mark for possible TLS13 RFC violation, if messages preceding key change do not align with record bounds
+    private boolean t13keyChangeHsExceedsRecordBoundary = false;
+
     SSLEngineInputRecord(HandshakeHash handshakeHash) {
         super(handshakeHash, SSLReadCipher.nullTlsReadCipher());
     }
@@ -64,6 +67,11 @@ int bytesInCompletePacket(
         return bytesInCompletePacket(srcs[srcsOffset]);
     }
 
+    @Override
+    public boolean t13keyChangeHsExceedsRecordBoundary() {
+        return t13keyChangeHsExceedsRecordBoundary;
+    }
+
     private int bytesInCompletePacket(ByteBuffer packet) throws SSLException {
         /*
          * SSLv2 length field is in bytes 0/1
@@ -338,6 +346,20 @@ private Plaintext[] decodeInputRecord(ByteBuffer packet)
                         handshakeHash.receive(handshakeFrag);
                     }
 
+                    // From RFC 8446:
+                    // "Implementations MUST verify that all messages immediately preceding a key change
+                    // align with a record boundary; if not, then they MUST terminate the
+                    // connection with an "unexpected_message" alert. Because the
+                    // ClientHello, EndOfEarlyData, ServerHello, Finished, and KeyUpdate
+                    // messages can immediately precede a key change, implementations
+                    // MUST send these messages in alignment with a record boundary."
+                    //
+                    // this check must be done here, as the handshakeBuffer is not accessible to the outer scope,
+                    // therefore there is no way to check whether the handshake message was aligned with the boundary
+                    if (nextPos < fragLim && SSLHandshake.precedesKeyChange(handshakeType)) {
+                        t13keyChangeHsExceedsRecordBoundary = true;
+                    }
+
                     plaintexts.add(
                         new Plaintext(contentType, majorVersion, minorVersion,
                             -1, -1L, handshakeFrag.slice())

src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java

@@ -59,6 +59,9 @@ final class SSLSocketInputRecord extends InputRecord implements SSLRecord {
     // Cache for incomplete handshake messages.
     private ByteBuffer handshakeBuffer = null;
 
+    // mark for possible TLS13 RFC violation, if messages preceding key change do not align with record bounds
+    private boolean t13keyChangeHsExceedsRecordBoundary = false;
+
     SSLSocketInputRecord(HandshakeHash handshakeHash) {
         super(handshakeHash, SSLReadCipher.nullTlsReadCipher());
     }
@@ -193,6 +196,11 @@ Plaintext[] decode(ByteBuffer[] srcs, int srcsOffset,
         return plaintext;
     }
 
+    @Override
+    public boolean t13keyChangeHsExceedsRecordBoundary() {
+        return t13keyChangeHsExceedsRecordBoundary;
+    }
+
     @Override
     void setReceiverStream(InputStream inputStream) {
         this.is = inputStream;
@@ -372,12 +380,11 @@ private Plaintext[] decodeInputRecord() throws IOException, BadPaddingException
                     // ClientHello, EndOfEarlyData, ServerHello, Finished, and KeyUpdate
                     // messages can immediately precede a key change, implementations
                     // MUST send these messages in alignment with a record boundary."
-                    if (helloVersion.useTLS13PlusSpec() &&
-                            nextPos < fragLim && SSLHandshake.precedesKeyChange(handshakeType)) {
-                        throw new SSLProtocolException(
-                                "The handshake message of type " + SSLHandshake.nameOf(handshakeType) +
-                                        " must end on a Record boundary."
-                        );
+                    //
+                    // this check must be done here, as the handshakeBuffer is not accessible to the outer scope,
+                    // therefore there is no way to check whether the handshake message was aligned with the boundary
+                    if (nextPos < fragLim && SSLHandshake.precedesKeyChange(handshakeType)) {
+                        t13keyChangeHsExceedsRecordBoundary = true;
                     }
 
                     handshakeFrag.position(nextPos);

src/java.base/share/classes/sun/security/ssl/ServerHello.java

@@ -1239,6 +1239,12 @@ public void consume(ConnectionContext context,
                     "The ServerHello.legacy_version field is not TLS 1.2");
             }
 
+            if (chc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {
+                throw chc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
+                        "CLIENT_HELLO messages must align with a record boundary");
+            }
+
+
             chc.negotiatedCipherSuite = serverHello.cipherSuite;
             chc.handshakeHash.determine(
                     chc.negotiatedProtocol, chc.negotiatedCipherSuite);