RFR: 8350830: Values converted incorrectly when reading TLS session tickets

[Daniel Jeliński]

On Wed, 9 Apr 2025 03:28:40 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.

Changes requested by djelinski (Reviewer).

src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 359:

> 357: 
> 358:         // PSK identity
> 359:         i = Byte.toUnsignedInt(buf.get());

Could you rewrite this constructor to use the helper methods from `sun.security.ssl.Record` instead of reading data from the buffer directly?

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

> 113:                 passphrase);
> 114: 
> 115:         SSLContext sslCtx = SSLContext.getInstance(sslProtocol);

Use SSLContextTemplate methods to generate the context, if possible

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

> 266:         clientEngine.setUseClientMode(true);
> 267:         SSLParameters cliSSLParams = clientEngine.getSSLParameters();
> 268:         cliSSLParams.setServerNames(List.of(SNI_NAME));

you could also use `SNI_NAME` as the host parameter of the `createSSLEngine` method above; this way you wouldn't need to set it here.

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

> 390:             System.out.println(this.sslc.getProvider().getName() + " " + this.sslc.getProtocol() + " - Session resumption SUCCEEDED");
> 391:         } else {
> 392:             System.out.println(this.sslc.getProvider().getName() + " " + this.sslc.getProtocol() + " - Session resumption FAILED");

should the test fail if this happens? Either change this to throw an exception, or remove this method.

test/jdk/sun/security/ssl/SSLSessionImpl/keystore_san.p12 line 1:

> 1: 0��
0��	*�H��

Can you use the SSLContextTemplate class instead?
If not, please add a readme describing how this file can be regenerated.

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

PR Review: https://git.openjdk.org/jdk/pull/24535#pullrequestreview-2752317840
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2034632391
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2034639617
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2034645788
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2034654626
PR Review Comment: https://git.openjdk.org/jdk/pull/24535#discussion_r2034624226