mirror of
https://github.com/frappe/gunicorn.git
synced 2026-07-02 10:41:30 +08:00
fix: keep _body_receiver alive across the keepalive smuggling gate
The smuggling guard added in #3614 reads self._body_receiver after _handle_http_request returns to refuse keepalive on a body that did not finish framing. But _handle_http_request's finally cleared the receiver before returning, so the gate always saw None and let keepalive proceed unconditionally. Move the clear into the connection loop's per-iteration cleanup (it already had one there for the same purpose). Adds an end-to-end regression test that pipelines a partial-body POST followed by a smuggled GET and asserts the second request is not served and the transport closes.
This commit is contained in:
parent
8d9d9030ff
commit
0a736ea4a2
@ -1078,9 +1078,11 @@ class ASGIProtocol(asyncio.Protocol):
|
|||||||
response_status = 500
|
response_status = 500
|
||||||
return False
|
return False
|
||||||
finally:
|
finally:
|
||||||
# Clear the body receiver reference
|
# NOTE: do NOT clear self._body_receiver here. _handle_connection
|
||||||
self._body_receiver = None
|
# 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:
|
try:
|
||||||
request_time = _RequestTime(time.monotonic() - request_start)
|
request_time = _RequestTime(time.monotonic() - request_start)
|
||||||
# Only build log data if access logging is enabled
|
# Only build log data if access logging is enabled
|
||||||
|
|||||||
174
tests/test_asgi_keepalive_smuggling.py
Normal file
174
tests/test_asgi_keepalive_smuggling.py
Normal file
@ -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
|
||||||
Loading…
x
Reference in New Issue
Block a user