RFR: 8366453: TLS 1.3 KeyUpdate record is not rejected if not on a record boundary

[Mikhail Yankelevich]

On Mon, 22 Sep 2025 19:48:20 GMT, Alice Pellegrini <duke at openjdk.org> wrote:

> According to RFC 8446 section 5.1
>> Handshake messages MUST NOT span key changes. 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.
> 
> The TLS implementation does not fail with alert(fatal, unexpected_message) when a KeyUpdate record is not on a record boundary, but this is required by the specification (as a key change happens immediately after a key update record)
> 
> 
> Since the data on whether a message aligns with a record boundary is only known in the implementations of `InputRecord` (as even incomplete parts of other handshake messages, if coming after one of the mentioned handshakes records, would require a failure, making checking that said message is the last complete one of that record insufficient), and the fact that, **if the negotiated protocol is TLS13** _(or even DTLS13 in the future)_, knowing that any of the mentioned messages did not align with the record boundary is enough to fail the connection, I am proposing to add this as a method of `InputRecord`; 
> 
> In addition, even if the handshake context was accessible from within `InputRecord`, for both ServerHello and ClientHello the negotiated protocol version is not known when the input record is decoded.
> 
> The change mentions the name of the message currently being consumed in the exception because (since the messages are consumed in the order in which they appear in the record's body, and the groups of messages contained in each record are consumed in the order in which said records were delivered) it can be shown that if that flag is set, the first consumer that calls `tls13keyChangeHsExceedsRecordBoundary` will correspond to the first message to violate the boundary requirement, among the messages in the record it was found in.
> 
> <br/><br/>
> 
> I would appreciate suggestions on how to make the code better, especially in terms of where and how to store the fact that the violation might (if the negotiated protocol is or will be TLS13) have happened, and where to put the comments mentioning the specification RFC8446, for example in the `InputRecord` base class or the TLS13 Consumers that were modified.

Just a few minor comments.
Also tests :)

src/java.base/share/classes/sun/security/ssl/Finished.java line 932:

> 930:             }
> 931: 
> 932:             if (chc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {

Do you think there might be a way to not repeat the same logic. Mb moving it to the `InputRecord` itself. This way you will also be able to remove `t13keyChangeHsExceedsRecordBoundary` which should streamline the code and reduce operations. 

If you think that this is better, it's fine with me

src/java.base/share/classes/sun/security/ssl/InputRecord.java line 161:

> 159:     }
> 160: 
> 161:     protected final void setT13keyChangeHsExceedsRecordBoundary() {

How about making this `markT13keyChangeHsExceedsRecordBoundary`, as it's not a setter. Or may be there is a better name. 

Also, I'd think about explicitly calling tls13 in the name, as it is going to be called in `SSLSocketInputRecord.java` for tls1.2 and older. Mb just name it in a generic way. But I don't mind to leave it as it is.

src/java.base/share/classes/sun/security/ssl/KeyUpdate.java line 194:

> 192:             PostHandshakeContext hc = (PostHandshakeContext)context;
> 193: 
> 194:             if (hc.negotiatedProtocol.useTLS13PlusSpec() && hc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {

Suggestion:

if (hc.negotiatedProtocol.useTLS13PlusSpec() &&
    hc.conContext.inputRecord.t13keyChangeHsExceedsRecordBoundary()) {

src/java.base/share/classes/sun/security/ssl/SSLEngineInputRecord.java line 349:

> 347:                     // MUST send these messages in alignment with a record boundary."
> 348:                     //
> 349:                     // this check must be done here, as the handshakeBuffer is not accessible to the outer scope,

nit: 80 characters

src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java line 376:

> 374:                     // MUST send these messages in alignment with a record boundary."
> 375:                     //
> 376:                     // this check must be done here, as the handshakeBuffer is not accessible to the outer scope,

nit: 80 characters

-------------

PR Review: https://git.openjdk.org/jdk/pull/27437#pullrequestreview-3267402523
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2378913282
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2378928358
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2378918372
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2378915832
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2378915303