New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
http.client cannot send non-ASCII request lines #80455
Comments
While the RFCs are rather clear that non-ASCII data would be out of spec,
The two most-obvious ways (to me, anyway) to try to send a request for /你好 (for example) are # Assume it will get UTF-8 encoded, as that's the default encoding
# for urllib.parse.quote()
conn.putrequest('GET', '/\u4f60\u597d')
# Assume it will get Latin-1 encoded, as
# * that's the encoding used in http.client.parse_headers(),
# * that's the encoding used for PEP-3333, and
# * it has a one-to-one mapping with bytes
conn.putrequest('GET', '/\xe4\xbd\xa0\xe5\xa5\xbd') both fail with something like
Trying to pre-encode like conn.putrequest('GET', b'/\xe4\xbd\xa0\xe5\xa5\xbd') at least doesn't raise an error, but still does not do what was intended; rather than a request line like
(or
depending on how you choose to interpret the bytes), the server gets
The trouble comes down to https://github.com/python/cpython/blob/v3.7.2/Lib/http/client.py#L1104-L1107 -- we don't actually have any control over what the caller passes as the url (so the assumption doesn't hold), nor do we know anything about the encoding that was *intended*. One of three fixes seems warranted:
|
Thank you Tim for the reasoned issue and proposed solutions. After reviewing these proposals with @eric.snow, we've decided that this approach is dangerous in that the proposed approaches has the potential to expose users unexpectedly to non-compliant behavior, where as currently they are assured compliance. Instead, we would like to see a more explicit opt-in, such as through a separate method or through a setting on the call and/or client object. Consider instead a solution that implements both Or consider another approach where HTTPConnection implements an Would either of those approaches suit your use-case? |
Fair enough. Seems kinda related to https://bugs.python.org/issue30458 -- looks like it was a fun one ;-) I think either approach would work for me; my existing work-around doesn't preclude either, particularly since I want it purely for testing purposes. For a bit of context, I work on a large-ish project (few hundred kloc if you include tests) that recently finished porting from python 2.7 to 3.6 & 3.7. As part of that process I discovered https://bugs.python.org/issue33973 and worked around it in openstack/swift@93b49c5. That was a prerequisite for running tests under py2 against a server running py3, but this bug complicated us running the *tests* under py3 as well. I eventually landed on openstack/swift@c0ae48b as a work-around. I could probably take another stab at a fix if you like, though I'm not entirely sure when I'll get to it at the moment. |
I believe this issue is more recent and widespread than I originally thought. I realized today that CherryPy is affected by this issue and disabled nightly tests as a result. It looks like the changed behavior was introduced in Python 3.7.4, so I expect stable releases to start failing also now. In that case, the web framework wishes to test that a null byte transmitted by the client is handled properly in the server, and without a patch, it's not possible for a project like cherrypy to transmit the invalid request to ensure a proper invalid response. I see now that the previously proposed solution wouldn't even address this use-case, as the null byte would still have been excluded. |
I've started drafting a patch at https://github.com/python/cpython/tree/feature/putrequest-hooks |
As I considered a patch for this, I realized there are actually two issues, the one in the title "http.client cannot send non-ASCII request lines" but also "the protection for invalid requests prevents usage to generate invalid requests". The former issue was new with Python 3 while the latter was introduced with bpo-30458. I can't decide if we should handle these two issues separately, or address them together in this ticket. |
Is there a reason why cherrypy doesn't URL encode the null byte in this test as per comment : cherrypy/cherrypy#1781 (comment) ? The original fix was adopted from golang (golang/go@829c5df) and as per the cherrypy test the golang client also should be forbidding invalid control character : https://play.golang.org/p/FTNtRmvlWrn Also as per the discussion one of our own stdlib tests were depending on this behavior in the http client and had to be changed : #12755 (comment) The change made in bpo-30458 also affects urllib3 test in 3.7.4 causing CI failure and the related discussion to encode URLs : urllib3/urllib3#1671 (comment) . urllib3 issue : urllib3/urllib3#1664 . IIRC urllib3 was also patched for this vulnerability in similar manner as part of larger refactor : urllib3/urllib3#1553 . I didn't verify yet, it's unreleased and how urllib3 client behaves before and after patch for the CRLF characters and similar tests that use urllib3 as client. Applying the URL encoding patch to cherrypy I can verify that the behavior is also tested diff --git a/cherrypy/test/test_static.py b/cherrypy/test/test_static.py def test_null_bytes(self):
- self.getPage('/static/\x00')
+ from urllib.parse import quote
+ self.getPage(quote('/static/\x00'))
self.assertStatus('404 Not Found')
After patch the test passes and ValueError is also raised properly as the null byte is decoded in the server and using it in os.stat to resolve null byte path. Breakages were expected in the discussion as adopting the fix from golang. Giving an option like a parameter to bypass the validation was also discussed in the linked but giving an option would also mean it could be abused or missed. |
@XTreak the encoded null-byte test would be an extra test case to consider. It is reasonable to test as many known invalid sequences as possible. Changing that byte to encoded notation would just replace one test with another effectively changing the semantics of it. To me, it's quite weird that it's considered a CVE at all: it's happening on the client side and it doesn't prevent the user from just feeding the proper bytes right into the socket so why overcomplicate things? |
I've created bpo-38216 to address the issue of sending invalid bytes in the request line, separate from the original intention and title issue about sending non-ASCII. |
Agreed that it's slightly different but I also admit I was also relying on the fact that encoded request will also be decoded by the server as done in most web frameworks to act upon the decoded payload. There could be systems where raw null byte might need to be transferred where the receiving system might not decode the payload but it seemed to be a workaround to ensure the required exception of serving null byte included static file was handled properly by CherryPy and also seemed to be adopted by others like urllib3.
The impacted function in http is used by functions like urllib.urlopen which is often feeded with input from the user that is not validated properly with some expectation that the function itself might handle the URL parsing and is safe enough. In this case it could lead to things like SSRF where malicious input could end up executing arbitrary code in some of the systems like Redis as demonstrated in the report. As for the CVE in question it was originally reported to golang and the same attack was also found out to be vulnerable. The fix adopted also seemed to have address few other security reports of similar nature. Thanks Jason, as bpo-38216 is now a separate issue we can discuss around there. I guess this issue then is not a 3.7 only regression anymore since the original issue reported predates 3.7. |
That's right - no longer a 3.7 regression here. |
I should say, though, this issue is a long-standing regression from Python 3.0. Although intentional, the inability for a client to override the encoding of the request line does make it impossible without replacing all of .putrequest to override that behavior, so although it would be a feature, it might justify a backport to supported bugfix Pythons to facilitate migration from Python 2. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: