From 6943dcf556610ece2ff3cddb39e59a05ef110661 Mon Sep 17 00:00:00 2001 From: Delta Regeer Date: Sat, 26 Oct 2024 22:10:36 -0600 Subject: [PATCH 1/4] Make DummySock() look more like an actual socket Fix CVE-2024-49768. --- docs/arguments.rst | 14 ++++++ src/waitress/channel.py | 11 ++++- tests/test_channel.py | 98 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 114 insertions(+), 9 deletions(-) diff --git a/docs/arguments.rst b/docs/arguments.rst index f9b9310..20fdfd0 100644 --- a/docs/arguments.rst +++ b/docs/arguments.rst @@ -301,3 +301,17 @@ url_prefix be stripped of the prefix. Default: ``''`` + +channel_request_lookahead + Sets the amount of requests we can continue to read from the socket, while + we are processing current requests. The default value won't allow any + lookahead, increase it above ``0`` to enable. + + When enabled this inserts a callable ``waitress.client_disconnected`` into + the environment that allows the task to check if the client disconnected + while waiting for the response at strategic points in the execution and to + cancel the operation. + + Default: ``0`` + + .. versionadded:: 2.0.0 \ No newline at end of file diff --git a/src/waitress/channel.py b/src/waitress/channel.py index 296a16a..20d0bb1 100644 --- a/src/waitress/channel.py +++ b/src/waitress/channel.py @@ -139,7 +139,7 @@ class HTTPChannel(wasyncore.dispatcher): # 1. We're not already about to close the connection. # 2. We're not waiting to flush remaining data before closing the # connection - # 3. There are not too many tasks already queued + # 3. There are not too many tasks already queued (if lookahead is enabled) # 4. There's no data in the output buffer that needs to be sent # before we potentially create a new task. @@ -194,6 +194,15 @@ class HTTPChannel(wasyncore.dispatcher): return False with self.requests_lock: + # Don't bother processing anymore data if this connection is about + # to close. This may happen if readable() returned True, on the + # main thread before the service thread set the close_when_flushed + # flag, and we read data but our service thread is attempting to + # shut down the connection due to an error. We want to make sure we + # do this while holding the request_lock so that we can't race + if self.will_close or self.close_when_flushed: + return False + while data: if self.request is None: self.request = self.parser_class(self.adj) diff --git a/tests/test_channel.py b/tests/test_channel.py index d86dbbe..bfe083a 100644 --- a/tests/test_channel.py +++ b/tests/test_channel.py @@ -18,7 +18,7 @@ class TestHTTPChannel(unittest.TestCase): map = {} inst = self._makeOne(sock, "127.0.0.1", adj, map=map) inst.outbuf_lock = DummyLock() - return inst, sock, map + return inst, sock.local(), map def test_ctor(self): inst, _, map = self._makeOneWithMap() @@ -303,7 +303,7 @@ class TestHTTPChannel(unittest.TestCase): inst.total_outbufs_len = len(inst.outbufs[0]) inst.adj.send_bytes = 1 inst.adj.outbuf_high_watermark = 2 - sock.send = lambda x: False + sock.remote.send = lambda x: False inst.will_close = False inst.last_activity = 0 result = inst.handle_write() @@ -327,7 +327,7 @@ class TestHTTPChannel(unittest.TestCase): def test__flush_some_full_outbuf_socket_returns_zero(self): inst, sock, map = self._makeOneWithMap() - sock.send = lambda x: False + sock.remote.send = lambda x: False inst.outbufs[0].append(b"abc") inst.total_outbufs_len = sum(len(x) for x in inst.outbufs) result = inst._flush_some() @@ -732,11 +732,12 @@ class TestHTTPChannelLookahead(TestHTTPChannel): ) return [body] - def _make_app_with_lookahead(self): + def _make_app_with_lookahead(self, recv_bytes=8192): """ Setup a channel with lookahead and store it and the socket in self """ adj = DummyAdjustments() + adj.recv_bytes = recv_bytes adj.channel_request_lookahead = 5 channel, sock, map = self._makeOneWithMap(adj=adj) channel.server.application = self.app_check_disconnect @@ -828,13 +829,65 @@ class TestHTTPChannelLookahead(TestHTTPChannel): self.assertEqual(data.split("\r\n")[-1], "finished") self.assertEqual(self.request_body, b"x") + def test_lookahead_bad_request_drop_extra_data(self): + """ + Send two requests, the first one being bad, split on the recv_bytes + limit, then emulate a race that could happen whereby we read data from + the socket while the service thread is cleaning up due to an error + processing the request. + """ + + invalid_request = [ + "GET / HTTP/1.1", + "Host: localhost:8080", + "Content-length: -1", + "", + ] + + invalid_request_len = len("".join([x + "\r\n" for x in invalid_request])) + + second_request = [ + "POST / HTTP/1.1", + "Host: localhost:8080", + "Content-Length: 1", + "", + "x", + ] + + full_request = invalid_request + second_request + + self._make_app_with_lookahead(recv_bytes=invalid_request_len) + self._send(*full_request) + self.channel.handle_read() + self.assertEqual(len(self.channel.requests), 1) + self.channel.server.tasks[0].service() + self.assertTrue(self.channel.close_when_flushed) + # Read all of the next request + self.channel.handle_read() + self.channel.handle_read() + # Validate that there is no more data to be read + self.assertEqual(self.sock.remote.local_sent, b"") + # Validate that we dropped the data from the second read, and did not + # create a new request + self.assertEqual(len(self.channel.requests), 0) + data = self.sock.recv(256).decode("ascii") + self.assertFalse(self.channel.readable()) + self.assertTrue(self.channel.writable()) + + # Handle the write, which will close the socket + self.channel.handle_write() + self.assertTrue(self.sock.closed) + + data = self.sock.recv(256) + self.assertEqual(len(data), 0) class DummySock: blocking = False closed = False def __init__(self): - self.sent = b"" + self.local_sent = b"" + self.remote_sent = b"" def setblocking(self, *arg): self.blocking = True @@ -852,14 +905,43 @@ class DummySock: self.closed = True def send(self, data): - self.sent += data + self.remote_sent += data return len(data) def recv(self, buffer_size): - result = self.sent[:buffer_size] - self.sent = self.sent[buffer_size:] + result = self.local_sent[:buffer_size] + self.local_sent = self.local_sent[buffer_size:] return result + def local(self): + outer = self + + class LocalDummySock: + def send(self, data): + outer.local_sent += data + return len(data) + + def recv(self, buffer_size): + result = outer.remote_sent[:buffer_size] + outer.remote_sent = outer.remote_sent[buffer_size:] + return result + + def close(self): + outer.closed = True + + @property + def sent(self): + return outer.remote_sent + + @property + def closed(self): + return outer.closed + + @property + def remote(self): + return outer + + return LocalDummySock() class DummyLock: notified = False -- 2.27.0