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

[friedbyalice]

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.

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.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8366453: TLS 1.3 KeyUpdate record is not rejected if not on a record boundary (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27437/head:pull/27437
$ git checkout pull/27437

Update a local copy of the PR:
$ git checkout pull/27437
$ git pull https://git.openjdk.org/jdk.git pull/27437/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27437

View PR using the GUI difftool:
$ git pr show -t 27437

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27437.diff

[bridgekeeper]

👋 Welcome back friedbyalice! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@friedbyalice The following label will be automatically applied to this pull request:

• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[myankelev]

Just a few minor comments.
Also tests :)

[myankelev]

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

[myankelev]

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.

[friedbyalice]

Renamed to markT13keyChangeHsExceedsRecordBoundary, not sure about not mentioning tls13 in its name.