Improve validation of Transfer-Encoding

Waitress only supports a single Transfer-Encoding and that is chunked.
We will read the whole request into a temporary buffer and then remove
the header and set the Content-Length.

However HTTP desync/HTTP request smuggling attacks could potentially
provide multiple Transfer-Encoding headers that would not get
appropriately treated by waitress.

Waitress now treats the header as potentially containing multiple
values, and validates that the last encoding listed is "chunked".

At this time Waitress does not support any other encodings, and all
other requests will be rejected with a 501 Not Implemented error.

3 files changed, 81 insertions, 1 deletions

diff --git a/waitress/parser.py b/waitress/parser.py
index c537964..dd591f2 100644
--- a/waitress/parser.py
+++ b/waitress/parser.py
@@ -26,6 +26,7 @@ from waitress.utilities import (
     BadRequest,
     RequestEntityTooLarge,
     RequestHeaderFieldsTooLarge,
+    ServerNotImplemented,
     find_double_newline,
 )
 
@@ -34,6 +35,10 @@ class ParsingError(Exception):
     pass
 
 
+class TransferEncodingNotImplemented(Exception):
+    pass
+
+
 class HTTPRequestParser(object):
     """A structure that collects the HTTP request.
 
@@ -126,31 +131,39 @@ class HTTPRequestParser(object):
                     except ParsingError as e:
                         self.error = BadRequest(e.args[0])
                         self.completed = True
+                    except TransferEncodingNotImplemented as e:
+                        self.error = ServerNotImplemented(e.args[0])
+                        self.completed = True
                     else:
                         if self.body_rcv is None:
                             # no content-length header and not a t-e: chunked
                             # request
                             self.completed = True
+
                         if self.content_length > 0:
                             max_body = self.adj.max_request_body_size
                             # we won't accept this request if the content-length
                             # is too large
+
                             if self.content_length >= max_body:
                                 self.error = RequestEntityTooLarge(
                                     "exceeds max_body of %s" % max_body
                                 )
                                 self.completed = True
                 self.headers_finished = True
+
                 return consumed
 
             # Header not finished yet.
             self.header_plus = s
+
             return datalen
         else:
             # In body.
             consumed = br.received(data)
             self.body_bytes_received += consumed
             max_body = self.adj.max_request_body_size
+
             if self.body_bytes_received >= max_body:
                 # this will only be raised during t-e: chunked requests
                 self.error = RequestEntityTooLarge("exceeds max_body of %s" % max_body)
@@ -162,6 +175,7 @@ class HTTPRequestParser(object):
             elif br.completed:
                 # The request (with the body) is ready to use.
                 self.completed = True
+
                 if self.chunked:
                     # We've converted the chunked transfer encoding request
                     # body into a normal request body, so we know its content
@@ -241,10 +255,31 @@ class HTTPRequestParser(object):
             # should not see the HTTP_TRANSFER_ENCODING header; we pop it
             # here
             te = headers.pop("TRANSFER_ENCODING", "")
-            if te.lower() == "chunked":
+
+            encodings = [encoding.strip().lower() for encoding in te.split(",") if encoding]
+
+            for encoding in encodings:
+                # Out of the transfer-codings listed in
+                # https://tools.ietf.org/html/rfc7230#section-4 we only support
+                # chunked at this time.
+
+                # Note: the identity transfer-coding was removed in RFC7230:
+                # https://tools.ietf.org/html/rfc7230#appendix-A.2 and is thus
+                # not supported
+                if encoding not in {"chunked"}:
+                    raise TransferEncodingNotImplemented(
+                        "Transfer-Encoding requested is not supported."
+                    )
+
+            if encodings and encodings[-1] == "chunked":
                 self.chunked = True
                 buf = OverflowableBuffer(self.adj.inbuf_overflow)
                 self.body_rcv = ChunkedReceiver(buf)
+            elif encodings:  # pragma: nocover
+                raise TransferEncodingNotImplemented(
+                    "Transfer-Encoding requested is not supported."
+                )
+
             expect = headers.get("EXPECT", "").lower()
             self.expect_continue = expect == "100-continue"
             if connection.lower() == "close":
diff --git a/waitress/tests/test_parser.py b/waitress/tests/test_parser.py
index aa01d9d..1a95e23 100644
--- a/waitress/tests/test_parser.py
+++ b/waitress/tests/test_parser.py
@@ -53,6 +53,21 @@ class TestHTTPRequestParser(unittest.TestCase):
         self.assertTrue(self.parser.completed)
         self.assertEqual(self.parser.error.__class__, BadRequest)
 
+    def test_received_bad_transfer_encoding(self):
+        from waitress.utilities import ServerNotImplemented
+        data = (
+            b"GET /foobar HTTP/1.1\r\n"
+            b"Transfer-Encoding: foo\r\n"
+            b"\r\n"
+            b"1d;\r\n"
+            b"This string has 29 characters\r\n"
+            b"0\r\n\r\n"
+        )
+        result = self.parser.received(data)
+        self.assertEqual(result, 48)
+        self.assertTrue(self.parser.completed)
+        self.assertEqual(self.parser.error.__class__, ServerNotImplemented)
+
     def test_received_nonsense_nothing(self):
         data = b"\r\n\r\n"
         result = self.parser.received(data)
@@ -196,6 +211,31 @@ class TestHTTPRequestParser(unittest.TestCase):
         self.parser.parse_header(data)
         self.assertEqual(self.parser.body_rcv.__class__.__name__, "ChunkedReceiver")
 
+
+    def test_parse_header_transfer_encoding_invalid(self):
+        from waitress.parser import TransferEncodingNotImplemented
+
+        data = b"GET /foobar HTTP/1.1\r\ntransfer-encoding: gzip\r\n"
+
+        try:
+            self.parser.parse_header(data)
+        except TransferEncodingNotImplemented as e:
+            self.assertIn("Transfer-Encoding requested is not supported.", e.args[0])
+        else:  # pragma: nocover
+            self.assertTrue(False)
+
+    def test_parse_header_transfer_encoding_invalid_multiple(self):
+        from waitress.parser import TransferEncodingNotImplemented
+
+        data = b"GET /foobar HTTP/1.1\r\ntransfer-encoding: gzip\r\ntransfer-encoding: chunked\r\n"
+
+        try:
+            self.parser.parse_header(data)
+        except TransferEncodingNotImplemented as e:
+            self.assertIn("Transfer-Encoding requested is not supported.", e.args[0])
+        else:  # pragma: nocover
+            self.assertTrue(False)
+
     def test_parse_header_11_expect_continue(self):
         data = b"GET /foobar HTTP/1.1\r\nexpect: 100-continue\r\n"
         self.parser.parse_header(data)
diff --git a/waitress/utilities.py b/waitress/utilities.py
index 3d9d397..0336897 100644
--- a/waitress/utilities.py
+++ b/waitress/utilities.py
@@ -302,3 +302,8 @@ class RequestEntityTooLarge(BadRequest):
 class InternalServerError(Error):
     code = 500
     reason = "Internal Server Error"
+
+
+class ServerNotImplemented(Error):
+    code = 501
+    reason = "Not Implemented"