Can't get coverage for ssl.SSLZeroReturnError

[facutuesca]

Our OpenSSLTLSSocket::recv method handles the SSLZeroReturnError exception on socket.recv by returning zero bytes. However, I have been unable to trigger that error (in order to get coverage for (2)):

    def recv(self, bufsize: int) -> bytes:
        """Receive data from the socket. The return value is a bytes object
        representing the data received. Should not work before the handshake
        is completed."""
        try:
            return self._socket.recv(bufsize) # (1)
        except ssl.SSLZeroReturnError:
            return b"" # (2)
        except Exception:
            with _error_converter():
                raise

The ssl docs say:

exception ssl.SSLZeroReturnError
A subclass of SSLError raised when trying to read or write and the SSL connection has been closed cleanly. Note that this doesn’t mean that the underlying transport (read TCP) has been closed.

I have tried creating a test and having our ThreadedEchoServer close its client socket (closing the connection cleanly), and then attempting a recv() on the client side, but the corresponding recv() call on (1) returns b"" without raising an exception.

Following that call, it ends up in the following read() function in the ssl module (link):

    def read(self, len=1024, buffer=None):
        """Read up to LEN bytes and return them.
        Return zero-length string on EOF."""

        self._checkClosed()
        if self._sslobj is None:
            raise ValueError("Read on closed or unwrapped SSL socket.")
        try:
            if buffer is not None:
                return self._sslobj.read(len, buffer)
            else:
                return self._sslobj.read(len)
        except SSLError as x: # (3)
            if x.args[0] == SSL_ERROR_EOF and self.suppress_ragged_eofs:
                if buffer is not None:
                    return 0
                else:
                    return b''
            else:
                raise

