RFR: 8299870: TLS record version check allows invalid records

[Matthew Donovan]

On Tue, 10 Jan 2023 19:25:32 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

> This update will introduce version negotiation issues. Per TLS spec, version 105.106 should be allowed and the version could be negotiated properly. When TLS 1.4 is defined later in the future, the code update here will cause serious compatibility issues. This has been a well-known issue in some implementations.

I'm not sure what you mean here. Can you point me towards the spec that you're referring to? 

If we need to support later, currently undefined, versions then is IllegalRecordVersion a valid test?

> 
> If you want to fix the javax/net/ssl/SSLEngine/IllegalRecordVersion.java issue, please refer to the JDK-8042449 patch details.

I looked at that original patch and the code history after that. The original patch (JDK-8042449) created/updated `InputRecord.checkRecordVersion()` But then in this [commit](https://github.com/openjdk/jdk/commit/87c6761704ad5ae5f03c0cbc4de33a97ddb9317c#diff-0598abb15a2271a12c36131833ff938ab9d841b1ed69bc1b4eeabfcc460067d8) the `checkRecordVersion()` method was removed entirely and any version-checking was moved to `ProtocolVersion.isNegotiable()`.

The original `checkRecordVersion()` is:

if ((version.v < ProtocolVersion.MIN.v) ||
            ((version.major & 0xFF) > (ProtocolVersion.MAX.major & 0xFF))) {

            // if it's not SSLv2, we're out of here.
            if (!allowSSL20Hello ||
                    (version.v != ProtocolVersion.SSL20Hello.v)) {
                throw new SSLException("Unsupported record version " + version);
            }
        }


The logic is structured differently, but I'm pretty sure that it's has the same result.

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

PR: https://git.openjdk.org/jdk/pull/11929