8275811 Incorrect instance to dispose #6084

djelinski · 2021-10-22T18:24:22Z

The current code that changes cipher suites disposes the new suite instead of the old one, which usually silently fails. This patch fixes the code to dispose the old instance instead.

DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and correctly disposes the old one, and DTLSInputRecord doesn't dispose anything

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8275811: Incorrect instance to dispose

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer) ⚠️ Review applies to 60c95ee

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084
$ git checkout pull/6084

Update a local copy of the PR:
$ git checkout pull/6084
$ git pull https://git.openjdk.java.net/jdk pull/6084/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6084

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6084.diff

bridgekeeper · 2021-10-22T18:25:27Z

👋 Welcome back djelinski! 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 · 2021-10-22T18:28:50Z

@djelinski 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 · 2021-10-22T18:39:20Z

Webrevs

XueleiFan · 2021-10-22T18:45:31Z

Did you want to cover the update for line 222 at OutputRecord.java as well?

djelinski · 2021-10-22T19:21:41Z

Ah, missed that one. Fixed.

XueleiFan

Looks good to me. Thank you!

openjdk · 2021-10-22T20:01:32Z

⚠️ @djelinski the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout networkinterface-cleanup
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-10-22T20:01:34Z

@djelinski 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:

8275811: Incorrect instance to dispose

Reviewed-by: xuelei

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 172 new commits pushed to the master branch:

558ee40: 8276615: Update CR number of some tests in ProblemList-zgc.txt
603bba2: 8271420: Extend CDS custom loader support to Windows platform
ce8c767: 8276220: Reduce excessive allocations in DateTimeFormatter
0ab910d: 8276066: Reset LoopPercentProfileLimit for x86 due to suboptimal performance
f3320d2: 8276588: Change "ccc" to "CSR" in HotSpot sources
32895ac: 8275650: Problemlist java/io/File/createTempFile/SpecialTempFile.java for Windows 11
c7f070f: 8276208: vmTestbase/nsk/jdb/repeat/repeat001/repeat001.java fails with "AssertionError: Unexpected output"
684edbb: 8273922: (fs) UserDefinedFileAttributeView doesn't handle file names that are just under the MAX_PATH limit (win)
7115892: 8276401: Use blessed modifier order in java.net.http
0ef8cbe: 8276385: Re-run blessed-modifier-order script on java.desktop and jdk.accessibility
... and 162 more: https://git.openjdk.java.net/jdk/compare/46b5bfbc38f14607f0db686a42f1fa96d2f61891...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) 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).

djelinski · 2021-10-22T20:19:08Z

Thanks @XueleiFan , but I guess this needs a bit more love. Just finished running jdk_security tests, and a few tests failed, apparently related:
javax/net/ssl/SSLEngine/NoAuthClientAuth.java
javax/net/ssl/TLSv1/TLSRehandshakeTest.java
javax/net/ssl/TLSv1/TLSRehandshakeWithCipherChangeTest.java
javax/net/ssl/TLSv1/TLSRehandshakeWithDataExTest.java
javax/net/ssl/TLSv11/TLSRehandshakeTest.java
javax/net/ssl/TLSv11/TLSRehandshakeWithDataExTest.java

I'll see if I can figure this out.

djelinski · 2021-10-22T22:07:10Z

The problem is with SSLEngine; we create the ChangeCipherSpec message in a delegated task, then encrypt it later using memoized cipher:

	at java.base/sun.security.ssl.SSLCipher$T10BlockWriteCipherGenerator$BlockWriteCipher.encrypt(SSLCipher.java:1206)
	at java.base/sun.security.ssl.OutputRecord.t10Encrypt(OutputRecord.java:441)
	at java.base/sun.security.ssl.OutputRecord.encrypt(OutputRecord.java:345)
	at java.base/sun.security.ssl.SSLEngineOutputRecord$HandshakeFragment.acquireCiphertext(SSLEngineOutputRecord.java:518)
	at java.base/sun.security.ssl.SSLEngineOutputRecord.acquireCiphertext(SSLEngineOutputRecord.java:346)
	at java.base/sun.security.ssl.SSLEngineOutputRecord.encode(SSLEngineOutputRecord.java:198)

