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