The read call that occurs in the test mentioned above (after the server closed the corresponding socket), results in an SSLError exception handled by (3). The first if ends up being True, and b"" is returned. If, on the other hand, we set
suppress_ragged_eofs to False when wrapping the socket, an exception is re-raised, but that exception is UNEXPECTED_EOF_WHILE_READING (not SSLZeroReturnError, which is the one we're looking for).

Any ideas of why we're not getting the expected exception?
cc @jvdprng @woodruffw

[jvdprng]

I tried to look into this at some point, and I believe that this error actually does not exist anymore, but I don't remember the source of this claim.

…

On Thu, Apr 11, 2024 at 3:48 PM Facundo Tuesca ***@***.***> wrote: Our OpenSSLTLSSocket::recv method handles the SSLZeroReturnError exception on socket.recv by returning zero bytes. However, I have been unable to trigger that error (in order to get coverage for (2)): def recv(self, bufsize: int) -> bytes: """Receive data from the socket. The return value is a bytes object representing the data received. Should not work before the handshake is completed.""" try: return self._socket.recv(bufsize) # (1) except ssl.SSLZeroReturnError: return b"" # (2) except Exception: with _error_converter(): raise The ssl docs <https://docs.python.org/3/library/ssl.html#ssl.SSLZeroReturnError> say: *exception* ssl.*SSLZeroReturnError* A subclass of SSLError <https://docs.python.org/3/library/ssl.html#ssl.SSLError> raised when trying to read or write and the SSL connection has been closed cleanly. Note that this doesn’t mean that the underlying transport (read TCP) has been closed. I have tried creating a test and having our ThreadedEchoServer close its client socket (closing the connection cleanly), and then attempting a recv() on the client side, but the corresponding recv() call on (1) returns b"" without raising an exception. Following that call, it ends up in the following read() function in the ssl module (link <https://github.com/python/cpython/blob/a2ae84726b8b46e6970ae862244dad1a82cf5d19/Lib/ssl.py#L1130-L1149> ): def read(self, len=1024, buffer=None): """Read up to LEN bytes and return them. Return zero-length string on EOF.""" self._checkClosed() if self._sslobj is None: raise ValueError("Read on closed or unwrapped SSL socket.") try: if buffer is not None: return self._sslobj.read(len, buffer) else: return self._sslobj.read(len) except SSLError as x: # (3) if x.args[0] == SSL_ERROR_EOF and self.suppress_ragged_eofs: if buffer is not None: return 0 else: return b'' else: raise The read call that occurs in the test mentioned above (after the server closed the corresponding socket), results in an SSLError exception handled by (3). The first if ends up being True, and b"" is returned. If, on the other hand, we set suppress_ragged_eofs to False when wrapping the socket, an exception is re-raised, but that exception is UNEXPECTED_EOF_WHILE_READING (not SSLZeroReturnError, which is the one we're looking for). Any ideas of why we're not getting the expected exception? cc @jvdprng <https://github.com/jvdprng> @woodruffw <https://github.com/woodruffw> — Reply to this email directly, view it on GitHub <#31>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A37KBPQGYRYMPYXLPSWKTZTY42IDXAVCNFSM6AAAAABGCLCIJ2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGIZTONZXGI2DOMI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Joop van de Pol Security Consultant | Trail of Bits trailofbits.com

[jvdprng]

OK, I tried to look into the Python ssl code, and here's what I understand:
When we call close on an OpenSSLTLSSocket, it calls shutdown on the SSLSocket. This just sets the internal _sslobj to None and calls shutdown of the super (socket).

If we want to shut down the connection more gracefully, we should call unwrap on the SSLSocket, which would call shutdown on the internal _sslobj, which tries to perform a two-way shutdown using the OpenSSL interface SSL_shutdown. However, this is a two-way shutdown, which means that the other side needs to acknowledge it (or it will register an unexpected EOF).

Anyway, if the server unwrap its internal SSLSocket, then the client will still be able to recv as normal, which I think is due to these lines. On the other hand, if the client tries to send, it will receive what I thought would be a SSLZeroReturnError, but the code path is something like this line and then this case, which does not count as a SSLZeroReturnError for some reason?

[jvdprng]

Oh, it does count as SSLZeroReturnError, but this is not filtered out for send in our shim. So, I'm not sure if it is possible to get an SSLZeroReturnError in the case of a recv, unless the other side of the communication is not spec-compliant in some way?

[jvdprng]

So, to conclude, the OpenSSL docs say

If a close_notify was received, SSL_RECEIVED_SHUTDOWN will be set, for setting SSL_SENT_SHUTDOWN the application must however still call SSL_shutdown(3) or SSL_set_shutdown() itself.

Furthermore, for SSL_ERROR_ZERO_RETURN, it states

SSL_ERROR_ZERO_RETURN
The TLS/SSL peer has closed the connection for writing by sending the close_notify alert. No more data can be read. Note that SSL_ERROR_ZERO_RETURN does not necessarily indicate that the underlying transport has been closed.

This error can also appear when the option SSL_OP_IGNORE_UNEXPECTED_EOF is set. See SSL_CTX_set_options(3) for more details.

However, even with ssl.OP_IGNORE_UNEXPECTED_EOF, I don't think we will get this error, because the docs say

SSL_OP_IGNORE_UNEXPECTED_EOF

Some TLS implementations do not send the mandatory close_notify alert on shutdown. If the application tries to wait for the close_notify alert but the peer closes the connection without sending it, an error is generated. When this option is enabled the peer does not need to send the close_notify alert and a closed connection will be treated as if the close_notify alert was received.

So I think that even with non-compliant peers, it is not possible to trigger this condition in this version of the library.

[jvdprng]

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:
python/cpython#73599
python/cpython#86366

[facutuesca]

@jvdprng thanks for looking into it! I also spent some time trying to trigger it (without success), but while doing so I also realized that the _error_converter ctx manager in recv:

tlslib.py/src/tlslib/stdlib.py

Lines 379 to 384 in 4b15bd2

	def recv(self, bufsize: int) -> bytes:
	"""Receive data from the socket. The return value is a bytes object
	representing the data received. Should not work before the handshake
	is completed."""
	try:
	with _error_converter(ignore_filter=(ssl.SSLZeroReturnError,)):

is actually making it impossible to get a ssl.SSLZeroReturnError, since error_converter  will pass on any exception in ignore_filter:

tlslib.py/src/tlslib/stdlib.py

Lines 62 to 63 in 4b15bd2

	except ignore_filter:
	pass

In other words, this test passes since the exception will be ignored:

    def test_hello(self):
        with stdlib._error_converter(ignore_filter=ssl.SSLZeroReturnError):
            raise ssl.SSLZeroReturnError()

(not that this matters much, since even after removing the ignore_filter, I still could not trigger the SSLZeroReturnError exception, probably because of the reasons you mentioned above)

[jvdprng]

Yeah, I think the point of the ignore_filter is to include any exception that you want to handle yourself, so the code should actually be something like the following?

@contextmanager
def _error_converter(
    ignore_filter: tuple[type[Exception]] | tuple[()] = (),
) -> typing.Generator[None, None, None]:
    """
    Catches errors from the ssl module and wraps them up in TLSError
    exceptions. Ignores certain kinds of exceptions as requested.
    """
    try:
        yield
    except ignore_filter as e:
        raise e
    except ssl.SSLWantReadError:
        raise WantReadError("Must read data") from None
    except ssl.SSLWantWriteError:
        raise WantWriteError("Must write data") from None
    except ssl.SSLError as e:
        raise TLSError(e) from None

[jvdprng]

Although in another place the ignore_filter is used to ignore an exception:

tlslib.py/src/tlslib/stdlib.py

Lines 398 to 401 in 4b15bd2

	# NOTE: OSError indicates that the other side has already hung up.
	with _error_converter(ignore_filter=(OSError,)):
	self._socket.shutdown(socket.SHUT_RDWR)
	return self._socket.close()

So now I don't know what the original point of this ignore_filter should have been.

[facutuesca]

To me the second meaning (ignoring and not re-raising the exception) makes more sense, since I don't understand why would you want to have the first meaning (re-raising the exception in ignore_filter as-is)

[jvdprng]

Solved by #38

[jvdprng]

By playing around with the asyncio test suite, I discovered how to raise the ssl.SSLZeroReturnError exception. The following testcase will work in #42

    def test_ssl_zero_return(self):
        server, client_config = limbo_server("webpki::san::exact-localhost-ip-san")

        with server:
            client_context = stdlib.STDLIB_BACKEND.client_context(client_config)
            client_sock = client_context.connect(server.socket.getsockname())
            client_sock.send(b"message 1")
            client_sock.send(b"message 2")

            try:
                client_sock.close(False)
            except tlslib.WantReadError:
                pass

            received = 0
            with self.assertRaisesRegex(tlslib.TLSError,"TLS/SSL connection has been closed (EOF)*"):
                while received < 3:
                    try:
                        client_sock.recv(1024)
                        received += 1
                    except tlslib.WantReadError:
                        continue

I'm planning to restore handling of this error by the shim and ensuring test coverage.

[jvdprng]

Solved in 31cae84 in #42