RFR: 8350830: Values converted incorrectly when reading TLS session tickets [v2]

[Mikhail Yankelevich]

On Wed, 9 Apr 2025 12:57:06 GMT, Nibedita Jena <duke at openjdk.org> wrote:

>> Session resumption without server side state was added under [JDK-8211018](https://bugs.openjdk.org/browse/JDK-8211018).
>> While it is TLSv1.2 session resumption, the client hello message is being parsed in SSLSessionImpl for each extensions.
>> 
>> Customer has reported handshake failure and is reproducible locally with exception NegativeArraySizeExceptions when there is ServerNameIndication with size > 127.
>> According to RFC 3546, the host_name limit allowed is 255.
>> With a sample testcase when the host_name length is > 127, exception is thrown:
>> javax.net.ssl|DEBUG|71|Thread-1|2025-04-06 17:13:07.278 UTC|ClientHello.java:825|Negotiated protocol version: TLSv1.2
>> javax.net.ssl|WARNING|71|Thread-1|2025-04-06 17:13:07.281 UTC|SSLSocketImpl.java:1672|handling exception (
>> "throwable" : {
>>   java.lang.NegativeArraySizeException: -1
>>         at java.base/sun.security.ssl.SSLSessionImpl.<init>(SSLSessionImpl.java:399)
>>         at java.base/sun.security.ssl.SessionTicketExtension$T12CHSessionTicketConsumer.consume(SessionTicketExtension.java:468)
>> 
>> e.g.
>> int l = buf.get();
>> b = new byte[l];  <-------------------- NegativeArraySizeException thrown here when > 127
>> 
>> For TLSv1.3, its not an issue until length > 255.
>> 
>> According to RFC 5077, PSK identity length allowed is <0..2^16-1> and so its value conversion being taken care of under this change.
>> Master secret is allowed for 48 bytes - master_secret[48], shouldnt be an issue.
>
> Nibedita Jena has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Minor change

test/jdk/sun/security/ssl/SSLSessionImpl/ResumeClientTLS12withSNI.java line 33:

> 31: import javax.net.ssl.*;
> 32: import javax.net.ssl.SSLEngineResult.HandshakeStatus;
> 33: import java.io.*;

Nitpick: wildcard import

test/jdk/sun/security/ssl/SSLSessionImpl/ResumeClientTLS12withSNI.java line 44:

> 42:      * Enables logging of the SSLEngine operations.
> 43:      */
> 44:     private static final boolean logging = false;

I strongly believe logging should be on at all times, no logging will make debugging challenging especially if the issue is intermittent.

test/jdk/sun/security/ssl/SSLSessionImpl/ResumeClientTLS12withSNI.java line 55:

> 53:      * after gaining some familiarity with this application.
> 54:      */
> 55:     private static final boolean debug = true;

Could you please get the debug from the `private static final boolean debug =
         Boolean.parseBoolean(System.getProperty("test.debug", "true"));` or `private static final boolean debug =
         Boolean.getBoolean("test.debug");` ? 

Also, why is the `debug` is set to true by default? It makes more sense to remove it all together imo and just log everything, with this off it will be very hard to see what's wrong.

test/jdk/sun/security/ssl/SSLSessionImpl/ResumeClientTLS12withSNI.java line 98:

> 96:      */
> 97:     public static void main(String args[]) throws Exception {
> 98:         if (debug) {

If you remove the debug, this can be removed and the property could be set at `@run` on line 26. I think this will also remove the need for othervm.

test/jdk/sun/security/ssl/SSLSessionImpl/ResumeClientTLS12withSNI.java line 361:

> 359:         if (resultOnce) {
> 360:             resultOnce = false;
> 361:             System.out.println("The format of the SSLEngineResult is: \n" +

Why is this needed? Wouldn't it be more appropriate to have it outside the logging and just print it out as a description?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2035405118
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2035398129
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2035180058
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2035469081
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2035473407