Merge pull request #277 from Pylons/invalid-whitespace-cont

Invalid whitespace in headers

CHANGES.txt

@@ -1,3 +1,30 @@
+1.4.2 (2020-01-??)
+------------------
+
+Security Fixes
+~~~~~~~~~~~~~~
+
+- This is a follow-up to the fix introduced in 1.4.1 to tighten up the way
+  Waitress strips whitespace from header values. This makes sure Waitress won't
+  accidentally treat non-printable characters as whitespace and lead to a
+  potental HTTP request smuggling/splitting security issue.
+
+  Thanks to ZeddYu Lu for the extra test cases.
+
+  Please see the security advisory for more information:
+  https://github.com/Pylons/waitress/security/advisories/GHSA-m5ff-3wj3-8ph4
+
+  CVE-ID: CVE-2019-16789
+
+Bugfixes
+~~~~~~~~
+
+- Updated the regex used to validate header-field content to match the errata
+  that was published for RFC7230.
+
+  See: https://www.rfc-editor.org/errata_search.php?rfc=7230&eid=4189
+
+
 1.4.1 (2019-12-24)
 ------------------
 
@@ -12,6 +39,8 @@ Security Fixes
   Please see the security advisory for more information:
   https://github.com/Pylons/waitress/security/advisories/GHSA-m5ff-3wj3-8ph4
 
+  CVE-ID: CVE-2019-16789
+
 1.4.0 (2019-12-20)
 ------------------
 
@@ -80,7 +109,7 @@ Security Fixes
   ``Transfer-Encoding: chunked`` instead of ``Transfer-Encoding: identity,
   chunked``.
 
-  PLease see the security advisory for more information: 
+  Please see the security advisory for more information:
   https://github.com/Pylons/waitress/security/advisories/GHSA-g2xc-35jw-c63p
 
   CVE-ID: CVE-2019-16786

setup.py

@@ -34,7 +34,7 @@
 
 setup(
     name="waitress",
-    version="1.4.1",
+    version="1.4.2",
     author="Zope Foundation and Contributors",
     author_email="zope-dev@zope.org",
     maintainer="Pylons Project",

waitress/parser.py

@@ -213,13 +213,15 @@ def parse_header(self, header_plus):
             if not header:
                 raise ParsingError("Invalid header")
 
-            key, value = header.group('name', 'value')
+            key, value = header.group("name", "value")
 
             if b"_" in key:
                 # TODO(xistence): Should we drop this request instead?
                 continue
 
-            value = value.strip()
+            # Only strip off whitespace that is considered valid whitespace by
+            # RFC7230, don't strip the rest
+            value = value.strip(b" \t")
             key1 = tostr(key.upper().replace(b"-", b"_"))
             # If a header already exists, we append subsequent values
             # seperated by a comma. Applications already need to handle
@@ -257,7 +259,16 @@ def parse_header(self, header_plus):
             # here
             te = headers.pop("TRANSFER_ENCODING", "")
 
-            encodings = [encoding.strip().lower() for encoding in te.split(",") if encoding]
+            # NB: We can not just call bare strip() here because it will also
+            # remove other non-printable characters that we explicitly do not
+            # want removed so that if someone attempts to smuggle a request
+            # with these characters we don't fall prey to it.
+            #
+            # For example \x85 is stripped by default, but it is not considered
+            # valid whitespace to be stripped by RFC7230.
+            encodings = [
+                encoding.strip(" \t").lower() for encoding in te.split(",") if encoding
+            ]
 
             for encoding in encodings:
                 # Out of the transfer-codings listed in

waitress/rfc7230.py

@@ -33,12 +33,20 @@
 # field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
 # field-vchar    = VCHAR / obs-text
 
+# Errata from: https://www.rfc-editor.org/errata_search.php?rfc=7230&eid=4189
+# changes field-content to:
+#
+# field-content  = field-vchar [ 1*( SP / HTAB / field-vchar )
+#                  field-vchar ]
+
 FIELD_VCHAR = "[" + VCHAR + OBS_TEXT + "]"
-FIELD_CONTENT = FIELD_VCHAR + "(" + RWS + FIELD_VCHAR + "){0,}"
+FIELD_CONTENT = FIELD_VCHAR + "([ \t" + VCHAR + OBS_TEXT + "]+" + FIELD_VCHAR + "){,1}"
 FIELD_VALUE = "(" + FIELD_CONTENT + "){0,}"
 
 HEADER_FIELD = re.compile(
     tobytes(
         "^(?P<name>" + TOKEN + "):" + OWS + "(?P<value>" + FIELD_VALUE + ")" + OWS + "$"
     )
 )
+
+OWS_STRIP = re.compile(OWS + "(?P<value>.*?)" + OWS)

waitress/tests/test_parser.py

@@ -236,6 +236,35 @@ def test_parse_header_transfer_encoding_invalid_multiple(self):
         else:  # pragma: nocover
             self.assertTrue(False)
 
+    def test_parse_header_transfer_encoding_invalid_whitespace(self):
+        from waitress.parser import TransferEncodingNotImplemented
+
+        data = b"GET /foobar HTTP/1.1\r\nTransfer-Encoding:\x85chunked\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_unicode(self):
+        from waitress.parser import TransferEncodingNotImplemented
+
+        # This is the binary encoding for the UTF-8 character
+        # https://www.compart.com/en/unicode/U+212A "unicode character "K""
+        # which if waitress were to accidentally do the wrong thing get
+        # lowercased to just the ascii "k" due to unicode collisions during
+        # transformation
+        data = b"GET /foobar HTTP/1.1\r\nTransfer-Encoding: chun\xe2\x84\xaaed\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)
@@ -394,6 +423,15 @@ def test_parse_header_multiple_values_header_folded_multiple(self):
         self.assertIn("FOO", self.parser.headers)
         self.assertEqual(self.parser.headers["FOO"], "bar, whatever, more, please, yes")
 
+    def test_parse_header_multiple_values_extra_space(self):
+        # Tests errata from: https://www.rfc-editor.org/errata_search.php?rfc=7230&eid=4189
+        from waitress.parser import ParsingError
+
+        data = b"GET /foobar HTTP/1.1\r\nfoo: abrowser/0.001 (C O M M E N T)\r\n"
+        self.parser.parse_header(data)
+
+        self.assertIn("FOO", self.parser.headers)
+        self.assertEqual(self.parser.headers["FOO"], "abrowser/0.001 (C O M M E N T)")
 
 
 class Test_split_uri(unittest.TestCase):