8298873: Update IllegalRecordVersion.java for changes to TLS implementation #11929

mpdonova · 2023-01-10T18:59:30Z

Updated ProtocolVersion.isNegotiable() to check a bounded range of version numbers.
Removed IllegalRecordVersion.java from ProblemList.txt

Tested with jdk_security and jdk_security3 test groups.

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-8298873: Update IllegalRecordVersion.java for changes to TLS implementation

Reviewers

Rajan Halade (@rhalade - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11929

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

Using diff file

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

bridgekeeper · 2023-01-10T19:00:57Z

👋 Welcome back mpdonova! 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 · 2023-01-10T19:03:47Z

@mpdonova 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.

mlbridge · 2023-01-10T19:07:42Z

Webrevs

XueleiFan

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.

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

mpdonova · 2023-01-10T20:16:02Z

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 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.

XueleiFan · 2023-01-10T20:34:49Z

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?

Please refer to "Appendix E. Backward Compatibility" of RFC 5246. Let see an example, suppose TLS 1.4 is defined. If the server is only able to accept TLS 1.3, if the client is using TLS 1.4 format, the connection cannot be established. But TLS 1.3 should be negotiated.

BTW, this filed has been deprecated and "MUST be ignored for all purposes" since TLS 1.3 (See RFC 8446).

If we need to support later, currently undefined, versions then is IllegalRecordVersion a valid test?
That's the good question. It may worthy of further evaluation and the test case could be removed if it is not valid.

rhalade · 2023-01-10T22:46:06Z

test/jdk/ProblemList.txt

@@ -587,7 +587,6 @@ sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java            8161536 generic-
 sun/security/tools/keytool/ListKeychainStore.sh                 8156889 macosx-all

 javax/net/ssl/SSLEngine/TestAllSuites.java                      8298874 generic-all
-javax/net/ssl/SSLEngine/IllegalRecordVersion.java               8298873 generic-all


I have closed 8298873 as duplicate of this bug. Can you please update IllegalRecordVersion test to list 8299870 under @bug.

Since 8299870 is test only bug now, we don't need to list it.

mpdonova · 2023-01-11T19:49:14Z

If we need to support later, currently undefined, versions then is IllegalRecordVersion a valid test?
That's the good question. It may worthy of further evaluation and the test case could be removed if it is not valid.

I reworked IllegalRecordVersion.java so that it creates a ClientHello with a bad value in that version field, continues the handshake to the end, and then verifies that a version was agreed upon.

If that sounds legitimate, I can clean up the code a little and push it.

XueleiFan · 2023-01-12T07:15:25Z

If we need to support later, currently undefined, versions then is IllegalRecordVersion a valid test?
That's the good question. It may worthy of further evaluation and the test case could be removed if it is not valid.

I reworked IllegalRecordVersion.java so that it creates a ClientHello with a bad value in that version field, continues the handshake to the end, and then verifies that a version was agreed upon.

If that sounds legitimate, I can clean up the code a little and push it.

I'm fine for this approach. The file/class name could be revised.

XueleiFan

Would you please update the subject and description of JDK-8299870 so that it fit better with the purpose of the patch?

XueleiFan · 2023-01-12T15:50:30Z

src/java.base/share/classes/sun/security/ssl/ProtocolVersion.java

+    static final ProtocolVersion MAX_TLS_SUPPORTED = TLS13;
+    static final ProtocolVersion MIN_TLS_SUPPORTED = SSL30;
+


Would you mind restore the update for ProrocolVersion.java?

XueleiFan · 2023-01-12T16:03:34Z

test/jdk/javax/net/ssl/SSLEngine/HandshakeWithInvalidRecordVersion.java

+    private static final String PATH_TO_STORES = "../etc";
+    private static final String KEYSTORE_FILE = "keystore";
+    private static final String TRUSTSTORE_FILE = "truststore";
+
+    private static final String KEYSTORE_PATH =
+            System.getProperty("test.src", "./") + "/" + PATH_TO_STORES +
+                    "/" + KEYSTORE_FILE;
+    private static final String TRUSTSTORE_PATH =
+            System.getProperty("test.src", "./") + "/" + PATH_TO_STORES +
+                    "/" + TRUSTSTORE_FILE;


It would be nice to avoid to use binary files. Would you mind to check if test/jdk/javax/net/ssl/templates/SSLContextTemplate.java could be used for the generation of SSLContext (for example test/jdk/javax/net/ssl/templates/SSLEngineTemplate.java)?

There are few other tests that need update as well. We should create backlog item to update all these tests to use the SSLEngineTemplate to generate SSLContext.

rhalade · 2023-01-12T23:55:02Z

src/java.base/share/classes/sun/security/ssl/ProtocolVersion.java

@@ -407,4 +407,4 @@ static ProtocolVersion selectedFrom(

        return selectedVersion;
    }
-}
+}


revert this change as well.

rhalade · 2023-01-19T18:59:25Z

Thinking may be it is better to close JDK-8299870 and "Not an Issue" for archival purpose and address the test fix as part of JDK-8298873. What do you think?

mpdonova · 2023-01-26T18:18:26Z

/integrate

openjdk · 2023-01-26T18:20:20Z

@mpdonova This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8298873: Update IllegalRecordVersion.java for changes to TLS implementation

Reviewed-by: rhalade

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 312 new commits pushed to the master branch:

