On Thu, 25 Sep 2025 12:31:28 GMT, Mikhail Yankelevich
<[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 Cons...
>
> 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.
Renamed to `markT13keyChangeHsExceedsRecordBoundary`, not sure about not
mentioning tls13 in its name.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27437#discussion_r2379414290
