Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlocks due to invalid handling of SSL sockets and select #504

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

curdbecker
Copy link

SSL sockets can't be used with select() in the same way raw sockets would, since the OS socket readable/writable state is not always identical to the one of the SSL stack. See also:
https://docs.python.org/3/library/ssl.html#notes-on-non-blocking-sockets

For instance, websockify is currently unable to proxy a connection to a SSL target - unless the server-side sends data first.

In this case, the OS socket is reported as readable - due to SSL protocol data like sessions tickets being exchanged - while the SSL socket is not readable since application data hasn't been received from the server-side yet. As SSL sockets are read from (and written to) in a blocking fashion, the current implementation then simply stalls. For an example, see here:

Screen.Recording.2022-01-08.at.19.14.27.3.mov

The following patch tries to leave most of the existing socket handling logic intact while addressing the special handling required for SSL sockets. In particular, SSL sockets are now put in non-blocking mode, so that false-positive readable reports from select() don't lead to a deadlock but an exception. (Unfortunately, the SSLSocket.pending() API doesn't seem to work reliably to archive the same with blocking SSL sockets.. :-/)

In addition, the patch introduces an assertion to ensure the internal buffer size is equal or above 16kb so that all available data can be directly read from a SSL socket in a single call. Otherwise, there would be additional (annoying) handling required to make sure no data is accidentally left behind when a SSL socket would be only partially read. In this case, select() wouldn't report this socket as readable again - unless new data is received - since the rest of the data has already been read and buffered by the SSL socket layer.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I don't think this is a common use case, so this is probably been overlooked for some time.

This looks mostly correct, except for a few notes.

websockify/websocketproxy.py Outdated Show resolved Hide resolved
# Otherwise, there could be still data available for reading, but select
# wouldn't report the socket as readable again, since all data has already
# been read by the SSL socket from the OS.
# see also: https://docs.python.org/3/library/ssl.html#notes-on-non-blocking-sockets
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it guarantees to give you everything in a single recv(). For one thing, there might be multiple frames buffered. So you need to do what that link specifies and continue calling recv() until it is drained. You have SSLSocket.pending() to help you out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. That's the good thing. Let me quote the documentation on OpenSSL's SSL_read():

The read functions work based on the SSL/TLS records. The data are received in records (with a maximum record size of 16kB). Only when a record has been completely received, can it be processed (decryption and check of integrity). Therefore, data that was not retrieved at the last read call can still be buffered inside the SSL layer and will be retrieved on the next read call. If num is higher than the number of bytes buffered then the read functions will return with the bytes buffered. If no more bytes are in the buffer, the read functions will trigger the processing of the next record. Only when the record has been received and processed completely will the read functions return reporting success. At most the contents of one record will be returned.

And unfortunately, SSLSocket.pending() doesn't seem to work at all on some platforms. For me, it always returns zero regardless if there's something pending or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I read that text it supports my initial assertion. If there are multiple records buffered then it states that only the first record will be returned. So you need to call it again.

@CendioOssman CendioOssman added the bug Something isn't working label Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants