Merge pull request #3618 from benoitc/test/asgi-pipelined-smuggling-regression

fix: keep _body_receiver alive across the keepalive smuggling gate

gunicorn/asgi/protocol.py

@@ -1078,9 +1078,11 @@ class ASGIProtocol(asyncio.Protocol):
                     response_status = 500
             return False
         finally:
-            # Clear the body receiver reference
-            self._body_receiver = None
-
+            # NOTE: do NOT clear self._body_receiver here.  _handle_connection
+            # reads it after this method returns to enforce the keepalive
+            # smuggling guard (refuse keepalive when the body was not framed
+            # complete).  The connection loop clears the reference itself
+            # after the gate has run.
             try:
                 request_time = _RequestTime(time.monotonic() - request_start)
                 # Only build log data if access logging is enabled

tests/test_asgi_keepalive_smuggling.py

@@ -0,0 +1,174 @@
+#
+# This file is part of gunicorn released under the MIT license.
+# See the NOTICE for more information.
+
+"""End-to-end regression test for the keepalive smuggling guard.
+
+Drives an ``ASGIProtocol`` against a fake transport with two pipelined
+requests: the first POST advertises a Content-Length the client never
+finishes sending; the app returns a response without consuming the body.
+The protocol MUST refuse keepalive (close the transport) and MUST NOT
+parse the second request from residual body bytes.
+
+Without the fix this test surfaces, the smuggling guard added in PR #3614
+is silently bypassed because ``_handle_http_request`` clears
+``_body_receiver`` in its ``finally`` block before the connection loop's
+gate can read it.  See the commit that added this test for the fix.
+"""
+
+import asyncio
+
+import pytest
+
+from gunicorn.config import Config
+from gunicorn.asgi.protocol import ASGIProtocol
+
+
+class _FakeTransport(asyncio.Transport):
+    """Minimal asyncio.Transport stand-in that captures writes and close."""
+
+    def __init__(self):
+        super().__init__()
+        self._buffer = bytearray()
+        self.closed = False
+        self._extra = {
+            'peername': ('127.0.0.1', 12345),
+            'sockname': ('127.0.0.1', 8000),
+            'ssl_object': None,
+        }
+
+    def get_extra_info(self, name, default=None):
+        return self._extra.get(name, default)
+
+    def write(self, data):
+        if not self.closed:
+            self._buffer.extend(data)
+
+    def close(self):
+        self.closed = True
+
+    def is_closing(self):
+        return self.closed
+
+    def can_write_eof(self):
+        return False
+
+    def set_write_buffer_limits(self, high=None, low=None):
+        pass
+
+    def get_write_buffer_size(self):
+        return 0
+
+    def pause_reading(self):
+        pass
+
+    def resume_reading(self):
+        pass
+
+    @property
+    def written(self):
+        return bytes(self._buffer)
+
+
+class _Log:
+    """Minimal logger compatible with what ASGIProtocol calls."""
+
+    def debug(self, *a, **k): pass
+    def info(self, *a, **k): pass
+    def warning(self, *a, **k): pass
+    def exception(self, *a, **k): pass
+
+    @property
+    def access_log_enabled(self):
+        return False
+
+
+def _build_worker(loop, app):
+    cfg = Config()
+    cfg.set('keepalive', 2)
+    cfg.set('timeout', 30)
+    # Force the Python parser so the test does not depend on gunicorn_h1c.
+    cfg.set('http_parser', 'python')
+
+    class _W:
+        pass
+    w = _W()
+    w.cfg = cfg
+    w.loop = loop
+    w.log = _Log()
+    w.asgi = app
+    w.nr_conns = 0
+    w.nr = 0
+    w.max_requests = 1000
+    w.alive = True
+    return w
+
+
+@pytest.mark.asyncio
+async def test_keepalive_refused_when_first_body_is_partial():
+    """Two pipelined requests on the same connection.  The first POST
+    advertises Content-Length: 100 but the client only sends 10 body
+    bytes.  The app returns 200 without consuming the body.  The
+    transport MUST close instead of serving a second response from the
+    residual bytes (which would be the second request the attacker
+    pipelined behind the short body).
+    """
+
+    async def app(scope, receive, send):
+        await send({
+            "type": "http.response.start",
+            "status": 200,
+            "headers": [(b"content-length", b"2")],
+        })
+        await send({
+            "type": "http.response.body",
+            "body": b"ok",
+            "more_body": False,
+        })
+
+    loop = asyncio.get_event_loop()
+    worker = _build_worker(loop, app)
+    protocol = ASGIProtocol(worker)
+    transport = _FakeTransport()
+
+    protocol.connection_made(transport)
+
+    first_request = (
+        b"POST /first HTTP/1.1\r\n"
+        b"Host: example.com\r\n"
+        b"Content-Length: 100\r\n"
+        b"\r\n"
+        b"only-ten-b"  # 10 bytes of the promised 100
+    )
+    smuggled_second = (
+        b"GET /smuggled HTTP/1.1\r\n"
+        b"Host: example.com\r\n"
+        b"\r\n"
+    )
+
+    protocol.data_received(first_request)
+    # Give the loop a chance to run the app and emit the response.
+    for _ in range(20):
+        await asyncio.sleep(0)
+
+    # The app has answered.  Now the attacker streams what looks like a
+    # second pipelined request.  This MUST NOT be served.
+    protocol.data_received(smuggled_second)
+    for _ in range(20):
+        await asyncio.sleep(0)
+
+    response = transport.written
+    # The first response was sent.
+    assert response.startswith(b"HTTP/1.1 200"), response[:60]
+    # Only one response was written; nothing for /smuggled.
+    assert response.count(b"HTTP/1.1 ") == 1, response
+    # The transport closed: the connection refused keepalive.
+    assert transport.closed is True
+
+    # Drain the connection task cleanly.
+    if protocol._task and not protocol._task.done():
+        protocol._task.cancel()
+        try:
+            await protocol._task
+        except (asyncio.CancelledError, Exception):
+            pass