a2a7703: 8299444: java.util.Set.copyOf allocates needlessly for empty input collections
d98a323: 8301070: Replace NULL with nullptr in share/memory/
315398c: 8221785: Let possibly_parallel_threads_do cover the same threads as threads_do
14114c2: 8301005: Clean up Copy::conjoint_*_atomic on windows
973f741: 8300968: Accessorize raw oop load in DeadCounterClosure
64ddf95: 8299858: [Metrics] Swap memory limit reported incorrectly when too large
28545dc: 8300247: Harden C1 xchg on AArch64 and PPC
3f63381: 8300913: ZGC: assert(to_addr != 0) failed: Should be forwarded
da80e7a: 8300962: Parallel: Remove PSParallelCompact::_total_invocations
7725fe8: 8299953: Merge ContiguousSpaceDCTOC into DirtyCardToOopClosure
... and 302 more: https://git.openjdk.org/jdk/compare/66db0bb6a15310e4e60ff1e33d40e03c52c4eca8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XueleiFan, @rhalade) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2023-01-26T18:21:21Z

@mpdonova
Your change (at version 13643dd) is now ready to be sponsored by a Committer.

rhalade · 2023-01-26T18:35:26Z

/sponsor

openjdk · 2023-01-26T18:36:38Z

Going to push as commit fc26d3e.
Since your change was applied there have been 312 commits pushed to the master branch:

a2a7703: 8299444: java.util.Set.copyOf allocates needlessly for empty input collections
d98a323: 8301070: Replace NULL with nullptr in share/memory/
315398c: 8221785: Let possibly_parallel_threads_do cover the same threads as threads_do
14114c2: 8301005: Clean up Copy::conjoint_*_atomic on windows
973f741: 8300968: Accessorize raw oop load in DeadCounterClosure
64ddf95: 8299858: [Metrics] Swap memory limit reported incorrectly when too large
28545dc: 8300247: Harden C1 xchg on AArch64 and PPC
3f63381: 8300913: ZGC: assert(to_addr != 0) failed: Should be forwarded
da80e7a: 8300962: Parallel: Remove PSParallelCompact::_total_invocations
7725fe8: 8299953: Merge ContiguousSpaceDCTOC into DirtyCardToOopClosure
... and 302 more: https://git.openjdk.org/jdk/compare/66db0bb6a15310e4e60ff1e33d40e03c52c4eca8...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-26T18:36:55Z

@rhalade @mpdonova Pushed as commit fc26d3e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

RogerRiggs · 2023-01-26T18:50:50Z

The copyright line has the incorrect syntax. Please followup with a fix.
Missing comma, after 2023.

8299870: TLS record version check allows invalid records

8157cef

openjdk bot added the rfr Pull request is ready for review label Jan 10, 2023

openjdk bot added the security security-dev@openjdk.org label Jan 10, 2023

XueleiFan suggested changes Jan 10, 2023

View reviewed changes

rhalade reviewed Jan 10, 2023

View reviewed changes

reverted change in ProtocolVersion, updated IllegalRecordVersion

1f5330f

mpdonova added 2 commits January 12, 2023 08:11

Updated IllegalRecordVersion

c82975a

renamed IllegalRecordVersion to HandshakeWithInvalidRecordVersion

01e0a60

XueleiFan reviewed Jan 12, 2023

View reviewed changes

reverted ProtocolVersion and changed to use SSLContextTemplate

46a35f9

rhalade reviewed Jan 12, 2023

View reviewed changes

revert one more chnage

b67c9de

openjdk bot removed the rfr Pull request is ready for review label Jan 13, 2023

removed extra whitespace

13643dd

openjdk bot added the rfr Pull request is ready for review label Jan 17, 2023

mpdonova changed the title ~~8299870: TLS record version check allows invalid records~~ 8298873: Update IllegalRecordVersion.java for changes to TLS implementation Jan 26, 2023

rhalade approved these changes Jan 26, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated sponsor Pull request is ready to be sponsored labels Jan 26, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 26, 2023

openjdk bot closed this Jan 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8298873: Update IllegalRecordVersion.java for changes to TLS implementation #11929

8298873: Update IllegalRecordVersion.java for changes to TLS implementation #11929

mpdonova commented Jan 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 •

edited

Loading

XueleiFan left a comment

mpdonova commented Jan 10, 2023

XueleiFan commented Jan 10, 2023

rhalade Jan 10, 2023

rhalade Jan 19, 2023

mpdonova commented Jan 11, 2023

XueleiFan commented Jan 12, 2023

XueleiFan left a comment

XueleiFan Jan 12, 2023

XueleiFan Jan 12, 2023

rhalade Jan 26, 2023

rhalade Jan 12, 2023

rhalade commented Jan 19, 2023

mpdonova commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

rhalade commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

RogerRiggs commented Jan 26, 2023

		static final ProtocolVersion MAX_TLS_SUPPORTED = TLS13;
		static final ProtocolVersion MIN_TLS_SUPPORTED = SSL30;

8298873: Update IllegalRecordVersion.java for changes to TLS implementation #11929

8298873: Update IllegalRecordVersion.java for changes to TLS implementation #11929

Conversation

mpdonova commented Jan 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 • edited Loading

Webrevs

XueleiFan left a comment

Choose a reason for hiding this comment

mpdonova commented Jan 10, 2023

XueleiFan commented Jan 10, 2023

rhalade Jan 10, 2023

Choose a reason for hiding this comment

rhalade Jan 19, 2023

Choose a reason for hiding this comment

mpdonova commented Jan 11, 2023

XueleiFan commented Jan 12, 2023

XueleiFan left a comment

Choose a reason for hiding this comment

XueleiFan Jan 12, 2023

Choose a reason for hiding this comment

XueleiFan Jan 12, 2023

Choose a reason for hiding this comment

rhalade Jan 26, 2023

Choose a reason for hiding this comment

rhalade Jan 12, 2023

Choose a reason for hiding this comment

rhalade commented Jan 19, 2023

mpdonova commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

rhalade commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

RogerRiggs commented Jan 26, 2023

mpdonova commented Jan 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 10, 2023 •

edited

Loading