Fix three RFC 9112 response framing and header issues

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.
 

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``
 -------------------------------------------------------------------
 

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
 

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)))

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""]

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
 

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()