Merge branch 'Pylons:main' into timeout-functional-tests

CHANGES.txt

@@ -1,10 +1,14 @@
-3.0.1 (unreleased)
+3.0.1 (2024-11-28)
 ------------------
 
+Security
+~~~~~~~~
+
 - Fix a bug that would lead to Waitress busy looping on select() on a half-open
   socket due to a race condition that existed when creating a new HTTPChannel.
-  See https://github.com/Pylons/waitress/pull/435 and
-  https://github.com/Pylons/waitress/issues/418
+  See https://github.com/Pylons/waitress/pull/435,
+  https://github.com/Pylons/waitress/issues/418 and
+  https://github.com/Pylons/waitress/security/advisories/GHSA-3f84-rpwh-47g6
 
   With thanks to Dylan Jay and Dieter Maurer for their extensive debugging and
   helping track this down.
@@ -13,6 +17,11 @@
   See https://github.com/Pylons/waitress/pull/434 and
   https://github.com/Pylons/waitress/issues/432
 
+- Fix a race condition in Waitress when `channel_request_lookahead` is enabled
+  that could lead to HTTP request smuggling.
+
+  See https://github.com/Pylons/waitress/security/advisories/GHSA-9298-4cf8-g4wj
+
 3.0.0 (2024-02-04)
 ------------------
 

CONTRIBUTORS.txt

@@ -148,3 +148,5 @@ Contributors
 - Frank Krick, 2018-10-29
 
 - Jonathan Vanasco, 2022-11-15
+
+- Simon King, 2024-11-12

docs/arguments.rst

@@ -314,3 +314,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

pyproject.toml

@@ -3,7 +3,7 @@ requires = ["setuptools >= 41"]
 build-backend = "setuptools.build_meta"
 
 [tool.black]
