`OpenTLSSocket.close` does not conform to spec, send `close notify` alert

[jvdprng]

We should use unwrap which calls SSL_shutdown under the hood. However, I think unwrap only supports a two-way shutdown:

By the way, I believe the ssl module only supports a two-way shutdown in unwrap due to this comment by Christian Heimes in 2021, but I do not know whether this is the latest. Some related issues: https://github.com/python/cpython/issues/73599 https://github.com/python/cpython/issues/86366

Originally posted by @jvdprng in https://github.com/trailofbits/tlslib.py/issues/31#issuecomment-2059045885

[jvdprng]

After a lot of trawling through code, I think I understand what's happening, but I'm not sure how we should implement closing of the socket.

Short version:

If we call SSLSocket.unwrap, we will receive:

• ssl.SSLWantWriteError if sending the close_notify alert was not successful
• ssl.SSLWantReadError if sending the close_notify alert was successful but the peer has not yet sent a close_notify alert
• Success if the peer has already sent the close_notify alert and sending our own close_notify alert was also successful

So, I think we should unwrap before closing the socket. In case of ssl.SSLWantWriteError the user should retry closing later. In case of ssl.SSLWantReadError the user should consider the write-side of the connection closed, and can (presumably?) continue reading until the peer closes their side with a close_notify. Unfortunately, the ssl module will prevent the ssl.SSLZeroReturnError from reaching us on the recv side, so we do not actually know whether the connection is closed (unless we call unwrap again and do not get an exception, or unless we treat a None result from recv as a sign that the connection is closed (?)).

What do you think the API should look like @woodruffw @facutuesca? Should we have a separate interface for shutting down the TLS connection on the socket?

Long version:

When we call unwrap, what's happening is a bit complicated. My understanding of the underlying shutdown implementation for non-blocking sockets is as follows:

1. After some initialization checking various properties of the socket, it enters an infinite loop. For non-blocking sockets, this initialization will just set timeout to 0.
2. In this loop, it calls SSL_shutdown.
   • A return code of 1 means a successful shutdown (I think this is essentially only possible if the other side has already sent a close_notify alert). In this case it breaks out of the loop and the function ends.
   • A return code of 0 means that the close_notify alert has been sent, but the other side has not yet responded with their own. In this case, it increments a counter and continues the loop. The second time this happens within a single call to the shutdown function, it breaks out of the loop to preserve some legacy behavior (???).
   • A return code <0 indicates an error. For blocking sockets it will try to handle want read/write errors by continuing the loop until the timeout is reached, but otherwise it passes the error to the caller.

Some important notes about the code:

• If the return code is >=0, the error code should not be set due to this line, which calls this function.

• The OpenSSL docs state that

  In general, calling SSL_shutdown() in nonblocking mode will initiate the shutdown process and return 0 to indicate that the shutdown process has not yet completed. Once the shutdown process has completed, subsequent calls to SSL_shutdown() will return 1. See the RETURN VALUES section for more information.

  This suggested to me that it should return 0 every time unless there's some kind of error.

• However, the OpenSSL source code seems to say something different. The SSL_shutdown function does something weird with a function pointer, but according to this comment I think the implementation is here. The comments in that part of the code seem to support what I see in my tests: It will return -1 with error WANT_WRITE if close_notify cannot be sent, 0 if close_notify was sent (only the first time), -1 with error WANT_READ if close_notify has been sent but the corresponding alert has not been received from the peer (after the first time).

So basically this means the following: when we call unwrap, the first call to SSL_shutdown will trigger sending the close_notify alert resulting in return code 0, causing the loop to be executed again. The second call to SSL_shutdown will trigger a WANT_READ error, which ssl turns into ssl.SSLWantReadError as the result of our call to unwrap. Any future call will result in the same error until the peer also sends a close_notify alert.

I tested this with a server calling something like

(client_tlssock, address) = tls_socket.accept()
while True:
    try:
        time.sleep(0.5)
        ret_sock = client_tlssock._socket.unwrap()
        break
    except ssl.SSLWantReadError:
        pass

and then used WireShark to analyze the traffic. Basically, the first call to client_tlssock._socket.unwrap() triggers the close_notify alert, which gets acknowledged by the peer on the TCP layer, and then nothing happens until the connection is closed by the client, which triggers an exception in the server.

[jvdprng]

Solved by #42