From 2f53c851eaf926767fbac62385615efd4886221c Mon Sep 17 00:00:00 2001
From: Jason Madden <jamadden@gmail.com>
Date: Thu, 31 Aug 2023 17:05:48 -0500
Subject: [PATCH] gevent.pywsgi: Much improved handling of chunk trailers.

Validation is much stricter to the specification.

Fixes #1989
---
 gevent/pywsgi.py          |   82 ++++++++++++++++++++++++++++++++++++++--------
 gevent/subprocess.py      |    7 ++-
 greentest/test__pywsgi.py |   31 ++++++++---------
 3 files changed, 89 insertions(+), 31 deletions(-)
 create mode 100644 docs/changes/1989.bugfix

--- a/gevent/pywsgi.py
+++ b/gevent/pywsgi.py
@@ -8,6 +8,23 @@ WSGI work is handled by :class:`WSGIHand
 created for each request. The server can be customized to use
 different subclasses of :class:`WSGIHandler`.
 
+This server is intended primarily for development and testing,
+and secondarily for other "safe" scenarios where it will not be
+exposed to potentially malicious input. The code has not been
+security audited, and is not intended for direct exposure to the
+public Internet. For production usage on the Internet, either
+choose a production-strength server such as gunicorn, or put
+a reverse proxy between gevent and the Internet.
+
+Complies more closely with the HTTP specification for chunked
+transfer encoding. In particular, we are much stricter about
+trailers, and trailers that are invalid (too long or featuring
+disallowed characters) forcibly close the connection to the
+client *after* the results have been sent.
+
+Trailers otherwise continue to be ignored and are not available
+to the WSGI application.
+
 """
 import errno
 from io import BytesIO
@@ -44,29 +61,48 @@ __all__ = [
 
 MAX_REQUEST_LINE = 8192
 # Weekday and month names for HTTP date/time formatting; always English!
-_WEEKDAYNAME = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]
-_MONTHNAME = [None,  # Dummy so we can use 1-based month numbers
+_WEEKDAYNAME = ("Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun")
+_MONTHNAME = (None,  # Dummy so we can use 1-based month numbers
               "Jan", "Feb", "Mar", "Apr", "May", "Jun",
-              "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]
+              "Jul", "Aug", "Sep", "Oct", "Nov", "Dec")
 
 # The contents of the "HEX" grammar rule for HTTP, upper and lowercase A-F plus digits,
 # in byte form for comparing to the network.
 _HEX = string.hexdigits.encode('ascii')
 
+# The characters allowed in "token" rules.
+
+# token          = 1*tchar
+# tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
+#                / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
+#                / DIGIT / ALPHA
+#                ; any VCHAR, except delimiters
+# ALPHA          =  %x41-5A / %x61-7A   ; A-Z / a-z
+_ALLOWED_TOKEN_CHARS = frozenset(
+    # Remember we have to be careful because bytestrings
+    # inexplicably iterate as integers, which are not equal to bytes.
+
+    # explicit chars then DIGIT
+    (c.encode('ascii') for c in "!#$%&'*+-.^_`|~0123456789")
+    # Then we add ALPHA
+) | {c.encode('ascii') for c in string.ascii_letters}
+assert b'A' in _ALLOWED_TOKEN_CHARS
+
+
 # Errors
 _ERRORS = dict()
 _INTERNAL_ERROR_STATUS = '500 Internal Server Error'
 _INTERNAL_ERROR_BODY = b'Internal Server Error'
-_INTERNAL_ERROR_HEADERS = [('Content-Type', 'text/plain'),
+_INTERNAL_ERROR_HEADERS = (('Content-Type', 'text/plain'),
                            ('Connection', 'close'),
-                           ('Content-Length', str(len(_INTERNAL_ERROR_BODY)))]
+                           ('Content-Length', str(len(_INTERNAL_ERROR_BODY))))
 _ERRORS[500] = (_INTERNAL_ERROR_STATUS, _INTERNAL_ERROR_HEADERS, _INTERNAL_ERROR_BODY)
 
 _BAD_REQUEST_STATUS = '400 Bad Request'
 _BAD_REQUEST_BODY = ''
-_BAD_REQUEST_HEADERS = [('Content-Type', 'text/plain'),
+_BAD_REQUEST_HEADERS = (('Content-Type', 'text/plain'),
                         ('Connection', 'close'),
-                        ('Content-Length', str(len(_BAD_REQUEST_BODY)))]
+                        ('Content-Length', str(len(_BAD_REQUEST_BODY))))
 _ERRORS[400] = (_BAD_REQUEST_STATUS, _BAD_REQUEST_HEADERS, _BAD_REQUEST_BODY)
 
 _REQUEST_TOO_LONG_RESPONSE = b"HTTP/1.1 414 Request URI Too Long\r\nConnection: close\r\nContent-length: 0\r\n\r\n"
@@ -509,7 +545,8 @@ class WSGIHandler(object):
         elif len(words) == 2:
             self.command, self.path = words
             if self.command != "GET":
