diff --git a/CHANGES.txt b/CHANGES.txt index c7f32ea4..5d77a737 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -4,6 +4,21 @@ Unreleased Bugfix ~~~~~~ +- Waitress no longer closes the connection after a response with a status code + of 1xx, 204 or 304. RFC 9112 section 6.3 says such a response carries no + message body and is terminated by the first empty line after the header + fields, so it is framed unambiguously without a ``Content-Length`` or a + ``Transfer-Encoding`` and the connection may be reused. Previously every one + of them was answered with ``Connection: close``, which meant a cache + revalidating against Waitress had to reconnect for each ``304 Not Modified`` + it received. See https://github.com/Pylons/waitress/issues/197 + +- The ``Via`` response header that Waitress adds when the WSGI application + supplies its own ``Server`` header now includes the received-protocol that + RFC 9110 section 7.6.3 requires, for example ``Via: 1.1 waitress`` rather + than the ``Via: waitress`` written before, which is not a well formed field + value. + - Renamed the HTTP header "Trailers" to "Trailer" to fix a typo and comply with the correct header name as specified in RFC 7230. diff --git a/docs/reverse-proxy.rst b/docs/reverse-proxy.rst index 6490e3d7..865aea23 100644 --- a/docs/reverse-proxy.rst +++ b/docs/reverse-proxy.rst @@ -112,6 +112,39 @@ To configure waitress to use the ``Forwarded`` header, set:: contain the IP address of the proxy. +Header field names containing underscores +----------------------------------------- + +Waitress silently drops any request header field whose name contains an +underscore, and does not pass it to the WSGI application. + +An underscore is a valid character in a field name, so ``X_Forwarded_For`` is a +legal HTTP header. The problem is the CGI style mapping that the WSGI environ +uses: field names are upper-cased and dashes are replaced with underscores, so +both ``X-Forwarded-For`` and ``X_Forwarded_For`` arrive at the application as +``HTTP_X_FORWARDED_FOR``. Without this rule a client could forge a header that +the proxy in front of Waitress believes only it is able to set, which would +defeat the ``trusted_proxy`` handling described above. + +This is the same conclusion every other implementation of the CGI mapping has +reached. nginx marks such names invalid via ``underscores_in_headers off`` and +then discards them under ``ignore_invalid_headers``; Apache httpd has dropped +them when building CGI variables since 2.4, as has mod_wsgi since 4.3.0; +Werkzeug's development server skips them unconditionally; and gunicorn's +``header_map`` setting documents ``drop`` as its safe default. Django applied +the same rule at the framework level in CVE-2015-0219, which is where this +class of bug was first described. + +Dropping the field rather than rejecting the whole request is deliberate. +Discarding it already removes the ambiguity that makes the header spoofable, so +refusing the request buys no further protection while risking legitimate +traffic — an underscore is a valid character in a field name, and rejecting +outright is itself hazardous in front of pipelining or proxies. + +If you need such a header to reach your application, rename it at the proxy to +use dashes instead. + + Using ``url_prefix`` to influence ``SCRIPT_NAME`` and ``PATH_INFO`` ------------------------------------------------------------------- diff --git a/src/waitress/parser.py b/src/waitress/parser.py index 1af4594d..d0ab077f 100644 --- a/src/waitress/parser.py +++ b/src/waitress/parser.py @@ -232,7 +232,19 @@ def parse_header(self, header_plus): key, value = header.group("name", "value") if b"_" in key: - # TODO(xistence): Should we drop this request instead? + # An underscore is a valid tchar, so this is a legal field + # name, but the CGI style mapping the WSGI environ uses is + # ambiguous for it: both "X-Forwarded-For" and + # "X_Forwarded_For" become HTTP_X_FORWARDED_FOR, so a client + # could otherwise forge a header that a proxy in front of us + # believes only it can set. + # + # Dropping the field rather than the request is what nginx + # (underscores_in_headers off, then ignore_invalid_headers), + # Apache httpd, mod_wsgi, Werkzeug and gunicorn (header_map, + # which documents "drop" as the safe default) all do. Dropping + # is enough to remove the ambiguity, so rejecting the request + # outright would buy nothing and only risk legitimate traffic. continue diff --git a/src/waitress/task.py b/src/waitress/task.py index bda3ae50..beecd694 100644 --- a/src/waitress/task.py +++ b/src/waitress/task.py @@ -243,15 +243,23 @@ def build_response_header(self): self.set_close_on_finish() if not content_length_header: - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx, 204 or 304. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, + # 204 or 304. + # + # Such a response carries no message body and is terminated by + # the first empty line after the header fields, so it is + # already framed unambiguously and the connection may be + # reused. Only a response that should have carried a body but + # has no length to declare needs the connection closed in + # order to delimit it. if self.has_body: self.response_headers.append(("Transfer-Encoding", "chunked")) self.chunked_response = True - if not self.close_on_finish: - self.set_close_on_finish() + if not self.close_on_finish: + self.set_close_on_finish() # under HTTP 1.1 keep-alive is default, no need to set the header else: @@ -265,7 +273,12 @@ def build_response_header(self): if ident: self.response_headers.append(("Server", ident)) else: - self.response_headers.append(("Via", ident or "waitress")) + # RFC 9110 section 7.6.3: Via = #( received-protocol RWS + # received-by [ RWS comment ] ). The received-protocol is not + # optional, so the pseudonym on its own is not a well formed field + # value; the protocol-name may only be elided when it is HTTP, + # which leaves the version of the request we received. + self.response_headers.append(("Via", f"{version} {ident or 'waitress'}")) if not date_header: self.response_headers.append(("Date", build_http_date(self.start_time))) diff --git a/tests/fixtureapps/notmodified.py b/tests/fixtureapps/notmodified.py new file mode 100644 index 00000000..1c53fd5e --- /dev/null +++ b/tests/fixtureapps/notmodified.py @@ -0,0 +1,7 @@ +def app(environ, start_response): # pragma: no cover + if environ["PATH_INFO"] == "/nocontent": + start_response("204 No Content", []) + else: + start_response("304 Not Modified", [("ETag", '"abc"')]) + + return [b""] diff --git a/tests/test_functional.py b/tests/test_functional.py index fae412c5..07e6a5d5 100644 --- a/tests/test_functional.py +++ b/tests/test_functional.py @@ -593,6 +593,42 @@ def test_pipelining(self): self.assertEqual(response_body, expect_body) +class NotModifiedTests: + def setUp(self): + from tests.fixtureapps import notmodified + + self.start_subprocess(notmodified.app) + + def tearDown(self): + self.stop_subprocess() + + def _check_bodiless_keepalive(self, path, status, reason): + # RFC 9112 section 6.3: a response with a 1xx, 204 or 304 status + # carries no message body and is terminated by the first empty line + # after the header fields. It is therefore framed unambiguously + # without a Content-Length or a Transfer-Encoding, and the connection + # may be reused; a cache revalidating against us should not have to + # reconnect for every 304 it gets back. + to_send = b"GET %s HTTP/1.1\r\nHost: localhost\r\n\r\n" % path + self.connect() + self.sock.send(to_send * 2) + + with self.sock.makefile("rb", 0) as fp: + for _ in range(2): + line = fp.readline() # status line + self.assertline(line, status, reason, "HTTP/1.1") + headers = parse_headers(fp) + self.assertNotIn("connection", headers) + self.assertNotIn("content-length", headers) + self.assertNotIn("transfer-encoding", headers) + + def test_304_does_not_close_connection(self): + self._check_bodiless_keepalive(b"/", "304", "Not Modified") + + def test_204_does_not_close_connection(self): + self._check_bodiless_keepalive(b"/nocontent", "204", "No Content") + + class ExpectContinueTests: def setUp(self): from tests.fixtureapps import echo @@ -1566,6 +1602,10 @@ class TcpPipeliningTests(PipeliningTests, TcpTests, unittest.TestCase): pass +class TcpNotModifiedTests(NotModifiedTests, TcpTests, unittest.TestCase): + pass + + class TcpExpectContinueTests(ExpectContinueTests, TcpTests, unittest.TestCase): pass @@ -1641,6 +1681,9 @@ class UnixEchoTests(EchoTests, UnixTests, unittest.TestCase): class UnixPipeliningTests(PipeliningTests, UnixTests, unittest.TestCase): pass + class UnixNotModifiedTests(NotModifiedTests, UnixTests, unittest.TestCase): + pass + class UnixExpectContinueTests(ExpectContinueTests, UnixTests, unittest.TestCase): pass diff --git a/tests/test_task.py b/tests/test_task.py index 278ab538..aac56cb7 100644 --- a/tests/test_task.py +++ b/tests/test_task.py @@ -203,57 +203,79 @@ def test_build_response_header_v11_200_no_content_length(self): self.assertIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_204_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx or 204. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "204 No Content" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 204 No Content") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") - self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_1xx_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx or 204. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "100 Continue" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 100 Continue") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") - self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) def test_build_response_header_v11_304_no_content_length_or_transfer_encoding(self): - # RFC 7230: MUST NOT send Transfer-Encoding or Content-Length - # for any response with a status code of 1xx, 204 or 304. + # RFC 9112 section 6.3: MUST NOT send Transfer-Encoding or + # Content-Length for any response with a status code of 1xx, 204 or + # 304. Such a response has no message body to delimit, so the + # connection may be reused; a 304 is the answer to a cache + # revalidation and closing on every one of them is expensive. inst = self._makeOne() inst.request = DummyParser() inst.version = "1.1" inst.status = "304 Not Modified" result = inst.build_response_header() lines = filter_lines(result) - self.assertEqual(len(lines), 4) + self.assertEqual(len(lines), 3) self.assertEqual(lines[0], b"HTTP/1.1 304 Not Modified") - self.assertEqual(lines[1], b"Connection: close") - self.assertTrue(lines[2].startswith(b"Date:")) - self.assertEqual(lines[3], b"Server: waitress") + self.assertTrue(lines[1].startswith(b"Date:")) + self.assertEqual(lines[2], b"Server: waitress") + self.assertFalse(inst.close_on_finish) + self.assertNotIn(("Connection", "close"), inst.response_headers) + + def test_build_response_header_v11_no_content_length_chunked_still_closes(self): + # A response that should have carried a body but has no length to + # declare is delimited by the chunked coding, and Waitress closes the + # connection afterwards; that behaviour is unchanged. + inst = self._makeOne() + inst.request = DummyParser() + inst.version = "1.1" + inst.status = "200 OK" + result = inst.build_response_header() + lines = filter_lines(result) + self.assertEqual(lines[0], b"HTTP/1.1 200 OK") + self.assertIn(b"Connection: close", lines) + self.assertIn(b"Transfer-Encoding: chunked", lines) self.assertTrue(inst.close_on_finish) - self.assertIn(("Connection", "close"), inst.response_headers) + self.assertTrue(inst.chunked_response) def test_build_response_header_via_added(self): + # RFC 9110 section 7.6.3: the received-protocol is a required part of + # a Via field value, the pseudonym alone is not well formed inst = self._makeOne() inst.request = DummyParser() inst.version = "1.0" @@ -265,7 +287,18 @@ def test_build_response_header_via_added(self): self.assertEqual(lines[1], b"Connection: close") self.assertTrue(lines[2].startswith(b"Date:")) self.assertEqual(lines[3], b"Server: abc") - self.assertEqual(lines[4], b"Via: waitress") + self.assertEqual(lines[4], b"Via: 1.0 waitress") + + def test_build_response_header_via_uses_request_version(self): + inst = self._makeOne() + inst.request = DummyParser() + inst.version = "1.1" + inst.status = "200 OK" + inst.content_length = 0 + inst.response_headers = [("Server", "abc")] + result = inst.build_response_header() + lines = filter_lines(result) + self.assertIn(b"Via: 1.1 waitress", lines) def test_build_response_header_date_exists(self): inst = self._makeOne()