-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 #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: python/cpython@bb8071a HTTP methods, however, are checked against the set of characters described in RFC-7230.
- Loading branch information
1 parent
d0bcf0b
commit 6c61fc4
Showing
6 changed files
with
725 additions
and
11 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") |
Oops, something went wrong.