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

[Bradford Wetmore]

On Fri, 25 Oct 2024 20:44:03 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Artur Barashev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 28 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8331682
>>  - Use more appropriate exception with the alert description
>>  - Update Copyright
>>  - Update @library directive
>>  - Merge branch 'master' into JDK-8331682
>>  - Produce appropriate exception message. Update tests.
>>  - Adjust line length
>>  - Additional error checking
>>  - Write and read to/from server in a single pass. Use SocketChannel.
>>  - Return null if there is no record we attempted to decode
>>  - ... and 18 more: https://git.openjdk.org/jdk/compare/22220c1b...aef08dd0
>
> src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1866:
> 
>> 1864:                         // In TLSv1.3 alert level can be ignored, we only get the alert.
>> 1865:                         final String msg = "Unexpected plaintext alert received: "
>> 1866:                                 + Alert.nameOf(bb.get(bb.position() + 1));
> 
> . I would also check the bb size, in case of IndexOutOfBoundsException.
> . I may also dump the alert level.
> . It may be not a plaintext alert.  From the context, I don't think we are sure of it.
> 
> Considering above, I may just dump the hex bytes for the remaining bytes in bb.  For example:
>    "Unexpected alert message received [hex bytes]"

1. If we got through the `bb.remaining() <= tagSize` arm, we're not going to be encrypted.  I think we are safe here.
2. I would dump the alert level (fatal/warning) + reason.
3. `bb` size definitely needs to be checked.  e.g. if `bb.remaining() == 0`, the `bb.position() + 1` will IOOBE.

> src/java.base/share/classes/sun/security/ssl/SSLCipher.java line 1870:
> 
>> 1868:                         if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
>> 1869:                             SSLLogger.info(msg);
>> 1870:                         }
> 
> It may not need the log any longer because the follow-up exception will cover the information.

But it may not be in the same log, depending on where the `SSLLogger` is directed vs. the Exceptions.  I'd say keep it in.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1817408746
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1817386974