RFR: 8331682: Slow networks/Impatient clients can potentially send unencrypted TLSv1.3 alerts that won't parse on the server [v26]

[Bradford Wetmore]

On Tue, 29 Oct 2024 19:35:54 GMT, Artur Barashev <abarashev at openjdk.org> wrote:

>> Check for unexpected plaintext alert message during TLSv1.3 handshake. This can happen if client doesn't receive ServerHello due to network timeout and tries to close the connection by sending an alert message.
>
> Artur Barashev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove logging

One minor issue, a few nits to consider, otherwise it looks reasonable to me.  

The opening of permissions for inheritance feels stylistically weird to me, but can live with it.

I'll review/sponsor it as soon as I see them.

src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1864:

> 1862:                     // Check for unexpected plaintext alert.
> 1863:                     if (contentType == ContentType.ALERT.id
> 1864:                         && bb.remaining() == 2) {

Minor nit.  `if` continuation lines should be 8 spaces.  e.g.

    if (this > that 
            && big > little) {
        doSomethingElse();
    }

([Oracle Java Code Style Guidelines](https://www.oracle.com/technetwork/java/codeconventions-150003.pdf) Section 4:  `Line wrapping for if statements should generally use the 8-space rule, since conventional (4
space) indentation makes seeing the body difficult.`)

src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1865:

> 1863:                     if (contentType == ContentType.ALERT.id
> 1864:                         && bb.remaining() == 2) {
> 1865:                         throw new GeneralSecurityException(String.format(

Ok, I am grudgingly ok with throwing a `GeneralSecurityException` here,  but only because it is then being rewrapped inside a `SSLProtocolException` in `decodeInputRecord`.  

`SSLProtocolException` isn't my first choice of exceptions, as it feels to me like it should be a `SSLHandshakeException` which is when the client/server were unable to negotiate the desired level of security for some reason.  But I see it's used throughout this class, so ok.  

>From `SSLProtocolException`

> Normally this indicates a flaw in one of the protocol implementations.

I wouldn't really consider it a flaw in one of the protocol implementations, as the implementation is only acting on the data is had available at the time it decided to shutdown.

src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1868:

> 1866:                             "Unexpected plaintext alert received: " +
> 1867:                             "Level: %s; Alert: %s",
> 1868:                             Alert.Level.nameOf(bb.get(bb.position())),

Take or leave this comment:  you could dump the raw values as well.

Unexpected plaintext alert received: Level: 1(warning); Alert: 90(user_canceled)

Again, not critical, but might be nice.

test/jdk/sun/security/ssl/SSLCipher/SSLEngineNoServerHelloClientShutdown.java line 1:

> 1: /*

As you used the templates as the basis for these tests, if you note any errors or see improvements that should be added to the templates please let's discuss and file a bug.  The template is meant to be clean enough that people writing tests can just add any-issue specific test code and everything should just work.

test/jdk/sun/security/ssl/SSLCipher/SSLEngineNoServerHelloClientShutdown.java line 67:

> 65:         "Level: warning; Alert: user_canceled";
> 66: 
> 67:     protected SSLEngine clientEngine;     // client Engine

I finally figured out why you had tweaked/removed the `final`s/`private`s protections in this file:  it's because you are having `SSLSocketNoServer...`  reuse some of this code.  It was very surprising on first read.

I probably would have just duplicated in the other file to have the test be standalone.

test/jdk/sun/security/ssl/SSLCipher/SSLSocketNoServerHelloClientShutdown.java line 52:

> 50: 
> 51: /**
> 52:  * To reproduce @bug 8331682 (client sends an unencrypted TLS alert during

We used to have a SSLSocketSSLEngineTemplate.java which did exactly this (Socket on client/Engine on Server by default, but could be switched IIRC), I wonder what happened to it.

test/lib/jdk/test/lib/security/SecurityUtils.java line 130:

> 128:     }
> 129: 
> 130:     public static void inspectTlsBuffer(ByteBuffer buffer) throws IOException {

Another take/leave minor nit:  maybe `dumpTlsPacketsBuffer` as that's all you're doing.

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

PR Review: https://git.openjdk.org/jdk/pull/21043#pullrequestreview-2410960880
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826351875
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826368022
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826370835
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826382738
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826381328
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826400074
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1826435828