On Fri, 25 Oct 2024 20:44:03 GMT, Xue-Lei Andrew Fan <xue...@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
