Remove support for non CRLF line endings

https://tools.ietf.org/html/rfc7230#section-3.5 says that servers MAY
implement their parsers to use only the LF as a delimeter between lines,
however if the frontend server does NOT do the same you can potentially
allow a single HTTP request to be treated differently by the two
servers.

This issue can be used to cause HTTP request smuggling or HTTP desync
which may lead to vulnerabilities.

To increase robustness Waitress will no longer allow bare LF for HTTP
messages/headers and chunked encoding and instead now enforces that the
line endings at CRLF.

waitress/parser.py

@@ -19,24 +19,14 @@
 import re
 from io import BytesIO
 
-from waitress.compat import (
-    tostr,
-    urlparse,
-    unquote_bytes_to_wsgi,
-)
-
 from waitress.buffers import OverflowableBuffer
-
-from waitress.receiver import (
-    FixedStreamReceiver,
-    ChunkedReceiver,
-)
-
+from waitress.compat import tostr, unquote_bytes_to_wsgi, urlparse
+from waitress.receiver import ChunkedReceiver, FixedStreamReceiver
 from waitress.utilities import (
-    find_double_newline,
+    BadRequest,
     RequestEntityTooLarge,
     RequestHeaderFieldsTooLarge,
-    BadRequest,
+    find_double_newline,
 )
 
 
@@ -95,8 +85,13 @@ def received(self, data):
                 # Header finished.
                 header_plus = s[:index]
                 consumed = len(data) - (len(s) - index)
-                # Remove preceeding blank lines.
+
+                # Remove preceeding blank lines. This is suggested by
+                # https://tools.ietf.org/html/rfc7230#section-3.5 to support
+                # clients sending an extra CR LF after another request when
+                # using HTTP pipelining
                 header_plus = header_plus.lstrip()
+
                 if not header_plus:
                     self.empty = True
                     self.completed = True
@@ -169,13 +164,15 @@ def parse_header(self, header_plus):
         Parses the header_plus block of text (the headers plus the
         first line of the request).
         """
-        index = header_plus.find(b"\n")
+        index = header_plus.find(b"\r\n")
         if index >= 0:
             first_line = header_plus[:index].rstrip()
-            header = header_plus[index + 1 :]
+            header = header_plus[index + 2 :]
         else:
-            first_line = header_plus.rstrip()
-            header = b""
+            raise ParsingError("HTTP message header invalid")
+
+        if b"\r" in first_line or b"\n" in first_line:
+            raise ParsingError("Bare CR or LF found in HTTP message")
 
         self.first_line = first_line  # for testing
 
@@ -299,8 +296,11 @@ def get_header_lines(header):
     Splits the header into lines, putting multi-line headers together.
     """
     r = []
-    lines = header.split(b"\n")
+    lines = header.split(b"\r\n")
     for line in lines:
+        if b"\r" in line or b"\n" in line:
+            raise ParsingError('Bare CR or LF found in header line "%s"' % tostr(line))
+
         if line.startswith((b" ", b"\t")):
             if not r:
                 # https://corte.si/posts/code/pathod/pythonservers/index.html

waitress/receiver.py

@@ -14,9 +14,7 @@
 """Data Chunk Receiver
 """
 
-from waitress.utilities import find_double_newline
-
-from waitress.utilities import BadRequest
+from waitress.utilities import BadRequest, find_double_newline
 
 
 class FixedStreamReceiver(object):
@@ -35,18 +33,23 @@ def __len__(self):
     def received(self, data):
         "See IStreamConsumer"
         rm = self.remain
+
         if rm < 1:
             self.completed = True  # Avoid any chance of spinning
+
             return 0
         datalen = len(data)
+
         if rm <= datalen:
             self.buf.append(data[:rm])
             self.remain = 0
             self.completed = True
+
             return rm
         else:
             self.buf.append(data)
             self.remain -= datalen
+
             return datalen
 
     def getfile(self):
@@ -59,6 +62,7 @@ def getbuf(self):
 class ChunkedReceiver(object):
 
     chunk_remainder = 0
+    validate_chunk_end = False
     control_line = b""
     all_chunks_received = False
     trailer = b""
@@ -76,35 +80,57 @@ def __len__(self):
 
     def received(self, s):
         # Returns the number of bytes consumed.
+
         if self.completed:
             return 0
         orig_size = len(s)
+
         while s:
             rm = self.chunk_remainder
+
             if rm > 0:
                 # Receive the remainder of a chunk.
                 to_write = s[:rm]
                 self.buf.append(to_write)
                 written = len(to_write)
                 s = s[written:]
+
                 self.chunk_remainder -= written
+
+                if self.chunk_remainder == 0:
+                    self.validate_chunk_end = True
+            elif self.validate_chunk_end:
+                pos = s.find(b"\r\n")
+
+                if pos == 0:
+                    # Chop off the terminating CR LF from the chunk
+                    s = s[2:]
+                else:
+                    self.error = BadRequest("Chunk not properly terminated")
+                    self.all_chunks_received = True
+
+                # Always exit this loop
+                self.validate_chunk_end = False
             elif not self.all_chunks_received:
                 # Receive a control line.
                 s = self.control_line + s
