On Mon, 22 Sep 2025 19:48:20 GMT, Alice Pellegrini <[email protected]> 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
