RFR: 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears to not be fully fixed [v2]

[Clive Verghese]

On Thu, 7 Jan 2021 23:39:34 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Clive Verghese has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Shutdown sockets to prevent leak
>
> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1678:
> 
>> 1676:         // Don't close the Socket in case of timeouts, interrupts or SocketException.
>> 1677:         if (cause instanceof InterruptedIOException ||
>> 1678:                 cause instanceof SocketException) {
> 
> Maybe we still need to shutdown the connection with a fatal alter for socket exception, otherwise there might be socket leaks.  Instead, the socket exception could be thrown after the fatal alert.

Updated to shutdown the socket in case of a SocketException.

> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 452:
> 
>> 450:             } catch (SocketException se) {
>> 451:                 // don't change exception in case of SocketException
>> 452:                 throw se;
> 
> Maybe, the fatal alter could be sent before thrown the socket exception.

The client is sending the fatal, 

However, the server, since it's producing the message, It's not reading from the socket to see that the client sent the `bad_certificate` 

SERVER                                                                  CLIENT
*                                <------------                       CLIENT_HELLO
CLIENT_HELLO_CONSUMER
SERVER_HELLO_PRODUCER            ------------->                     SERVER_HELLO_CONSUMER
CERTIFICATE_PRODUCER             ------------->                     CERTIFICATE_CONSUMER
CERTIFICATE_STATUS               ------------->                     Still in CERTIFICATE_CONSUMER
START SERVER_KEY_EXCHANGE
*                                <-------------                    CERTIFICATE_CONSUMER sends bad_certificate alert
*                                <-------------                    CLIENT_CLOSES_SOCKET
SERVER_KEY_EXCHANGE
attempts to write to socket      --------||||
(broken_pipe exception)

Server throws a SocketException(broken_pipe) exception instead of SSLException(bad_certificate) or SSLHandshakeException(bad_certificate)

When in the producer, the server does not read from the socket, and hence does not process the bad_certificate alert from the client

The SERVER_KEY_EXCHANGE produce then attempts to write to the socket, which encounters the broken pipe. 

We could, in the SSLSocketImpl::handleException, attempt to check if there is a message available in the socket. If so, read the message and throw the appropriate exception. 

I could open a follow up JBS issue to address this.

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

PR: https://git.openjdk.java.net/jdk/pull/1968