-                pos = s.find(b"\n")
+                pos = s.find(b"\r\n")
+
                 if pos < 0:
                     # Control line not finished.
                     self.control_line = s
                     s = ""
                 else:
                     # Control line finished.
                     line = s[:pos]
-                    s = s[pos + 1 :]
+                    s = s[pos + 2 :]
                     self.control_line = b""
                     line = line.strip()
+
                     if line:
                         # Begin a new chunk.
                         semi = line.find(b";")
+
                         if semi >= 0:
                             # discard extension info.
                             line = line[:semi]
@@ -113,6 +139,7 @@ def received(self, s):
                         except ValueError:  # garbage in input
                             self.error = BadRequest("garbage in chunked encoding input")
                             sz = 0
+
                         if sz > 0:
                             # Start a new chunk.
                             self.chunk_remainder = sz
@@ -123,15 +150,14 @@ def received(self, s):
             else:
                 # Receive the trailer.
                 trailer = self.trailer + s
+
                 if trailer.startswith(b"\r\n"):
                     # No trailer.
                     self.completed = True
+
                     return orig_size - (len(trailer) - 2)
-                elif trailer.startswith(b"\n"):
-                    # No trailer.
-                    self.completed = True
-                    return orig_size - (len(trailer) - 1)
                 pos = find_double_newline(trailer)
+
                 if pos < 0:
                     # Trailer not finished.
                     self.trailer = trailer
@@ -140,7 +166,9 @@ def received(self, s):
                     # Finished the trailer.
                     self.completed = True
                     self.trailer = trailer[:pos]
+
                     return orig_size - (len(trailer) - pos)
+
         return orig_size
 
     def getfile(self):

waitress/tests/test_channel.py

@@ -423,7 +423,7 @@ def test_del_channel(self):
     def test_received(self):
         inst, sock, map = self._makeOneWithMap()
         inst.server = DummyServer()
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.server.tasks, [inst])
         self.assertTrue(inst.requests)
 
@@ -438,7 +438,7 @@ def test_received_preq_not_completed(self):
         inst.request = preq
         preq.completed = False
         preq.empty = True
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.requests, ())
         self.assertEqual(inst.server.tasks, [])
 
@@ -449,7 +449,7 @@ def test_received_preq_completed_empty(self):
         inst.request = preq
         preq.completed = True
         preq.empty = True
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.request, None)
         self.assertEqual(inst.server.tasks, [])
 
@@ -460,7 +460,7 @@ def test_received_preq_error(self):
         inst.request = preq
         preq.completed = True
         preq.error = True
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.request, None)
         self.assertEqual(len(inst.server.tasks), 1)
         self.assertTrue(inst.requests)
@@ -473,24 +473,10 @@ def test_received_preq_completed_connection_close(self):
         preq.completed = True
         preq.empty = True
         preq.connection_close = True
-        inst.received(b"GET / HTTP/1.1\n\n" + b"a" * 50000)
+        inst.received(b"GET / HTTP/1.1\r\n\r\n" + b"a" * 50000)
         self.assertEqual(inst.request, None)
         self.assertEqual(inst.server.tasks, [])
 
-    def test_received_preq_completed_n_lt_data(self):
-        inst, sock, map = self._makeOneWithMap()
-        inst.server = DummyServer()
-        preq = DummyParser()
-        inst.request = preq
-        preq.completed = True
-        preq.empty = False
-        line = b"GET / HTTP/1.1\n\n"
-        preq.retval = len(line)
-        inst.received(line + line)
-        self.assertEqual(inst.request, None)
-        self.assertEqual(len(inst.requests), 2)
-        self.assertEqual(len(inst.server.tasks), 1)
-
     def test_received_headers_finished_expect_continue_false(self):
         inst, sock, map = self._makeOneWithMap()
         inst.server = DummyServer()
@@ -501,7 +487,7 @@ def test_received_headers_finished_expect_continue_false(self):
         preq.completed = False
         preq.empty = False
         preq.retval = 1
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.request, preq)
         self.assertEqual(inst.server.tasks, [])
         self.assertEqual(inst.outbufs[0].get(100), b"")
@@ -515,7 +501,7 @@ def test_received_headers_finished_expect_continue_true(self):
         preq.headers_finished = True
         preq.completed = False
         preq.empty = False
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.request, preq)
         self.assertEqual(inst.server.tasks, [])
         self.assertEqual(sock.sent, b"HTTP/1.1 100 Continue\r\n\r\n")
@@ -532,7 +518,7 @@ def test_received_headers_finished_expect_continue_true_sent_true(self):
         preq.completed = False
         preq.empty = False
         inst.sent_continue = True
-        inst.received(b"GET / HTTP/1.1\n\n")
+        inst.received(b"GET / HTTP/1.1\r\n\r\n")
         self.assertEqual(inst.request, preq)
         self.assertEqual(inst.server.tasks, [])
         self.assertEqual(sock.sent, b"")