-target-version = ['py35', 'py36', 'py37', 'py38']
+target-version = ['py39', 'py310', 'py311', 'py312', 'py313']
 exclude = '''
 /(
   \.git

src/waitress/channel.py

@@ -140,7 +140,7 @@ def readable(self):
         # 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.
 
@@ -196,6 +196,15 @@ def received(self, data):
             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)
@@ -349,7 +358,7 @@ def write_soon(self, data):
                     raise ClientDisconnected
                 num_bytes = len(data)
 
-                if data.__class__ is ReadOnlyFileBasedBuffer:
+                if isinstance(data, ReadOnlyFileBasedBuffer):
                     # they used wsgi.file_wrapper
                     self.outbufs.append(data)
                     nextbuf = OverflowableBuffer(self.adj.outbuf_overflow)

src/waitress/proxy_headers.py

@@ -84,7 +84,8 @@ def raise_for_multiple_values():
         try:
             forwarded_for = []
 
-            for forward_hop in environ["HTTP_X_FORWARDED_FOR"].split(","):
+            raw_forwarded_for = environ["HTTP_X_FORWARDED_FOR"].split(",")
+            for forward_hop in raw_forwarded_for:
                 forward_hop = forward_hop.strip()
                 forward_hop = undquote(forward_hop)
 
@@ -103,6 +104,9 @@ def raise_for_multiple_values():
             client_addr = forwarded_for[0]
 
             untrusted_headers.remove("X_FORWARDED_FOR")
+            environ["HTTP_X_FORWARDED_FOR"] = ",".join(
+                raw_forwarded_for[-trusted_proxy_count:]
+            ).strip()
         except Exception as ex:
             raise MalformedProxyHeader(
                 "X-Forwarded-For", str(ex), environ["HTTP_X_FORWARDED_FOR"]
@@ -115,7 +119,8 @@ def raise_for_multiple_values():
         try:
             forwarded_host_multiple = []
 
-            for forward_host in environ["HTTP_X_FORWARDED_HOST"].split(","):
+            raw_forwarded_host = environ["HTTP_X_FORWARDED_HOST"].split(",")
+            for forward_host in raw_forwarded_host:
                 forward_host = forward_host.strip()
                 forward_host = undquote(forward_host)
                 forwarded_host_multiple.append(forward_host)
@@ -124,6 +129,9 @@ def raise_for_multiple_values():
             forwarded_host = forwarded_host_multiple[0]
 
             untrusted_headers.remove("X_FORWARDED_HOST")
+            environ["HTTP_X_FORWARDED_HOST"] = ",".join(
+                raw_forwarded_host[-trusted_proxy_count:]
+            ).strip()
         except Exception as ex:
             raise MalformedProxyHeader(
                 "X-Forwarded-Host", str(ex), environ["HTTP_X_FORWARDED_HOST"]
@@ -163,8 +171,9 @@ def raise_for_multiple_values():
     # If the Forwarded header exists, it gets priority
     if forwarded:
         proxies = []
+        raw_forwarded = forwarded.split(",")
         try:
-            for forwarded_element in forwarded.split(","):
+            for forwarded_element in raw_forwarded:
                 # Remove whitespace that may have been introduced when
                 # appending a new entry
                 forwarded_element = forwarded_element.strip()
@@ -213,6 +222,9 @@ def raise_for_multiple_values():
             raise MalformedProxyHeader("Forwarded", str(ex), environ["HTTP_FORWARDED"])
 
         proxies = proxies[-trusted_proxy_count:]
+        environ["HTTP_FORWARDED"] = ",".join(
+            raw_forwarded[-trusted_proxy_count:]
+        ).strip()
 
         # Iterate backwards and fill in some values, the oldest entry that
         # contains the information we expect is the one we use. We expect

src/waitress/runner.py

@@ -19,7 +19,7 @@
 import logging
 import os
 import os.path
-import re
+import pkgutil
 import sys
 
 from waitress import serve
@@ -179,47 +179,6 @@
 
 """
 
-RUNNER_PATTERN = re.compile(
-    r"""
-    ^
-    (?P<module>
-        [a-z_][a-z0-9_]*(?:\.[a-z_][a-z0-9_]*)*
-    )
-    :
-    (?P<object>
-        [a-z_][a-z0-9_]*(?:\.[a-z_][a-z0-9_]*)*
-    )
-    $
-    """,
-    re.I | re.X,
-)
-
-
-def match(obj_name):
-    matches = RUNNER_PATTERN.match(obj_name)
-    if not matches:
-        raise ValueError(f"Malformed application '{obj_name}'")
-    return matches.group("module"), matches.group("object")
-
-
-def resolve(module_name, object_name):
-    """Resolve a named object in a module."""
-    # We cast each segments due to an issue that has been found to manifest
-    # in Python 2.6.6, but not 2.6.8, and may affect other revisions of Python
-    # 2.6 and 2.7, whereby ``__import__`` chokes if the list passed in the
-    # ``fromlist`` argument are unicode strings rather than 8-bit strings.
-    # The error triggered is "TypeError: Item in ``fromlist '' not a string".
-    # My guess is that this was fixed by checking against ``basestring``
-    # rather than ``str`` sometime between the release of 2.6.6 and 2.6.8,
-    # but I've yet to go over the commits. I know, however, that the NEWS
-    # file makes no mention of such a change to the behaviour of
-    # ``__import__``.
-    segments = [str(segment) for segment in object_name.split(".")]
-    obj = __import__(module_name, fromlist=segments[:1])
-    for segment in segments:
-        obj = getattr(obj, segment)
-    return obj
-
 
 def show_help(stream, name, error=None):  # pragma: no cover
     if error is not None:
@@ -268,25 +227,14 @@ def run(argv=sys.argv, _serve=serve):
     if logger.level == logging.NOTSET:
         logger.setLevel(logging.INFO)
 
-    try:
-        module, obj_name = match(args[0])
-    except ValueError as exc:
-        show_help(sys.stderr, name, str(exc))
-        show_exception(sys.stderr)
-        return 1
-
     # Add the current directory onto sys.path
     sys.path.append(os.getcwd())
 
     # Get the WSGI function.
     try:
-        app = resolve(module, obj_name)
-    except ImportError:
-        show_help(sys.stderr, name, f"Bad module '{module}'")
-        show_exception(sys.stderr)
-        return 1
-    except AttributeError:
-        show_help(sys.stderr, name, f"Bad object name '{obj_name}'")
+        app = pkgutil.resolve_name(args[0])
+    except (ValueError, ImportError, AttributeError) as exc:
+        show_help(sys.stderr, name, str(exc))
         show_exception(sys.stderr)
         return 1
     if kw["call"]:

src/waitress/server.py

@@ -34,7 +34,7 @@ def create_server(
     _start=True,  # test shim
     _sock=None,  # test shim
     _dispatcher=None,  # test shim
-    **kw  # adjustments
+    **kw,  # adjustments
 ):
     """
     if __name__ == '__main__':
@@ -193,7 +193,7 @@ def __init__(
         adj=None,  # adjustments
         sockinfo=None,  # opaque object
         bind_socket=True,
-        **kw
+        **kw,
     ):
         if adj is None:
             adj = Adjustments(**kw)
@@ -387,7 +387,7 @@ def __init__(
             dispatcher=None,  # dispatcher
             adj=None,  # adjustments
             sockinfo=None,  # opaque object
-            **kw
+            **kw,
         ):
             if sockinfo is None:
                 sockinfo = (socket.AF_UNIX, socket.SOCK_STREAM, None, None)

src/waitress/task.py

@@ -13,7 +13,6 @@
 ##############################################################################
 
 from collections import deque
-import socket
 import sys
 import threading
 import time
@@ -389,7 +388,7 @@ def start_response(status, headers, exc_info=None):
 
             self.complete = True
 
-            if status.__class__ is not str:
+            if not isinstance(status, str):
                 raise AssertionError("status %s is not a string" % status)
             if "\n" in status or "\r" in status:
                 raise ValueError(
@@ -400,11 +399,11 @@ def start_response(status, headers, exc_info=None):
 
             # Prepare the headers for output
             for k, v in headers:
-                if k.__class__ is not str:
+                if not isinstance(k, str):
                     raise AssertionError(
                         f"Header name {k!r} is not a string in {(k, v)!r}"
                     )
-                if v.__class__ is not str:
+                if not isinstance(v, str):
                     raise AssertionError(
                         f"Header value {v!r} is not a string in {(k, v)!r}"
                     )
@@ -437,7 +436,7 @@ def start_response(status, headers, exc_info=None):
 
         can_close_app_iter = True
         try:
-            if app_iter.__class__ is ReadOnlyFileBasedBuffer:
+            if isinstance(app_iter, ReadOnlyFileBasedBuffer):
                 cl = self.content_length
                 size = app_iter.prepare(cl)
                 if size:

src/waitress/wasyncore.py

@@ -53,19 +53,14 @@
 
 from errno import (
     EAGAIN,
-    EALREADY,
     EBADF,
     ECONNABORTED,
     ECONNRESET,
-    EINPROGRESS,
     EINTR,
-    EINVAL,
-    EISCONN,
     ENOTCONN,
     EPIPE,
     ESHUTDOWN,
     EWOULDBLOCK,
-    errorcode,
 )
 import logging
 import os
@@ -75,7 +70,7 @@
 import time
 import warnings
 
-from . import compat, utilities
+from . import utilities
 
 _DISCONNECTED = frozenset({ECONNRESET, ENOTCONN, ESHUTDOWN, ECONNABORTED, EPIPE, EBADF})