http.client cannot send non-ASCII request lines

[tipabu]

BPO	36274
Nosy	@jaraco, @orsenthil, @larryhastings, @benjaminp, @ned-deily, @mcepl, @ericsnowcurrently, @webknjaz, @miss-islington, @tipabu, @tirkarthi
PRs	bpo-36274: Encode request lines as Latin-1 #12314 bpo-36274: Encode request lines with surrogate escapes #12315 bpo-38216, bpo-36274: Allow subclasses to override validation and encoding behavior #16321 bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior #16448 [3.8] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16460 [3.7] bpo-38216, bpo-36274: Allow subclasses to separately override v… #16461 [3.6] bpo-38216, bpo-36274: Allow subclasses to separately override v… #16462 [3.5] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16475 [2.7] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16476

^{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:

assignee = None
closed_at = <Date 2019-10-12.12:10:23.284>
created_at = <Date 2019-03-12.20:33:23.907>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'http.client cannot send non-ASCII request lines'
updated_at = <Date 2019-10-12.12:10:23.283>
user = 'https://github.com/tipabu'

bugs.python.org fields:

activity = <Date 2019-10-12.12:10:23.283>
actor = 'larry'
assignee = 'none'
closed = True
closed_date = <Date 2019-10-12.12:10:23.284>
closer = 'larry'
components = ['Library (Lib)']
creation = <Date 2019-03-12.20:33:23.907>
creator = 'tburke'
dependencies = []
files = []
hgrepos = []
issue_num = 36274
keywords = ['patch']
message_count = 19.0
messages = ['337802', '351834', '352023', '352340', '352342', '352452', '352597', '352711', '352725', '352739', '352743', '352750', '352980', '353448', '353450', '353454', '353465', '354160', '354226']
nosy_count = 11.0
nosy_names = ['jaraco', 'orsenthil', 'larry', 'benjamin.peterson', 'ned.deily', 'mcepl', 'eric.snow', 'webknjaz', 'miss-islington', 'tburke', 'xtreak']
pr_nums = ['12314', '12315', '16321', '16448', '16460', '16461', '16462', '16475', '16476']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36274'
versions = ['Python 3.7', 'Python 3.8']

[tipabu]

While the RFCs are rather clear that non-ASCII data would be out of spec,

• that doesn't prevent a poorly-behaved client from sending non-ASCII bytes on the wire, which means
• as an application developer, it's useful to be able to mimic such a client to verify expected behavior while still using stdlib to handle things like header parsing, particularly since
• this worked perfectly well on Python 2.

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

UnicodeEncodeError: 'ascii' codec can't encode characters in position ...

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

GET /你好 HTTP/1.1

(or

/你好

depending on how you choose to interpret the bytes), the server gets

GET b'/\xe4\xbd\xa0\xe5\xa5\xbd' HTTP/1.1

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:

• Switch to using Latin-1 to encode instead of ASCII (again, leaning on the precedent set in parse_headers and PEP-3333). This may make it too easy to write an out-of-spec client, however.
• Continue to use ASCII to encode, but include errors='surrogateescape' to give callers an escape hatch. This seems like a reasonably high bar to ensure that the caller actually intends to send unquoted data.
• Accept raw bytes and actually use them (rather than their repr()), allowing the caller to decide upon an appropriate encoding.

[jaraco]

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 .putrequest and .putrequest_raw or .putrequest_allow_invalid_bytes that sends a clear signal to the user that they're bypassing the default protections.

Or consider another approach where HTTPConnection implements an _encode_request() method that a subclass with a specialized need could override.

Would either of those approaches suit your use-case?

[tipabu]

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.

[jaraco]

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.

[jaraco]

I've started drafting a patch at https://github.com/python/cpython/tree/feature/putrequest-hooks

[jaraco]

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.

[ned-deily]

See msg352596 in bpo-30458 for discussion of whether the regression should be considered a "release blocker" for the imminent 3.7.5 and 3.5.8 releases.

[tirkarthi]

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
index cdd821ae..85a51cf3 100644
--- a/cherrypy/test/test_static.py
+++ b/cherrypy/test/test_static.py
@@ -398,7 +398,8 @@ class StaticTest(helper.CPWebCase):
self.assertInBody("I couldn't find that thing")

     def test_null_bytes(self):
-        self.getPage('/static/\x00')
+        from urllib.parse import quote
+        self.getPage(quote('/static/\x00'))
         self.assertStatus('404 Not Found')

 @classmethod

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.

[webknjaz]

@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?

[jaraco]

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.

[tirkarthi]

@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.

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.

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?

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.