(line numbers may be off a bit)

If the memoized cipher is disposed, the operation produces incorrect output. This happens only when ChangeCipherSpec or KeyUpdate is encrypted and we're using SSLEngine.

Looks like we can move writeCipher disposal to the relevant OutputRecord subclasses. Will look into it.

djelinski · 2021-10-23T07:07:14Z

After reviewing the scope of changes to fix writeCipher disposal I decided to remove it entirely. It would probably be a nice follow-up enhancement, but I'm not confident I'd implement it correctly on the first try, so I'd rather not introduce it in a bugfix PR. @XueleiFan is that acceptable to you?

On a side note, I had another look at DTLSOutputRecord. It looks like we keep a reference to prevWriteCipher only to call dispose() during the next cipher change. I admit I don't know how often DTLS changes ciphers, but I think GC would have collected that cipher earlier if we didn't keep it.

XueleiFan · 2021-10-24T04:58:45Z

After reviewing the scope of changes to fix writeCipher disposal I decided to remove it entirely. It would probably be a nice follow-up enhancement, but I'm not confident I'd implement it correctly on the first try, so I'd rather not introduce it in a bugfix PR. @XueleiFan is that acceptable to you?

I'm not sure of the removal. Please hold on the integration, and I will have a further look if I have cycles.

djelinski · 2021-11-01T14:22:24Z

Hi @XueleiFan,
I updated the PR to dispose old write ciphers after encrypting the last record. Tests in jdk_security passed. Let me know what you think.

XueleiFan

Thank you for the update. It looks good to me, except a minor comment.

XueleiFan · 2021-11-01T19:49:32Z

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

+    // SSLEngine and SSLSocket
+    abstract void disposeWriteCipher();
+


Alternatively, this method could have a default implementation that throws UnsupportedOperationException. Then, there is no need to update DTLSOutputRecord.java.

I like that idea. Updated the PR.

djelinski · 2021-11-02T06:29:45Z

jdk_security still passes, automated checks green. @XueleiFan could you sponsor?

/integrate

openjdk · 2021-11-02T06:30:51Z

@djelinski
Your change (at version 8c9508c) is now ready to be sponsored by a Committer.

XueleiFan · 2021-11-02T23:35:51Z

Please hold off on the integration, the regression testing failed.

XueleiFan · 2021-11-02T23:43:42Z

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

@@ -422,6 +432,15 @@ void queueUpAlert(byte level, byte description) {
            handshakeMemos.add(memo);
        }