-                raise _InvalidClientRequest('Expected GET method: %r', raw_requestline)
+                raise _InvalidClientRequest('Expected GET method; Got command=%r; path=%r; raw=%r' % (
+                    self.command, self.path, raw_requestline,))
             self.request_version = "HTTP/0.9"
             # QQQ I'm pretty sure we can drop support for HTTP/0.9
         else:
@@ -888,14 +925,28 @@ class WSGIHandler(object):
                     close()
                 try:
                     self.wsgi_input._discard()
+                except _InvalidClientInput:
+                    # This one is deliberately raised to the outer
+                    # scope, because, with the incoming stream in some bad state,
+                    # we can't be sure we can synchronize and properly parse the next
+                    # request.
+                    raise
                 except (socket.error, IOError):
-                    # Don't let exceptions during discarding
+                    # Don't let socket exceptions during discarding
                     # input override any exception that may have been
                     # raised by the application, such as our own _InvalidClientInput.
                     # In the general case, these aren't even worth logging (see the comment
                     # just below)
                     pass
-        except _InvalidClientInput:
+        except _InvalidClientInput as ex:
+            # DO log this one because:
+            # - Some of the data may have been read and acted on by the
+            #   application;
+            # - The response may or may not have been sent;
+            # - It's likely that the client is bad, or malicious, and
+            #   users might wish to take steps to block the client.
+            self._handle_client_error(ex)
+            self.close_connection = True
             self._send_error_response_if_possible(400)
         except socket.error as ex:
             if ex.args[0] in (errno.EPIPE, errno.ECONNRESET):
@@ -938,16 +989,21 @@ class WSGIHandler(object):
     def _handle_client_error(self, ex):
         # Called for invalid client input
         # Returns the appropriate error response.
-        if not isinstance(ex, ValueError):
+        if not isinstance(ex, (ValueError, _InvalidClientInput)):
             # XXX: Why not self._log_error to send it through the loop's
             # handle_error method?
+            # _InvalidClientRequest is a ValueError; _InvalidClientInput is an IOError.
             traceback.print_exc()
         if isinstance(ex, _InvalidClientRequest):
             # These come with good error messages, and we want to let
             # log_error deal with the formatting, especially to handle encoding
-            self.log_error(*ex.args)
+            # However, the error messages do not include the requesting IP
+            # necessarily, so we do add that.
+            self.log_error('(from %s) %s', self.client_address, ex.formatted_message)
         else:
-            self.log_error('Invalid request: %s', str(ex) or ex.__class__.__name__)
+            self.log_error('Invalid request (from %s): %s',
+                           self.client_address,
+                           str(ex) or ex.__class__.__name__)
         return ('400', _BAD_REQUEST_RESPONSE)
 
     def _headers(self):
--- a/gevent/subprocess.py
+++ b/gevent/subprocess.py
@@ -248,10 +248,11 @@ if PY3:
 
         To capture standard error in the result, use ``stderr=STDOUT``::
 
-            >>> check_output(["/bin/sh", "-c",
+        >>> output = check_output(["/bin/sh", "-c",
             ...               "ls -l non_existent_file ; exit 0"],
-            ...              stderr=STDOUT)
-            b'ls: non_existent_file: No such file or directory\n'
+            ...              stderr=STDOUT).decode('ascii').strip()
+        >>> print(output.rsplit(':', 1)[1].strip())
+        No such file or directory
 
         There is an additional optional argument, "input", allowing you to
         pass a string to the subprocess's stdin.  If you use this argument
--- a/greentest/test__pywsgi.py
+++ b/greentest/test__pywsgi.py
@@ -21,21 +21,10 @@ from __future__ import print_function
 from gevent import monkey
 monkey.patch_all(thread=False)
 
-try:
-    from urllib.parse import parse_qs
-except ImportError:
-    # Python 2
-    from cgi import parse_qs
+from urllib.parse import parse_qs
 import os
 import sys
-try:
-    # On Python 2, we want the C-optimized version if
-    # available; it has different corner-case behaviour than
-    # the Python implementation, and it used by socket.makefile
-    # by default.
-    from cStringIO import StringIO
-except ImportError:
-    from io import BytesIO as StringIO
+from io import BytesIO as StringIO
 import weakref
 try:
     from wsgiref.validate import validator
@@ -167,6 +156,11 @@ class Response(object):
 
     @classmethod
     def read(cls, fd, code=200, reason='default', version='1.1', body=None, chunks=None, content_length=None):
+        """
+        Read an HTTP response, optionally perform assertions,
+        and return the Response object.
+        """
+        # pylint:disable=too-many-branches
         _status_line, headers = read_headers(fd)
         self = cls(_status_line, headers)
         if code is not None:
@@ -554,8 +548,15 @@ class TestBigChunks(TestChunkedApp):
 
 class TestChunkedPost(TestCase):
 
-    @staticmethod
-    def application(env, start_response):
+    calls = 0
+
+    def setUp(self):
+        super().setUp()
+        self.calls = 0
+
+    def application(self, env, start_response):
+        self.calls += 1
+        self.assertTrue(env.get('wsgi.input_terminated'))
         start_response('200 OK', [('Content-Type', 'text/plain')])
         if env['PATH_INFO'] == '/a':
             data = env['wsgi.input'].read(6)
