Re: RFR: 8275811 Incorrect instance to dispose [v6]
On Wed, 3 Nov 2021 09:16:47 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > bugfix > /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(). - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
I would suggest to fix in a separate bug.
Thanks,
Xuelei
> On Nov 3, 2021, at 2:27 PM, Daniel Jeliński wrote:
>
> On Wed, 3 Nov 2021 09:12:29 GMT, Daniel Jeliński
> wrote:
>
>>> src/java.base/share/classes/sun/security/ssl/SSLEngineOutputRecord.java
>>> line 436:
>>>
434:
435: void queueUpCipherDispose() {
436: 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](https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L101)
> 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](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L406)
> - not updated
> [here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L535)
> because it's TLS 1.3
> - not updated
> [here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L577)
> 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.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
On Wed, 3 Nov 2021 09:12:29 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLEngineOutputRecord.java line
>> 436:
>>
>>> 434:
>>> 435: void queueUpCipherDispose() {
>>> 436: 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](https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L101)
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](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L406)
- not updated
[here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L535)
because it's TLS 1.3
- not updated
[here](https://github.com/openjdk/jdk/blob/04a806ec86a388b8de31d42f904c4321beb69e14/src/java.base/share/classes/sun/security/ssl/ClientHello.java#L577)
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.
-
PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v6]
On Wed, 3 Nov 2021 09:16:47 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > bugfix > > 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v6]
> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: bugfix - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/8c9508c2..c18de4e8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
On Tue, 2 Nov 2021 23:43:42 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> avoid modifying DTLSOutputRecord
>
> src/java.base/share/classes/sun/security/ssl/SSLEngineOutputRecord.java line
> 436:
>
>> 434:
>> 435: void queueUpCipherDispose() {
>> 436: 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.
-
PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
On Tue, 2 Nov 2021 23:35:51 GMT, Xue-Lei Andrew Fan wrote: > 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) - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
On Mon, 1 Nov 2021 20:38:32 GMT, Daniel Jeliński wrote:
>> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106),
>> and DTLSInputRecord [doesn't dispose
>> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57)
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> avoid modifying DTLSOutputRecord
src/java.base/share/classes/sun/security/ssl/SSLEngineOutputRecord.java line
436:
> 434:
> 435: void queueUpCipherDispose() {
> 436: 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;
+ }
-
PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
On Mon, 1 Nov 2021 20:38:32 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > avoid modifying DTLSOutputRecord Please hold on the integration, the regression testing failed. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Sun, 24 Oct 2021 04:58:45 GMT, Xue-Lei Andrew Fan wrote: >> Did you want to cover the update for line 222 at OutputRecord.java as well? > >> 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. jdk_security still passes, automated checks green. @XueleiFan could you sponsor? - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v4]
On Mon, 1 Nov 2021 19:49:32 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Dispose write cipher after changing ciphers > > src/java.base/share/classes/sun/security/ssl/OutputRecord.java line 146: > >> 144: // SSLEngine and SSLSocket >> 145: abstract void disposeWriteCipher(); >> 146: > > 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v5]
> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: avoid modifying DTLSOutputRecord - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/60c95ee3..8c9508c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=03-04 Stats: 8 lines in 2 files changed: 2 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v4]
On Mon, 1 Nov 2021 14:13:43 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Dispose write cipher after changing ciphers Thank you for the update. It looks good to me, except a minor comment. src/java.base/share/classes/sun/security/ssl/OutputRecord.java line 146: > 144: // SSLEngine and SSLSocket > 145: abstract void disposeWriteCipher(); > 146: Alternatively, this method could have a default implementation that throws UnsupportedOperationException. Then, there is no need to update DTLSOutputRecord.java. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Sun, 24 Oct 2021 04:58:45 GMT, Xue-Lei Andrew Fan wrote: >> Did you want to cover the update for line 222 at OutputRecord.java as well? > >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v4]
> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Dispose write cipher after changing ciphers - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/dfc1a9ee..60c95ee3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=02-03 Stats: 48 lines in 4 files changed: 48 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:45:31 GMT, Xue-Lei Andrew Fan wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Did you want to cover the update for line 222 at OutputRecord.java as well? > 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:45:31 GMT, Xue-Lei Andrew Fan wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Did you want to cover the update for line 222 at OutputRecord.java as well? 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106). 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v3]
> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Do not dispose writeCipher in changeCipherSpec - we may still need it - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/1ee99ae4..dfc1a9ee Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=01-02 Stats: 13 lines in 1 file changed: 0 ins; 13 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v2]
On Fri, 22 Oct 2021 19:25:38 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Fix one more case 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:45:31 GMT, Xue-Lei Andrew Fan wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Did you want to cover the update for line 222 at OutputRecord.java as well? 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. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v2]
On Fri, 22 Oct 2021 19:25:38 GMT, Daniel Jeliński wrote: >> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Fix one more case Looks good to me. Thank you! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v2]
> 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Fix one more case - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/773c073c..1ee99ae4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:24:22 GMT, Daniel Jeliński wrote: > 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Ah, missed that one. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:24:22 GMT, Daniel Jeliński wrote: > 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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Did you want to cover the update for line 222 at OutputRecord.java as well? - PR: https://git.openjdk.java.net/jdk/pull/6084
RFR: 8275811 Incorrect instance to dispose
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](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), and DTLSInputRecord [doesn't dispose anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) - Commit messages: - Dispose correct cipher Changes: https://git.openjdk.java.net/jdk/pull/6084/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6084&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275811 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
