Merge pull request #776 from gevent/issue775

Fix #775

.landscape.yml

@@ -41,7 +41,7 @@ ignore-paths:
   - greentest/getaddrinfo_module.py
 ignore-patterns:
   # disabled code
-  - greentest/xtest_.*py
+  - ^greentest/xtest_.*py
   # standard library code
   - ^greentest/2.*
   - ^greentest/3.*

changelog.rst

@@ -22,6 +22,10 @@
 - :class:`~.Group` and :class:`~.Pool` now return whether
   :meth:`~.Group.join` returned with an empty group. Suggested by Filippo Sironi in
   :pr:`503`.
+- Security: :mod:`gevent.pywsgi` now checks that the values passed to
+  ``start_response`` do not contain a carriage return or newline in
+  order to prevent HTTP response splitting (header injection), raising
+  a :exc:`ValueError` if they do. See :issue:`775`.
 
 1.1.0 (Mar 5, 2016)
 ===================

gevent/pywsgi.py

@@ -751,12 +751,16 @@ class WSGIHandler(object):
 
     def start_response(self, status, headers, exc_info=None):
         """
+         .. versionchanged:: 1.2a1
+            Avoid HTTP header injection by raising a :exc:`ValueError`
+            if *status* or any *header* name or value contains a carriage
+            return or newline.
          .. versionchanged:: 1.1b5
             Pro-actively handle checking the encoding of the status line
             and headers during this method. On Python 2, avoid some
             extra encodings.
         """
-        # pylint:disable=too-many-branches
+        # pylint:disable=too-many-branches,too-many-statements
         if exc_info:
             try:
                 if self.headers_sent:
@@ -776,6 +780,7 @@ class WSGIHandler(object):
         # UnicodeError without any clue which header was wrong.
         # Note that this results in copying the header list at this point, not modifying it,
         # although we are allowed to do so if needed. This slightly increases memory usage.
+        # We also check for HTTP Response Splitting vulnerabilities
         response_headers = []
         header = None
         value = None
@@ -785,6 +790,10 @@ class WSGIHandler(object):
                     raise UnicodeError("The header must be a native string", header, value)
                 if not isinstance(value, str):
                     raise UnicodeError("The value must be a native string", header, value)
+                if '\r' in header or '\n' in header:
+                    raise ValueError('carriage return or newline in header name', header)
+                if '\r' in value or '\n' in value:
+                    raise ValueError('carriage return or newline in header value', value)
                 # Either we're on Python 2, in which case bytes is correct, or
                 # we're on Python 3 and the user screwed up (because it should be a native
                 # string). In either case, make sure that this is latin-1 compatible. Under
@@ -806,6 +815,8 @@ class WSGIHandler(object):
         # Same as above
         if not isinstance(status, str):
             raise UnicodeError("The status string must be a native string")
+        if '\r' in status or '\n' in status:
+            raise ValueError("carriage return or newline in status", status)
         # don't assign to anything until the validation is complete, including parsing the
         # code
         code = int(status.split(' ', 1)[0])
@@ -1263,3 +1274,30 @@ class WSGIServer(StreamServer):
         # pylint:disable=method-hidden
         handler = self.handler_class(sock, address, self)
         handler.handle()
+
+def _main():
+    # Provisional main handler, for quick tests, not production
+    # usage.
+    from gevent import monkey; monkey.patch_all()
+
+    import argparse
+    import importlib
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument("app", help="dotted name of WSGI app callable [module:callable]")
+    parser.add_argument("-b", "--bind",
+                        help="The socket to bind",
+                        default=":8080")
+
+    args = parser.parse_args()
+
+    module_name, app_name = args.app.split(':')
+    module = importlib.import_module(module_name)
+    app = getattr(module, app_name)
+    bind = args.bind
+
+    server = WSGIServer(bind, app)
+    server.serve_forever()
+
+if __name__ == '__main__':
+    _main()

greentest/test__pywsgi.py

@@ -17,6 +17,7 @@
 # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.
+# pylint: disable=too-many-lines,unused-argument
 from __future__ import print_function
 from gevent import monkey
 monkey.patch_all(thread=False)
@@ -1244,6 +1245,9 @@ Cookie: name2="value2"\n\n'''.replace('\n', '\r\n'))
 
 class TestLeakInput(TestCase):
 
+    _leak_wsgi_input = None
+    _leak_environ = None
+
     def application(self, environ, start_response):
         pi = environ["PATH_INFO"]
         self._leak_wsgi_input = environ["wsgi.input"]
@@ -1268,6 +1272,49 @@ class TestLeakInput(TestCase):
         assert d.startswith(b"HTTP/1.1 200 OK"), "bad response: %r" % d
         self._leak_environ.pop('_leak')
 
+class TestHTTPResponseSplitting(TestCase):
+    # The validator would prevent the app from doing the
+    # bad things it needs to do
+    validator = None
+
+    status = '200 OK'
+    headers = ()
+    start_exc = None
+
+    def setUp(self):
+        TestCase.setUp(self)
+        self.start_exc = None
+        self.status = TestHTTPResponseSplitting.status
+        self.headers = TestHTTPResponseSplitting.headers
+
+    def application(self, environ, start_response):
+        try:
+            start_response(self.status, self.headers)
+        except Exception as e: # pylint: disable=broad-except
+            self.start_exc = e
+        else:
+            self.start_exc = None
+        return ()
+
+    def _assert_failure(self, message):
+        fd = self.makefile()
+        fd.write('GET / HTTP/1.0\r\nHost: localhost\r\n\r\n')
+        fd.read()
+        self.assertIsInstance(self.start_exc, ValueError)
+        self.assertEqual(self.start_exc.args[0], message)
+
+    def test_newline_in_status(self):
+        self.status = '200 OK\r\nConnection: close\r\nContent-Length: 0\r\n\r\n'
+        self._assert_failure('carriage return or newline in status')
+
+    def test_newline_in_header_value(self):
+        self.headers = [('Test', 'Hi\r\nConnection: close')]
+        self._assert_failure('carriage return or newline in header value')
+
+    def test_newline_in_header_name(self):
+        self.headers = [('Test\r\n', 'Hi')]
+        self._assert_failure('carriage return or newline in header name')
+
 
 class TestInvalidEnviron(TestCase):
     validator = None