From 2b66a306fbb4af8032e89e34848d16f5cfe79776 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Sun, 26 May 2019 20:45:59 -0700 Subject: [PATCH 01/13] Raise TypeError for non-bytes Agent.request method --- src/twisted/web/client.py | 3 +++ src/twisted/web/test/test_agent.py | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/src/twisted/web/client.py b/src/twisted/web/client.py index e65ac9ad259..afcd5d4a26d 100644 --- a/src/twisted/web/client.py +++ b/src/twisted/web/client.py @@ -1714,6 +1714,9 @@ def request(self, method, uri, headers=None, bodyProducer=None): @see: L{twisted.web.iweb.IAgent.request} """ + if not isinstance(method, bytes): + raise TypeError('method={!r} is {}, but must be bytes'.format( + method, type(method))) parsedURI = URI.fromBytes(uri) try: endpoint = self._getEndpoint(parsedURI) diff --git a/src/twisted/web/test/test_agent.py b/src/twisted/web/test/test_agent.py index afc63044bdf..7615d71e44d 100644 --- a/src/twisted/web/test/test_agent.py +++ b/src/twisted/web/test/test_agent.py @@ -989,6 +989,14 @@ def getConnection(this, key, ep): self.assertEqual(agent._pool.connected, True) + def test_nonBytesMethod(self): + """ + L{Agent.request} raises L{TypeError} when the C{method} argument isn't + L{bytes}. + """ + self.assertRaises(TypeError, self.agent.request, u'GET', b'http://foo.example/') + + def test_unsupportedScheme(self): """ L{Agent.request} returns a L{Deferred} which fails with From 725cba630ed76171c17e55381b4f53a34240684a Mon Sep 17 00:00:00 2001 From: Tom Most Date: Sun, 26 May 2019 20:48:36 -0700 Subject: [PATCH 02/13] Newsfile --- src/twisted/newsfragments/9643.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 src/twisted/newsfragments/9643.bugfix diff --git a/src/twisted/newsfragments/9643.bugfix b/src/twisted/newsfragments/9643.bugfix new file mode 100644 index 00000000000..db7850cff7a --- /dev/null +++ b/src/twisted/newsfragments/9643.bugfix @@ -0,0 +1 @@ +twisted.web.client.Agent.request() now produces TypeError when the method argument is not bytes, rather than failing to generate the request. From 48220a4faa143bb2fa1253bac11aff4c43d256ac Mon Sep 17 00:00:00 2001 From: Tom Most Date: Sun, 26 May 2019 20:59:06 -0700 Subject: [PATCH 03/13] Unlengthen line --- src/twisted/web/test/test_agent.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/twisted/web/test/test_agent.py b/src/twisted/web/test/test_agent.py index 7615d71e44d..b2864589bdf 100644 --- a/src/twisted/web/test/test_agent.py +++ b/src/twisted/web/test/test_agent.py @@ -994,7 +994,8 @@ def test_nonBytesMethod(self): L{Agent.request} raises L{TypeError} when the C{method} argument isn't L{bytes}. """ - self.assertRaises(TypeError, self.agent.request, u'GET', b'http://foo.example/') + self.assertRaises(TypeError, self.agent.request, + u'GET', b'http://foo.example/') def test_unsupportedScheme(self): From 10f6d03ec90a844d496214450604ac89b50739b5 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 28 May 2019 19:50:46 -0700 Subject: [PATCH 04/13] ProxyAgent too --- src/twisted/newsfragments/9643.bugfix | 2 +- src/twisted/web/client.py | 6 +++--- src/twisted/web/test/test_agent.py | 9 +++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/twisted/newsfragments/9643.bugfix b/src/twisted/newsfragments/9643.bugfix index db7850cff7a..05740570568 100644 --- a/src/twisted/newsfragments/9643.bugfix +++ b/src/twisted/newsfragments/9643.bugfix @@ -1 +1 @@ -twisted.web.client.Agent.request() now produces TypeError when the method argument is not bytes, rather than failing to generate the request. +twisted.web.client.Agent.request() and twisted.web.client.ProxyAgent.request() now produce TypeError when the method argument is not bytes, rather than failing to generate the request. diff --git a/src/twisted/web/client.py b/src/twisted/web/client.py index afcd5d4a26d..e1a92fef4b2 100644 --- a/src/twisted/web/client.py +++ b/src/twisted/web/client.py @@ -1490,6 +1490,9 @@ def _requestWithEndpoint(self, key, endpoint, method, parsedURI, Issue a new request, given the endpoint and the path sent as part of the request. """ + if not isinstance(method, bytes): + raise TypeError('method={!r} is {}, but must be bytes'.format( + method, type(method))) # Create minimal headers, if necessary: if headers is None: headers = Headers() @@ -1714,9 +1717,6 @@ def request(self, method, uri, headers=None, bodyProducer=None): @see: L{twisted.web.iweb.IAgent.request} """ - if not isinstance(method, bytes): - raise TypeError('method={!r} is {}, but must be bytes'.format( - method, type(method))) parsedURI = URI.fromBytes(uri) try: endpoint = self._getEndpoint(parsedURI) diff --git a/src/twisted/web/test/test_agent.py b/src/twisted/web/test/test_agent.py index b2864589bdf..fc67d154198 100644 --- a/src/twisted/web/test/test_agent.py +++ b/src/twisted/web/test/test_agent.py @@ -2484,6 +2484,15 @@ def setUp(self): self.agent._proxyEndpoint = self.StubEndpoint(oldEndpoint, self) + def test_nonBytesMethod(self): + """ + L{ProxyAgent.request} raises L{TypeError} when the C{method} argument + isn't L{bytes}. + """ + self.assertRaises(TypeError, self.agent.request, + u'GET', b'http://foo.example/') + + def test_proxyRequest(self): """ L{client.ProxyAgent} issues an HTTP request against the proxy, with the From 160238b48a722e20ec1c95d4030e7f4c6227d003 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 23:08:54 -0700 Subject: [PATCH 05/13] Use HTTPS links in the readme --- README.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.rst b/README.rst index 3b03e648351..ea489d8f572 100644 --- a/README.rst +++ b/README.rst @@ -41,10 +41,10 @@ Additional instructions for installing this software are in `the installation in Documentation and Support ------------------------- -Twisted's documentation is available from the `Twisted Matrix website `_. +Twisted's documentation is available from the `Twisted Matrix website `_. This documentation contains how-tos, code examples, and an API reference. -Help is also available on the `Twisted mailing list `_. +Help is also available on the `Twisted mailing list `_. There is also a pair of very lively IRC channels, ``#twisted`` (for general Twisted questions) and ``#twisted.web`` (for Twisted Web), on ``chat.freenode.net``. From d29ec93adcb7079ecff9f27eade642861c4b9df2 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 23:11:59 -0700 Subject: [PATCH 06/13] Use HTTPS links in setup.py --- src/twisted/python/_setup.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/twisted/python/_setup.py b/src/twisted/python/_setup.py index d3e3786d02d..f71a8ab2d74 100644 --- a/src/twisted/python/_setup.py +++ b/src/twisted/python/_setup.py @@ -56,7 +56,7 @@ author_email="twisted-python@twistedmatrix.com", maintainer="Glyph Lefkowitz", maintainer_email="glyph@twistedmatrix.com", - url="http://twistedmatrix.com/", + url="https://twistedmatrix.com/", license="MIT", long_description="""\ An extensible framework for Python programming, with special focus @@ -307,7 +307,7 @@ def prepare_extensions(self): # _XOPEN_SOURCE_EXTENDED macros to build in order to gain access to # the msg_control, msg_controllen, and msg_flags members in # sendmsg.c. (according to - # http://stackoverflow.com/questions/1034587). See the documentation + # https://stackoverflow.com/questions/1034587). See the documentation # of X/Open CAE in the standards(5) man page of Solaris. if sys.platform.startswith('sunos'): self.define_macros.append(('_XOPEN_SOURCE', 1)) From 9d247119b5d4fd21a9edd06e9fcfc035a1da4235 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 23:17:51 -0700 Subject: [PATCH 07/13] Add project_urls --- src/twisted/python/_setup.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/twisted/python/_setup.py b/src/twisted/python/_setup.py index f71a8ab2d74..5c279007da8 100644 --- a/src/twisted/python/_setup.py +++ b/src/twisted/python/_setup.py @@ -57,6 +57,11 @@ maintainer="Glyph Lefkowitz", maintainer_email="glyph@twistedmatrix.com", url="https://twistedmatrix.com/", + project_urls={ + 'Documentation': 'https://twistedmatrix.com/documents/current/', + 'Source': 'https://github.com/twisted/twisted', + 'Issues': 'https://twistedmatrix.com/trac/report', + }, license="MIT", long_description="""\ An extensible framework for Python programming, with special focus From 688325e95a0032e0f8d015993a54cfd3fcc16546 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 23:23:40 -0700 Subject: [PATCH 08/13] Remove obsolete appveyor badge --- README.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/README.rst b/README.rst index ea489d8f572..7c02996faf3 100644 --- a/README.rst +++ b/README.rst @@ -4,7 +4,6 @@ Twisted |pypi|_ |coverage|_ |travis|_ -|appveyor|_ |circleci|_ For information on what's new in Twisted 19.2.0, see the `NEWS `_ file that comes with the distribution. @@ -105,8 +104,5 @@ Again, see the included `LICENSE `_ file for specific legal details. .. |travis| image:: https://travis-ci.org/twisted/twisted.svg?branch=trunk .. _travis: https://travis-ci.org/twisted/twisted -.. |appveyor| image:: https://ci.appveyor.com/api/projects/status/x4oyqtl9cqc2i2l8 -.. _appveyor: https://ci.appveyor.com/project/adiroiban/twisted - .. |circleci| image:: https://circleci.com/gh/twisted/twisted.svg?style=svg .. _circleci: https://circleci.com/gh/twisted/twisted From 106374f6f6d09c136d7a5509b68cd0d04b6f5e38 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Mon, 3 Jun 2019 23:24:24 -0700 Subject: [PATCH 09/13] Remove obsolete codecov badge --- README.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/README.rst b/README.rst index 7c02996faf3..bbb938fd896 100644 --- a/README.rst +++ b/README.rst @@ -2,7 +2,6 @@ Twisted ======= |pypi|_ -|coverage|_ |travis|_ |circleci|_ @@ -95,9 +94,6 @@ Warranty Again, see the included `LICENSE `_ file for specific legal details. -.. |coverage| image:: https://codecov.io/github/twisted/twisted/coverage.svg?branch=trunk -.. _coverage: https://codecov.io/github/twisted/twisted - .. |pypi| image:: http://img.shields.io/pypi/v/twisted.svg .. _pypi: https://pypi.python.org/pypi/twisted From 4e919ebd8655ddea962d8241f4c131e0c0fe0fa5 Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 4 Jun 2019 14:38:38 -0700 Subject: [PATCH 10/13] Generate long_description from README.rst --- src/twisted/python/_setup.py | 51 ++++++++++++++++++++++---- src/twisted/python/test/test_setup.py | 52 ++++++++++++++++++++++----- tox.ini | 6 +++- 3 files changed, 94 insertions(+), 15 deletions(-) diff --git a/src/twisted/python/_setup.py b/src/twisted/python/_setup.py index 5c279007da8..ae7c166656f 100644 --- a/src/twisted/python/_setup.py +++ b/src/twisted/python/_setup.py @@ -33,8 +33,10 @@ @var notPortedModules: Modules that are not yet ported to Python 3. """ +import io import os import platform +import re import sys from distutils.command import build_ext @@ -63,10 +65,6 @@ 'Issues': 'https://twistedmatrix.com/trac/report', }, license="MIT", - long_description="""\ -An extensible framework for Python programming, with special focus -on event-based network programming and multiprotocol integration. -""", classifiers=[ "Programming Language :: Python :: 2.7", "Programming Language :: Python :: 3", @@ -211,15 +209,56 @@ def _checkPythonVersion(): -def getSetupArgs(extensions=_EXTENSIONS): +def _longDescriptionArgsFromReadme(readme): """ + Generate a PyPI long description from the readme. - @return: The keyword arguments to be used the the setup method. + @param readme: Path to the readme reStructuredText file. + @type readme: C{str} + + @return: Keyword arguments to be passed to C{setuptools.setup()}. + @rtype: C{str} + """ + with io.open(readme, encoding='utf-8') as f: + readmeRst = f.read() + + # Munge links of the form `NEWS `_ to point at the appropriate + # location on GitHub so that they function when the long description is + # displayed on PyPI. + longDesc = re.sub( + r'`([^`]+)\s+<(?!https?://)([^>]+)>`_', + r'`\1 `_', + readmeRst, + flags=re.I, + ) + + return { + 'long_description': longDesc, + 'long_description_content_type': 'text/x-rst', + } + + + +def getSetupArgs(extensions=_EXTENSIONS, readme='README.rst'): + """ + Generate arguments for C{setuptools.setup()} + + @param extensions: C extension modules to maybe build. This argument is to + be used for testing. + @type extensions: C{list} of C{ConditionalExtension} + + @param readme: Path to the readme reStructuredText file. This argument is + to be used for testing. + @type readme: C{str} + + @return: The keyword arguments to be used by the setup method. @rtype: L{dict} """ _checkPythonVersion() arguments = STATIC_PACKAGE_METADATA.copy() + if readme: + arguments.update(_longDescriptionArgsFromReadme(readme)) # This is a workaround for distutils behavior; ext_modules isn't # actually used by our custom builder. distutils deep-down checks diff --git a/src/twisted/python/test/test_setup.py b/src/twisted/python/test/test_setup.py index 4fa01426cbc..2df8a382d9b 100644 --- a/src/twisted/python/test/test_setup.py +++ b/src/twisted/python/test/test_setup.py @@ -8,24 +8,24 @@ import os - from pkg_resources import parse_requirements from setuptools.dist import Distribution import twisted -from twisted.trial.unittest import TestCase +from twisted.trial.unittest import SynchronousTestCase from twisted.python import _setup, filepath from twisted.python.compat import _PY3 from twisted.python._setup import ( BuildPy3, getSetupArgs, + _longDescriptionArgsFromReadme, ConditionalExtension, _EXTRAS_REQUIRE, ) -class SetupTests(TestCase): +class SetupTests(SynchronousTestCase): """ Tests for L{getSetupArgs}. """ @@ -40,7 +40,7 @@ def test_conditionalExtensions(self): bad_ext = ConditionalExtension("whatever", ["whatever.c"], condition=lambda b: False) - args = getSetupArgs(extensions=[good_ext, bad_ext]) + args = getSetupArgs(extensions=[good_ext, bad_ext], readme=None) # ext_modules should be set even though it's not used. See comment # in getSetupArgs @@ -60,7 +60,7 @@ def test_win32Definition(self): ext = ConditionalExtension("whatever", ["whatever.c"], define_macros=[("whatever", 2)]) - args = getSetupArgs(extensions=[ext]) + args = getSetupArgs(extensions=[ext], readme=None) builder = args["cmdclass"]["build_ext"](Distribution()) self.patch(os, "name", "nt") @@ -69,7 +69,7 @@ def test_win32Definition(self): -class OptionalDependenciesTests(TestCase): +class OptionalDependenciesTests(SynchronousTestCase): """ Tests for L{_EXTRAS_REQUIRE} """ @@ -295,7 +295,7 @@ def __getattr__(self, name): -class WithPlatformTests(TestCase): +class WithPlatformTests(SynchronousTestCase): """ Tests for L{_checkCPython} when used with a (fake) C{platform} module. """ @@ -316,7 +316,7 @@ def test_other(self): -class BuildPy3Tests(TestCase): +class BuildPy3Tests(SynchronousTestCase): """ Tests for L{BuildPy3}. """ @@ -363,3 +363,39 @@ def test_find_package_modules(self): ]), sorted(result), ) + + + +class LongDescriptionTests(SynchronousTestCase): + """ + Tests for C{_getLongDescriptionArgs()} + + Note that the validity of the reStructuredText syntax is tested separately + using L{twine check} in L{tox.ini}. + """ + def test_generate(self): + """ + L{_longDescriptionArgsFromReadme()} outputs a L{long_description} in + reStructuredText format. Local links are transformed into absolute ones + that point at the Twisted GitHub repository. + """ + path = self.mktemp() + with open(path, 'w') as f: + f.write('\n'.join([ + 'Twisted', + '=======', + '', + 'Changes: `NEWS `_.', + "Read `the docs `_.\n", + ])) + + self.assertEqual({ + 'long_description': '''\ +Twisted +======= + +Changes: `NEWS `_. +Read `the docs `_. +''', + 'long_description_content_type': 'text/x-rst', + }, _longDescriptionArgsFromReadme(path)) diff --git a/tox.ini b/tox.ini index 428005eca82..2a89cce4caf 100644 --- a/tox.ini +++ b/tox.ini @@ -28,7 +28,7 @@ minversion=2.4 skip_missing_interpreters=True toxworkdir=build/ -envlist=lint,pyflakes,apidocs,narrativedocs,newsfragment,manifest-checker,py27-alldeps-nocov,py36-alldeps-nocov +envlist=lint,pyflakes,apidocs,narrativedocs,newsfragment,manifest-checker,twine,py27-alldeps-nocov,py36-alldeps-nocov [testenv] ;; dependencies managed by extras in t.p._setup.py._EXTRAS_REQUIRE @@ -60,6 +60,8 @@ deps = ; Code quality checkers manifest-checker: check-manifest + twine: twine + lint: pyflakes lint: twistedchecker>=0.7.1 lint: diff-cover==0.9.12 @@ -131,6 +133,8 @@ commands = manifest-checker: check-manifest --ignore "docs/_build*,docs/historic/*,admin*,bin/admin*,twisted/topfiles/*.Old" + twine: twine check {distdir}/*.* + wheels: cibuildwheel --output-dir {toxinidir}/wheelhouse {toxinidir} wheels: python setup.py sdist --formats=gztar,zip --dist-dir={toxinidir}/wheelhouse wheels: ls -l {toxinidir}/wheelhouse From 0f02a46b6db012804d9409aed5d220d68905f0ba Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 4 Jun 2019 14:51:15 -0700 Subject: [PATCH 11/13] Add newsfragment --- src/twisted/newsfragments/9648.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 src/twisted/newsfragments/9648.feature diff --git a/src/twisted/newsfragments/9648.feature b/src/twisted/newsfragments/9648.feature new file mode 100644 index 00000000000..113ddb2cba2 --- /dev/null +++ b/src/twisted/newsfragments/9648.feature @@ -0,0 +1 @@ +The PyPI page for Twisted has been enhanced to include more information and useful links. From 131c2e1b8a02bc4ff107aa306a21910fbc39604a Mon Sep 17 00:00:00 2001 From: Tom Most Date: Tue, 4 Jun 2019 14:52:49 -0700 Subject: [PATCH 12/13] Fix lint failure --- src/twisted/python/test/test_setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/twisted/python/test/test_setup.py b/src/twisted/python/test/test_setup.py index 2df8a382d9b..0cad6f87288 100644 --- a/src/twisted/python/test/test_setup.py +++ b/src/twisted/python/test/test_setup.py @@ -38,7 +38,7 @@ def test_conditionalExtensions(self): good_ext = ConditionalExtension("whatever", ["whatever.c"], condition=lambda b: True) bad_ext = ConditionalExtension("whatever", ["whatever.c"], - condition=lambda b: False) + condition=lambda b: False) args = getSetupArgs(extensions=[good_ext, bad_ext], readme=None) From 6c61fc4503ae39ab8ecee52d10f10ee2c371d7e2 Mon Sep 17 00:00:00 2001 From: Mark Williams Date: Wed, 5 Jun 2019 00:03:37 -0700 Subject: [PATCH 13/13] Prevent CRLF injections described in CVE-2019-12387 Author: markrwilliams Reviewers: glyph Fixes: ticket:9647 Twisted's HTTP client APIs were vulnerable to maliciously constructed HTTP methods, hosts, and/or paths, URI components such as paths and query parameters. These vulnerabilities were beyond the header name and value injection vulnerabilities addressed in: https://twistedmatrix.com/trac/ticket/9420 https://github.com/twisted/twisted/pull/999/ The following client APIs will raise a ValueError if given a method, host, or URI that includes newlines or other disallowed characters: - twisted.web.client.Agent.request - twisted.web.client.ProxyAgent.request - twisted.web.client.Request.__init__ - twisted.web.client.Request.writeTo ProxyAgent is patched separately from Agent because unlike other agents (e.g. CookieAgent) it is not implemented as an Agent wrapper. Request.__init__ checks its method and URI so that errors occur closer to their originating input. Request.method and Request.uri are both public APIs, however, so Request.writeTo (via Request._writeHeaders) also checks the validity of both before writing anything to the wire. Additionally, the following deprecated client APIs have also been patched: - twisted.web.client.HTTPPageGetter.__init__ - twisted.web.client.HTTPPageDownloader.__init__ - twisted.web.client.HTTPClientFactory.__init__ - twisted.web.client.HTTPClientFactory.setURL - twisted.web.client.HTTPDownloader.__init__ - twisted.web.client.HTTPDownloader.setURL - twisted.web.client.getPage - twisted.web.client.downloadPage These have been patched prior to their removal so that they won't be vulnerable in the last Twisted release that includes them. They represent a best effort, because testing every combination of these public APIs would require more code than deprecated APIs warrant. In all cases URI components, including hostnames, are restricted to the characters allowed in path components. This mirrors the CPython patch (for bpo-30458) that addresses equivalent vulnerabilities: https://github.com/python/cpython/commit/bb8071a4cae5ab3fe321481dd3d73662ffb26052 HTTP methods, however, are checked against the set of characters described in RFC-7230. --- src/twisted/web/_newclient.py | 85 +++++- src/twisted/web/client.py | 22 +- src/twisted/web/newsfragments/9647.bugfix | 1 + src/twisted/web/test/injectionhelpers.py | 168 ++++++++++++ src/twisted/web/test/test_agent.py | 147 +++++++++- src/twisted/web/test/test_webclient.py | 313 +++++++++++++++++++++- 6 files changed, 725 insertions(+), 11 deletions(-) create mode 100644 src/twisted/web/newsfragments/9647.bugfix create mode 100644 src/twisted/web/test/injectionhelpers.py diff --git a/src/twisted/web/_newclient.py b/src/twisted/web/_newclient.py index 370f47d574d..74a8a6c2d1f 100644 --- a/src/twisted/web/_newclient.py +++ b/src/twisted/web/_newclient.py @@ -29,6 +29,8 @@ from __future__ import division, absolute_import __metaclass__ = type +import re + from zope.interface import implementer from twisted.python.compat import networkString @@ -579,6 +581,74 @@ def connectionLost(self, reason): +_VALID_METHOD = re.compile( + br"\A[%s]+\Z" % ( + bytes().join( + ( + b"!", b"#", b"$", b"%", b"&", b"'", b"*", + b"+", b"-", b".", b"^", b"_", b"`", b"|", b"~", + b"\x30-\x39", + b"\x41-\x5a", + b"\x61-\x7A", + ), + ), + ), +) + + + +def _ensureValidMethod(method): + """ + An HTTP method is an HTTP token, which consists of any visible + ASCII character that is not a delimiter (i.e. one of + C{"(),/:;<=>?@[\\]{}}.) + + @param method: the method to check + @type method: L{bytes} + + @return: the method if it is valid + @rtype: L{bytes} + + @raise ValueError: if the method is not valid + + @see: U{https://tools.ietf.org/html/rfc7230#section-3.1.1}, + U{https://tools.ietf.org/html/rfc7230#section-3.2.6}, + U{https://tools.ietf.org/html/rfc5234#appendix-B.1} + """ + if _VALID_METHOD.match(method): + return method + raise ValueError("Invalid method {!r}".format(method)) + + + +_VALID_URI = re.compile(br'\A[\x21-\x7e]+\Z') + + + +def _ensureValidURI(uri): + """ + A valid URI cannot contain control characters (i.e., characters + between 0-32, inclusive and 127) or non-ASCII characters (i.e., + characters with values between 128-255, inclusive). + + @param uri: the URI to check + @type uri: L{bytes} + + @return: the URI if it is valid + @rtype: L{bytes} + + @raise ValueError: if the URI is not valid + + @see: U{https://tools.ietf.org/html/rfc3986#section-3.3}, + U{https://tools.ietf.org/html/rfc3986#appendix-A}, + U{https://tools.ietf.org/html/rfc5234#appendix-B.1} + """ + if _VALID_URI.match(uri): + return uri + raise ValueError("Invalid URI {!r}".format(uri)) + + + @implementer(IClientRequest) class Request: """ @@ -618,8 +688,8 @@ def __init__(self, method, uri, headers, bodyProducer, persistent=False): connection, defaults to C{False}. @type persistent: L{bool} """ - self.method = method - self.uri = uri + self.method = _ensureValidMethod(method) + self.uri = _ensureValidURI(uri) self.headers = headers self.bodyProducer = bodyProducer self.persistent = persistent @@ -664,8 +734,15 @@ def _writeHeaders(self, transport, TEorCL): # method would probably be good. It would be nice if this method # weren't limited to issuing HTTP/1.1 requests. requestLines = [] - requestLines.append(b' '.join([self.method, self.uri, - b'HTTP/1.1\r\n'])) + requestLines.append( + b' '.join( + [ + _ensureValidMethod(self.method), + _ensureValidURI(self.uri), + b'HTTP/1.1\r\n', + ] + ), + ) if not self.persistent: requestLines.append(b'Connection: close\r\n') if TEorCL is not None: diff --git a/src/twisted/web/client.py b/src/twisted/web/client.py index e1a92fef4b2..340906f6215 100644 --- a/src/twisted/web/client.py +++ b/src/twisted/web/client.py @@ -47,6 +47,9 @@ def urlunparse(parts): from twisted.web.http_headers import Headers from twisted.logger import Logger +from twisted.web._newclient import _ensureValidURI, _ensureValidMethod + + class PartialDownloadError(error.Error): """ @@ -78,11 +81,13 @@ class HTTPPageGetter(http.HTTPClient): _completelyDone = True - _specialHeaders = set((b'host', b'user-agent', b'cookie', b'content-length')) + _specialHeaders = set( + (b'host', b'user-agent', b'cookie', b'content-length'), + ) def connectionMade(self): - method = getattr(self.factory, 'method', b'GET') - self.sendCommand(method, self.factory.path) + method = _ensureValidMethod(getattr(self.factory, 'method', b'GET')) + self.sendCommand(method, _ensureValidURI(self.factory.path)) if self.factory.scheme == b'http' and self.factory.port != 80: host = self.factory.host + b':' + intToBytes(self.factory.port) elif self.factory.scheme == b'https' and self.factory.port != 443: @@ -362,7 +367,7 @@ def __init__(self, url, method=b'GET', postdata=None, headers=None, # just in case a broken http/1.1 decides to keep connection alive self.headers.setdefault(b"connection", b"close") self.postdata = postdata - self.method = method + self.method = _ensureValidMethod(method) self.setURL(url) @@ -389,6 +394,7 @@ def __repr__(self): return "<%s: %s>" % (self.__class__.__name__, self.url) def setURL(self, url): + _ensureValidURI(url.strip()) self.url = url uri = URI.fromBytes(url) if uri.scheme and uri.host: @@ -733,7 +739,7 @@ def _makeGetterFactory(url, factoryFactory, contextFactory=None, @return: The factory created by C{factoryFactory} """ - uri = URI.fromBytes(url) + uri = URI.fromBytes(_ensureValidURI(url.strip())) factory = factoryFactory(url, *args, **kwargs) if uri.scheme == b'https': from twisted.internet import ssl @@ -1493,6 +1499,9 @@ def _requestWithEndpoint(self, key, endpoint, method, parsedURI, if not isinstance(method, bytes): raise TypeError('method={!r} is {}, but must be bytes'.format( method, type(method))) + + method = _ensureValidMethod(method) + # Create minimal headers, if necessary: if headers is None: headers = Headers() @@ -1717,6 +1726,7 @@ def request(self, method, uri, headers=None, bodyProducer=None): @see: L{twisted.web.iweb.IAgent.request} """ + uri = _ensureValidURI(uri.strip()) parsedURI = URI.fromBytes(uri) try: endpoint = self._getEndpoint(parsedURI) @@ -1750,6 +1760,8 @@ def request(self, method, uri, headers=None, bodyProducer=None): """ Issue a new request via the configured proxy. """ + uri = _ensureValidURI(uri.strip()) + # Cache *all* connections under the same key, since we are only # connecting to a single destination, the proxy: key = ("http-proxy", self._proxyEndpoint) diff --git a/src/twisted/web/newsfragments/9647.bugfix b/src/twisted/web/newsfragments/9647.bugfix new file mode 100644 index 00000000000..b76916cdc9e --- /dev/null +++ b/src/twisted/web/newsfragments/9647.bugfix @@ -0,0 +1 @@ +All HTTP clients in twisted.web.client now raise a ValueError when called with a method and/or URL that contain invalid characters. This mitigates CVE-2019-12387. Thanks to Alex Brasetvik for reporting this vulnerability. \ No newline at end of file diff --git a/src/twisted/web/test/injectionhelpers.py b/src/twisted/web/test/injectionhelpers.py new file mode 100644 index 00000000000..ffeb8629c42 --- /dev/null +++ b/src/twisted/web/test/injectionhelpers.py @@ -0,0 +1,168 @@ +""" +Helpers for URI and method injection tests. + +@see: U{CVE-2019-12387} +""" + +import string + + +UNPRINTABLE_ASCII = ( + frozenset(range(0, 128)) - + frozenset(bytearray(string.printable, 'ascii')) +) + +NONASCII = frozenset(range(128, 256)) + + + +class MethodInjectionTestsMixin(object): + """ + A mixin that runs HTTP method injection tests. Define + L{MethodInjectionTestsMixin.attemptRequestWithMaliciousMethod} in + a L{twisted.trial.unittest.SynchronousTestCase} subclass to test + how HTTP client code behaves when presented with malicious HTTP + methods. + + @see: U{CVE-2019-12387} + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt to send a request with the given method. This should + synchronously raise a L{ValueError} if either is invalid. + + @param method: the method (e.g. C{GET\x00}) + + @param uri: the URI + + @type method: + """ + raise NotImplementedError() + + + def test_methodWithCLRFRejected(self): + """ + Issuing a request with a method that contains a carriage + return and line feed fails with a L{ValueError}. + """ + with self.assertRaises(ValueError) as cm: + method = b"GET\r\nX-Injected-Header: value" + self.attemptRequestWithMaliciousMethod(method) + self.assertRegex(str(cm.exception), "^Invalid method") + + + def test_methodWithUnprintableASCIIRejected(self): + """ + Issuing a request with a method that contains unprintable + ASCII characters fails with a L{ValueError}. + """ + for c in UNPRINTABLE_ASCII: + method = b"GET%s" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousMethod(method) + self.assertRegex(str(cm.exception), "^Invalid method") + + + def test_methodWithNonASCIIRejected(self): + """ + Issuing a request with a method that contains non-ASCII + characters fails with a L{ValueError}. + """ + for c in NONASCII: + method = b"GET%s" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousMethod(method) + self.assertRegex(str(cm.exception), "^Invalid method") + + + +class URIInjectionTestsMixin(object): + """ + A mixin that runs HTTP URI injection tests. Define + L{MethodInjectionTestsMixin.attemptRequestWithMaliciousURI} in a + L{twisted.trial.unittest.SynchronousTestCase} subclass to test how + HTTP client code behaves when presented with malicious HTTP + URIs. + """ + + def attemptRequestWithMaliciousURI(self, method): + """ + Attempt to send a request with the given URI. This should + synchronously raise a L{ValueError} if either is invalid. + + @param uri: the URI. + + @type method: + """ + raise NotImplementedError() + + + def test_hostWithCRLFRejected(self): + """ + Issuing a request with a URI whose host contains a carriage + return and line feed fails with a L{ValueError}. + """ + with self.assertRaises(ValueError) as cm: + uri = b"http://twisted\r\n.invalid/path" + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") + + + def test_hostWithWithUnprintableASCIIRejected(self): + """ + Issuing a request with a URI whose host contains unprintable + ASCII characters fails with a L{ValueError}. + """ + for c in UNPRINTABLE_ASCII: + uri = b"http://twisted%s.invalid/OK" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") + + + def test_hostWithNonASCIIRejected(self): + """ + Issuing a request with a URI whose host contains non-ASCII + characters fails with a L{ValueError}. + """ + for c in NONASCII: + uri = b"http://twisted%s.invalid/OK" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") + + + def test_pathWithCRLFRejected(self): + """ + Issuing a request with a URI whose path contains a carriage + return and line feed fails with a L{ValueError}. + """ + with self.assertRaises(ValueError) as cm: + uri = b"http://twisted.invalid/\r\npath" + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") + + + def test_pathWithWithUnprintableASCIIRejected(self): + """ + Issuing a request with a URI whose path contains unprintable + ASCII characters fails with a L{ValueError}. + """ + for c in UNPRINTABLE_ASCII: + uri = b"http://twisted.invalid/OK%s" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") + + + def test_pathWithNonASCIIRejected(self): + """ + Issuing a request with a URI whose path contains non-ASCII + characters fails with a L{ValueError}. + """ + for c in NONASCII: + uri = b"http://twisted.invalid/OK%s" % (bytearray([c]),) + with self.assertRaises(ValueError) as cm: + self.attemptRequestWithMaliciousURI(uri) + self.assertRegex(str(cm.exception), "^Invalid URI") diff --git a/src/twisted/web/test/test_agent.py b/src/twisted/web/test/test_agent.py index fc67d154198..040dc9ef340 100644 --- a/src/twisted/web/test/test_agent.py +++ b/src/twisted/web/test/test_agent.py @@ -11,7 +11,7 @@ from zope.interface.verify import verifyObject -from twisted.trial.unittest import TestCase +from twisted.trial.unittest import TestCase, SynchronousTestCase from twisted.web import client, error, http_headers from twisted.web._newclient import RequestNotSent, RequestTransmissionFailed from twisted.web._newclient import ResponseNeverReceived, ResponseFailed @@ -51,6 +51,10 @@ from twisted.test.proto_helpers import AccumulatingProtocol from twisted.test.iosim import IOPump, FakeTransport from twisted.test.test_sslverify import certificatesForAuthorityAndServer +from twisted.web.test.injectionhelpers import ( + MethodInjectionTestsMixin, + URIInjectionTestsMixin, +) from twisted.web.error import SchemeNotSupported from twisted.logger import globalLogPublisher @@ -897,6 +901,7 @@ class AgentTests(TestCase, FakeReactorAndConnectMixin, AgentTestsMixin, """ Tests for the new HTTP client API provided by L{Agent}. """ + def makeAgent(self): """ @return: a new L{twisted.web.client.Agent} instance @@ -1327,6 +1332,48 @@ def test_endpointFactoryPool(self): +class AgentMethodInjectionTests( + FakeReactorAndConnectMixin, + MethodInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Agent} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: see L{MethodInjectionTestsMixin} + """ + agent = client.Agent(self.createReactor()) + uri = b"http://twisted.invalid" + agent.request(method, uri, client.Headers(), None) + + + +class AgentURIInjectionTests( + FakeReactorAndConnectMixin, + URIInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Agent} against URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided method. + + @param uri: see L{URIInjectionTestsMixin} + """ + agent = client.Agent(self.createReactor()) + method = b"GET" + agent.request(method, uri, client.Headers(), None) + + + class AgentHTTPSTests(TestCase, FakeReactorAndConnectMixin, IntegrationTestingMixin): """ @@ -3202,3 +3249,101 @@ def test_changeCacheSize(self): self.assertEquals(5, len(policy._cache)) self.assertIn(hostn, policy._cache) + + + +class RequestMethodInjectionTests( + MethodInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Request} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: see L{MethodInjectionTestsMixin} + """ + client.Request( + method=method, + uri=b"http://twisted.invalid", + headers=http_headers.Headers(), + bodyProducer=None, + ) + + + +class RequestWriteToMethodInjectionTests( + MethodInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Request.writeTo} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: see L{MethodInjectionTestsMixin} + """ + headers = http_headers.Headers({b"Host": [b"twisted.invalid"]}) + req = client.Request( + method=b"GET", + uri=b"http://twisted.invalid", + headers=headers, + bodyProducer=None, + ) + req.method = method + req.writeTo(StringTransport()) + + + +class RequestURIInjectionTests( + URIInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Request} against HTTP URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param method: see L{URIInjectionTestsMixin} + """ + client.Request( + method=b"GET", + uri=uri, + headers=http_headers.Headers(), + bodyProducer=None, + ) + + + +class RequestWriteToURIInjectionTests( + URIInjectionTestsMixin, + SynchronousTestCase, +): + """ + Test L{client.Request.writeTo} against HTTP method injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided method. + + @param method: see L{URIInjectionTestsMixin} + """ + headers = http_headers.Headers({b"Host": [b"twisted.invalid"]}) + req = client.Request( + method=b"GET", + uri=b"http://twisted.invalid", + headers=headers, + bodyProducer=None, + ) + req.uri = uri + req.writeTo(StringTransport()) diff --git a/src/twisted/web/test/test_webclient.py b/src/twisted/web/test/test_webclient.py index 41cff5407a0..680e02780d6 100644 --- a/src/twisted/web/test/test_webclient.py +++ b/src/twisted/web/test/test_webclient.py @@ -7,6 +7,7 @@ from __future__ import division, absolute_import +import io import os from errno import ENOSPC @@ -20,7 +21,8 @@ from twisted.web import server, client, error, resource from twisted.web.static import Data from twisted.web.util import Redirect -from twisted.internet import reactor, defer, interfaces +from twisted.internet import address, reactor, defer, interfaces +from twisted.internet.protocol import ClientFactory from twisted.python.filepath import FilePath from twisted.protocols.policies import WrappingFactory from twisted.test.proto_helpers import ( @@ -35,6 +37,12 @@ from twisted.logger import (globalLogPublisher, FilteringLogObserver, LogLevelFilterPredicate, LogLevel, Logger) +from twisted.web.test.injectionhelpers import ( + MethodInjectionTestsMixin, + URIInjectionTestsMixin, +) + + serverPEM = FilePath(test.__file__).sibling('server.pem') serverPEMPath = serverPEM.asBytesMode().path @@ -1519,3 +1527,306 @@ def test_httpDownloaderDeprecated(self): L{client.HTTPDownloader} is deprecated. """ self._testDeprecatedClass("HTTPDownloader") + + + +class GetPageMethodInjectionTests( + MethodInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.getPage} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: see L{MethodInjectionTestsMixin} + """ + uri = b'http://twisted.invalid' + client.getPage(uri, method=method) + + + +class GetPageURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.getPage} against URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: see L{URIInjectionTestsMixin} + """ + client.getPage(uri) + + + +class DownloadPageMethodInjectionTests( + MethodInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.getPage} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: see L{MethodInjectionTestsMixin} + """ + uri = b'http://twisted.invalid' + client.downloadPage(uri, file=io.BytesIO(), method=method) + + + +class DownloadPageURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.downloadPage} against URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: see L{URIInjectionTestsMixin} + """ + client.downloadPage(uri, file=io.BytesIO()) + + + +def makeHTTPPageGetterFactory(protocolClass, method, host, path): + """ + Make a L{ClientFactory} that can be used with + L{client.HTTPPageGetter} and its subclasses. + + @param protocolClass: The protocol class + @type protocolClass: A subclass of L{client.HTTPPageGetter} + + @param method: the HTTP method + + @param host: the host + + @param path: The URI path + + @return: A L{ClientFactory}. + """ + factory = ClientFactory.forProtocol(protocolClass) + + factory.method = method + factory.host = host + factory.path = path + + factory.scheme = b"http" + factory.port = 0 + factory.headers = {} + factory.agent = b"User/Agent" + factory.cookies = {} + + return factory + + + +class HTTPPageGetterMethodInjectionTests( + MethodInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.HTTPPageGetter} against HTTP method injections. + """ + protocolClass = client.HTTPPageGetter + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: L{MethodInjectionTestsMixin} + """ + transport = StringTransport() + factory = makeHTTPPageGetterFactory( + self.protocolClass, + method=method, + host=b"twisted.invalid", + path=b"/", + ) + getter = factory.buildProtocol( + address.IPv4Address("TCP", "127.0.0.1", 0), + ) + getter.makeConnection(transport) + + + +class HTTPPageGetterURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Test L{client.HTTPPageGetter} against HTTP URI injections. + """ + protocolClass = client.HTTPPageGetter + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: L{URIInjectionTestsMixin} + """ + transport = StringTransport() + # Setting the host and path to the same value is imprecise but + # doesn't require parsing an invalid URI. + factory = makeHTTPPageGetterFactory( + self.protocolClass, + method=b"GET", + host=uri, + path=uri, + ) + getter = factory.buildProtocol( + address.IPv4Address("TCP", "127.0.0.1", 0), + ) + getter.makeConnection(transport) + + + +class HTTPPageDownloaderMethodInjectionTests( + HTTPPageGetterMethodInjectionTests +): + + """ + Test L{client.HTTPPageDownloader} against HTTP method injections. + """ + protocolClass = client.HTTPPageDownloader + + + +class HTTPPageDownloaderURIInjectionTests( + HTTPPageGetterURIInjectionTests +): + """ + Test L{client.HTTPPageDownloader} against HTTP URI injections. + """ + protocolClass = client.HTTPPageDownloader + + + +class HTTPClientFactoryMethodInjectionTests( + MethodInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPClientFactory} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: L{MethodInjectionTestsMixin} + """ + client.HTTPClientFactory(b"https://twisted.invalid", method) + + + +class HTTPClientFactoryURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPClientFactory} against HTTP URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: L{URIInjectionTestsMixin} + """ + client.HTTPClientFactory(uri) + + + +class HTTPClientFactorySetURLURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPClientFactory.setURL} against HTTP URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: L{URIInjectionTestsMixin} + """ + client.HTTPClientFactory(b"https://twisted.invalid").setURL(uri) + + + +class HTTPDownloaderMethodInjectionTests( + MethodInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPDownloader} against HTTP method injections. + """ + + def attemptRequestWithMaliciousMethod(self, method): + """ + Attempt a request with the provided method. + + @param method: L{MethodInjectionTestsMixin} + """ + client.HTTPDownloader( + b"https://twisted.invalid", + io.BytesIO(), + method=method, + ) + + + +class HTTPDownloaderURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPDownloader} against HTTP URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: L{URIInjectionTestsMixin} + """ + client.HTTPDownloader(uri, io.BytesIO()) + + + +class HTTPDownloaderSetURLURIInjectionTests( + URIInjectionTestsMixin, + unittest.SynchronousTestCase, +): + """ + Tests L{client.HTTPDownloader.setURL} against HTTP URI injections. + """ + + def attemptRequestWithMaliciousURI(self, uri): + """ + Attempt a request with the provided URI. + + @param uri: L{URIInjectionTestsMixin} + """ + downloader = client.HTTPDownloader( + b"https://twisted.invalid", + io.BytesIO(), + ) + downloader.setURL(uri)