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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions websockify/websocketproxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,11 @@ def do_proxy(self, target):
else:
self.heartbeat = None

# see comment below - an assertion to deal with the
# intrinsics of non-blocking SSL sockets and select.
assert not isinstance(target, ssl.SSLSocket) \
or (self.buffer_size >= 16 * 1024 and not target.getblocking())

while True:
wlist = []

Expand Down Expand Up @@ -228,22 +233,43 @@ def do_proxy(self, target):
self.server.target_host, self.server.target_port)
raise self.CClose(closed['code'], closed['reason'])


if target in outs:
# Send queued client data to the target
dat = tqueue.pop(0)
sent = target.send(dat)
try:
sent = target.send(dat)
except ssl.SSLWantWriteError:
# nothing was sent - sending needs to be retried later
sent = 0

if sent == len(dat):
self.print_traffic(">")
else:
# requeue the remaining data
tqueue.insert(0, dat[sent:])
self.print_traffic(".>")


if target in ins:
# Receive target data, encode it and queue for client
buf = target.recv(self.buffer_size)
try:
# It is strictly required that buffer size is more than 16kb, so that
# all possible data can be received from a SSL socket in a single call.
# 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.


# The maximum size of a single SSL record is 16kb. And OpenSSL's SSL_read()
# will only return data from the current record - until it has been fully read.
# see also: https://www.openssl.org/docs/man1.1.1/man3/SSL_read.html
buf = target.recv(self.buffer_size)
except ssl.SSLWantReadError:
# The underlying OS socket had data, but there isn't any data to
# receive from the SSL socket yet (SSL record not yet fully received,
# only SSL control data received, e.g. re-handshake, session tickets, ...)
# Let's retry later.
continue

if len(buf) == 0:
if self.verbose:
self.log_message("%s:%s: Target closed connection",
Expand Down
4 changes: 4 additions & 0 deletions websockify/websockifyserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,10 @@ def socket(host, port=None, connect=False, prefer_ipv6=False,
sock.connect(addrs[0][4])
if use_ssl:
sock = ssl.wrap_socket(sock)
# SSL socket must be non-blocking in order to properly
# handle receiving and sending data using select.
# See also the comment(s) in ProxyRequestHandler.do_proxy()
sock.setblocking(0)
else:
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
sock.bind(addrs[0][4])
Expand Down