+        void queueUpCipherDispose() {
+            RecordMemo lastMemo = handshakeMemos.getLast();


Sorry, I missed that the getLast could throw exception if it is empty. I may check it before the call to getLast.

+ if (handshakeMemos.isEmpty()) { + return; + }

my mistake. Replaced with peekLast, should be better now.

Well, looks like this uncovered another, unrelated bug. handshakeMemos is only empty here when we handle TLS1.3 server hello and session ID is empty, see stack trace:

Caused by: java.util.NoSuchElementException at java.base/java.util.LinkedList.getLast(LinkedList.java:261) at java.base/sun.security.ssl.SSLEngineOutputRecord$HandshakeFragment.queueUpCipherDispose(SSLEngineOutputRecord.java:436) at java.base/sun.security.ssl.SSLEngineOutputRecord.disposeWriteCipher(SSLEngineOutputRecord.java:159) at java.base/sun.security.ssl.OutputRecord.changeWriteCiphers(OutputRecord.java:198) at java.base/sun.security.ssl.ServerHello$T13ServerHelloConsumer.consume(ServerHello.java:1372)

This is only supposed to be empty when jdk.tls.client.useCompatibilityMode is false, which it never is (also the comment above that line is copy/pasted, should be fixed). So I did some more digging and found that we do not set sessionId in clientHello when resuming TLS1.3 session:

sessionId is set to empty here

not updated here because it's TLS 1.3

not updated here because session is not null

Apparently we have no tests for TLS1.3 session resumption in jdk_security and no tests for useCompatibilityMode=false (otherwise I would have noticed this sooner).

Let me know if I should fix these issues here or in a separate PR.

djelinski · 2021-11-03T08:09:23Z

Please hold off on the integration, the regression testing failed.

could you point me to the failing test? I'm running the jdk_security suite; only sun/security/mscapi/ShortRSAKeyWithinTLS is failing, and it's failing because of environmental reasons (Windows is asking for some PIN)

EDIT
found them; started testing jdk_core and the tests surfaced pretty quickly.

XueleiFan · 2021-11-03T16:03:12Z

Please hold off on the integration, the regression testing failed.

could you point me to the failing test? I'm running the jdk_security suite; only sun/security/mscapi/ShortRSAKeyWithinTLS is failing, and it's failing because of environmental reasons (Windows is asking for some PIN)

EDIT found them; started testing jdk_core and the tests surfaced pretty quickly.

I normally run tier1 and tier2 test.

mlbridge · 2021-11-03T21:39:40Z

Mailing list message from Xuelei Fan on security-dev:

I would suggest to fix in a separate bug.

Thanks,
Xuelei

djelinski · 2021-11-04T07:30:45Z

/integrate

tier1, tier2 and jdk_core are all clean now. I think we're good to go.

A few closing thoughts:
Do PKCS11 implementations release resources when doFinal is called? Our documentation for Cipher does not mention that doFinal should be called to release resources, and our PKCS11 guide doesn't say that providers should release resources when doFinal is called.
Do we need to dispose GCM ciphers? They always call init and doFinal in matching pairs, so the extra doFinal in dispose looks superfluous.

openjdk · 2021-11-04T07:31:52Z

@djelinski
Your change (at version c18de4e) is now ready to be sponsored by a Committer.

XueleiFan · 2021-11-16T20:32:52Z

/integrate

tier1, tier2 and jdk_core are all clean now. I think we're good to go.

Thanks, I will sponsor the integration soon.

Do PKCS11 implementations release resources when doFinal is called? Our documentation for Cipher does not mention that doFinal should be called to release resources, and our PKCS11 guide doesn't say that providers should release resources when doFinal is called.

That's a pretty old and interesting topic. I don't think doFinal could really have PKCS11 release its resources. As there is not close APIs for key instance, there is not much we can do right now. Maybe, we could consider to support closable keys in the future.

Do we need to dispose GCM ciphers? They always call init and doFinal in matching pairs, so the extra doFinal in dispose looks superfluous.

It makes sense. But it may make it complicated to check different ciphers. In the long run, the doFinal() should be replaced with something like close().

XueleiFan · 2021-11-16T20:33:23Z

/sponsor

openjdk · 2021-11-16T20:34:24Z

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

b0a463f: 8169468: NoResizeEventOnDMChangeTest.java fails because FS Window didn't receive all resizes!
e5ffdf9: 8276231: ciReplay: SIGSEGV when replay compiling lambdas
d5e47d6: 8277089: Use system binutils to build hsdis
f3eb501: 8276162: Optimise unsigned comparison pattern
9a9a157: 8276905: Use appropriate macosx_version_minimum value while compiling metal shaders
7906eb0: 8277119: Add asserts in GenericTaskQueueSet methods
1c45c8a: 8274757: Cleanup unnecessary calls to Throwable.initCause() in java.management module
c06df25: 8274662: Replace 'while' cycles with iterator with enhanced-for in jdk.hotspot.agent
9629627: 8274163: Use String.equals instead of String.compareTo in jdk.jcmd
0bc2683: 8274190: Use String.equals instead of String.compareTo in jdk.internal.jvmstat
... and 329 more: https://git.openjdk.java.net/jdk/compare/46b5bfbc38f14607f0db686a42f1fa96d2f61891...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-16T20:34:44Z

@XueleiFan @djelinski Pushed as commit cddc6ce.

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

Dispose correct cipher

773c073

openjdk bot added the security security-dev@openjdk.org label Oct 22, 2021

djelinski marked this pull request as ready for review October 22, 2021 18:33

openjdk bot added the rfr Pull request is ready for review label Oct 22, 2021

Fix one more case

1ee99ae

XueleiFan approved these changes Oct 22, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2021

Do not dispose writeCipher in changeCipherSpec - we may still need it

dfc1a9e

Dispose write cipher after changing ciphers

60c95ee

XueleiFan approved these changes Nov 1, 2021

View reviewed changes

avoid modifying DTLSOutputRecord

8c9508c

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 2, 2021

XueleiFan reviewed Nov 2, 2021

View reviewed changes

bugfix

c18de4e

openjdk bot removed the sponsor Pull request is ready to be sponsored label Nov 3, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 4, 2021

openjdk bot closed this Nov 16, 2021

openjdk bot added integrated Pull request has been integrated and 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 Nov 16, 2021

djelinski deleted the networkinterface-cleanup branch November 18, 2021 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8275811 Incorrect instance to dispose #6084

8275811 Incorrect instance to dispose #6084

djelinski commented Oct 22, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 22, 2021

openjdk bot commented Oct 22, 2021

mlbridge bot commented Oct 22, 2021 •

edited

Loading

XueleiFan commented Oct 22, 2021

djelinski commented Oct 22, 2021

XueleiFan left a comment

openjdk bot commented Oct 22, 2021

openjdk bot commented Oct 22, 2021 •

edited

Loading

djelinski commented Oct 22, 2021

djelinski commented Oct 22, 2021

djelinski commented Oct 23, 2021

XueleiFan commented Oct 24, 2021 •

edited

Loading

djelinski commented Nov 1, 2021

XueleiFan left a comment

XueleiFan Nov 1, 2021

djelinski Nov 1, 2021

djelinski commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

XueleiFan commented Nov 2, 2021 •

edited

Loading

XueleiFan Nov 2, 2021

djelinski Nov 3, 2021

djelinski Nov 3, 2021

djelinski commented Nov 3, 2021 •

edited

Loading

XueleiFan commented Nov 3, 2021 •

edited

Loading

mlbridge bot commented Nov 3, 2021

djelinski commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

XueleiFan commented Nov 16, 2021

XueleiFan commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

		// SSLEngine and SSLSocket
		abstract void disposeWriteCipher();

8275811 Incorrect instance to dispose #6084

8275811 Incorrect instance to dispose #6084

Conversation

djelinski commented Oct 22, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 22, 2021

openjdk bot commented Oct 22, 2021

mlbridge bot commented Oct 22, 2021 • edited Loading

Webrevs

XueleiFan commented Oct 22, 2021

djelinski commented Oct 22, 2021

XueleiFan left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 22, 2021

openjdk bot commented Oct 22, 2021 • edited Loading

djelinski commented Oct 22, 2021

djelinski commented Oct 22, 2021

djelinski commented Oct 23, 2021

XueleiFan commented Oct 24, 2021 • edited Loading

djelinski commented Nov 1, 2021

XueleiFan left a comment

Choose a reason for hiding this comment

XueleiFan Nov 1, 2021

Choose a reason for hiding this comment

djelinski Nov 1, 2021

Choose a reason for hiding this comment

djelinski commented Nov 2, 2021

openjdk bot commented Nov 2, 2021

XueleiFan commented Nov 2, 2021 • edited Loading

XueleiFan Nov 2, 2021

Choose a reason for hiding this comment

djelinski Nov 3, 2021

Choose a reason for hiding this comment

djelinski Nov 3, 2021

Choose a reason for hiding this comment

djelinski commented Nov 3, 2021 • edited Loading

XueleiFan commented Nov 3, 2021 • edited Loading

mlbridge bot commented Nov 3, 2021

djelinski commented Nov 4, 2021

openjdk bot commented Nov 4, 2021

XueleiFan commented Nov 16, 2021

XueleiFan commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

djelinski commented Oct 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 22, 2021 •

edited

Loading

openjdk bot commented Oct 22, 2021 •

edited

Loading

XueleiFan commented Oct 24, 2021 •

edited

Loading

XueleiFan commented Nov 2, 2021 •

edited

Loading

djelinski commented Nov 3, 2021 •

edited

Loading

XueleiFan commented Nov 3, 2021 •

edited

Loading