RFR: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93) [v3]
Xue-Lei Andrew Fan
xuelei at openjdk.java.net
Sat May 8 00:26:29 UTC 2021
On Fri, 7 May 2021 12:01:16 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:
>> Hello All,
>>
>> Could you please review the fix for the JDK-8241248?
>> The issue happens during the TLSv1.3 handshake without server stateless session resumption in case of server receives several parallel requests with the same pre_shared_key.
>> The main idea of the fix is to remove resuming session from the session cache in the early stage.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8241248
>> Webrev 8u: http://cr.openjdk.java.net/~abakhtin/8241248/webrev.v0/
>>
>> The test from the bug report using OpenSSL is passed ( -Djdk.tls.server.enableSessionTicketExtension=false )
>> javax/net/ssl and sun/security/ssl jtreg tests passed
>>
>> Regards
>> Alexey
>
> Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision:
>
> Add Cache.pull method
Thank you for take my comments. The code looks nice to me, except a few trivial comments.
src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line 377:
> 375: // If we are keeping state, see if the identity is in the cache
> 376: if (requestedId.identity.length == SessionId.MAX_LENGTH) {
> 377: s = sessionCache.pull(requestedId.identity);
Would you please add a comment here so as we know why a pull method could be used here? For example:
- // If we are keeping state, see if the identity is in the cache
+ // If we are keeping state, see if the identity is in the
+ // cache. Note that for TLS 1.3, we would also clean
+ // up the cached session if it is not rejoinable.
src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line 183:
> 181: if (id != null)
> 182: return sessionCache.pull(new SessionId(id));
> 183: return null;
As this is an internal method, it should be safe to assume that the id is non-null. I'm fine if you want to keep the non-null check, but please use braces for if-statement (see also https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449).
src/java.base/share/classes/sun/security/util/Cache.java line 430:
> 428: return null;
> 429: }
> 430: V value;
I may add a blank line before this line.
src/java.base/share/classes/sun/security/util/Cache.java line 442:
> 440: entry.invalidate();
> 441: return value;
> 442: }
I may adjust the lines a little bit so as to avoid duplicated operations (see the implementation code of isValid()).
long time = (lifetime == 0) ? 0 : System.currentTimeMillis();
if (entry.isValid(time)) {
V value = entry.getValue();
entry.invalidate();
return value;
} else {
if (DEBUG) {
System.out.println("Ignoring expired entry");
}
return null;
}
-------------
PR: https://git.openjdk.java.net/jdk/pull/3664
More information about the security-dev
mailing list