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

[Artur Barashev]

On Thu, 17 Oct 2024 18:48:29 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

> > > Currently, if an alter message cannot be decrypted, exception thrown. When alert is received, if it is not decryptable, treat it as an alert and close the connection accordingly, without sending more message to peers.
> > 
> > 
> > 
> > * For SSLSocket usage: aren't connection closed upstream anyhow when we throw an exception?
> 
> Yes. But what's the problem we want to address for this PR? If I read your patch correctly, you want to fix the exception for a gracefully close, right? I think you are on the right direction.
> 
> > * For SSLEngine usage: SSLEngine has no notion of connection.
> 
> SSLEngine's way is to use status and messages to guide connection. The logic should be the same as what do you in the patch.
> 
> > * For both usages: `SSLCipher` and `SSLSocketInputRecord` have no context, we don't know if we are at handshake stage or not , etc. We check for all this at SSLTransport that has the context. Besides, it's better to process the alert and finish the handshake nicely than just cut the connection.
> 
> Sorry for the confusing, we may refer connection as two different things. I meant TLS connections. Close the TLS connection could be nicely, and follow the TLS specification.
> 
> SSLCipher has contentType. If I read your patch correctly, you are creating a different Plaintext for further processes. Is it doable in SSLCipher so that you don't have to cash input record? I think you are on the right direction, I was just wondering if it is possible to simplify the implementation.

Yes, the goal is a graceful close. In TLSTransport we simply pass the plaintext alert downstream as if it was decrypted successfully. We want to address just this particular issue and not to cause any collateral damage by simplifying things. For example: if plaintext alert comes after handshake we want to throw an exception. We do several checks against the context in SSLTransport, we can't do those in SSLCipher. As for caching input record: we can pass the record upstream with `BadPaddingException` as I've suggested above.

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

PR Comment: https://git.openjdk.org/jdk/pull/21043#issuecomment-2420463236