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

[Daniel Jeliński]

On Fri, 20 Sep 2024 21:35:53 GMT, Artur Barashev <duke at openjdk.org> wrote:

>> https://bugs.openjdk.org/browse/JDK-8331682
>
> Artur Barashev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   - Switch server to use plaintext after getting the unexpected plaintext alert message during TLSv1.3 handshake
>   - Always send user_cancelled alert before close_notify alert during handshake. This is actually a different issue which was discovered during this fix.
>   - Update tests accordingly

I don't think we should worry too much about the server->client direction. If the client cancels the handshake, I don't expect it to handle the delayed DHE in ServerHello, which would be necessary to process the remainder of the server flight.

src/java.base/share/classes/sun/security/ssl/SSLTransport.java line 151:

> 149: 
> 150:                     // Switch server to use plaintext.
> 151:                     context.handshakeContext.conContext.outputRecord.changeWriteCiphers(

now this is plain wrong. Please revert. It won't help once you fix the test case, see my comments there.

test/jdk/javax/net/ssl/TLSv13/SSLEngineNoServerHelloClientShutdown.java line 166:

> 164:         logEngineStatus(serverEngine, serverResult);
> 165:         runDelegatedTasks(serverEngine);
> 166:         sTOc.clear();  // SH packet went missing.  Timeout on Client.

As I mentioned, TLS runs on top of TCP. Data might be truncated, but may never be received out of order To simulate this, you should never use `clear`, you can only use an alternating sequence of `flip` and `compact`.
Please remove this line.

test/jdk/javax/net/ssl/TLSv13/SSLEngineNoServerHelloClientShutdown.java line 174:

> 172:         logEngineStatus(serverEngine, serverResult);
> 173:         runDelegatedTasks(serverEngine);
> 174:         sTOc.clear();  // CCS packet went missing.  Timeout on Client.

remove this line too

test/jdk/javax/net/ssl/TLSv13/SSLEngineNoServerHelloClientShutdown.java line 182:

> 180:         logEngineStatus(serverEngine, serverResult);
> 181:         runDelegatedTasks(serverEngine);
> 182:         sTOc.clear();  // EE/etc. packet went missing.  Timeout on Client.

remove this line too

test/jdk/javax/net/ssl/TLSv13/SSLEngineNoServerHelloClientShutdown.java line 202:

> 200:         runDelegatedTasks(serverEngine);
> 201: 
> 202:         cTOs.clear();

replace with cTOs.compact()

test/jdk/javax/net/ssl/TLSv13/SSLEngineNoServerHelloClientShutdown.java line 235:

> 233:         runDelegatedTasks(clientEngine);
> 234: 
> 235:         sTOc.clear();

replace with sTOc.compact()

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

PR Review: https://git.openjdk.org/jdk/pull/21043#pullrequestreview-2319660975
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483330
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483458
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483511
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483537
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483598
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1769483652