Skip to content
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

After the python3 fix for CVE-2019-9740 test_null_bytes fails #1781

Closed
mcepl opened this issue May 31, 2019 · 14 comments · Fixed by #1807

Comments

@mcepl
Copy link

commented May 31, 2019

I'm submitting a ...

  • bug report
  • feature request
  • question about the decisions made in the repository

Do you want to request a feature or report a bug?
bug

What is the current behavior?
When running the testsuite StaticTest.test_null_bytes fails with:

[  107s] __________________________ StaticTest.test_null_bytes __________________________
[  107s] 
[  107s] self = <cherrypy.test.test_static.StaticTest testMethod=test_null_bytes>
[  107s] 
[  107s]     def test_null_bytes(self):
[  107s] >       self.getPage('/static/\x00')
[  107s] 
[  107s] cherrypy/test/test_static.py:401: 
[  107s] _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
[  107s] cherrypy/test/helper.py:326: in getPage
[  107s]     protocol, raise_subcls)
[  107s] /usr/lib/python3.7/site-packages/cheroot/test/webtest.py:212: in getPage
[  107s]     raise_subcls, ssl_context=self.ssl_context,
[  107s] /usr/lib/python3.7/site-packages/cheroot/test/webtest.py:514: in openURL
[  107s]     skip_accept_encoding=True,
[  107s] _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
[  107s] 
[  107s] self = <http.client.HTTPConnection object at 0x7f1697a032b0>, method = 'GET'
[  107s] url = '/static/\x00', skip_host = True, skip_accept_encoding = True
[  107s] 
[  107s]     def putrequest(self, method, url, skip_host=False,
[  107s]                    skip_accept_encoding=False):
[  107s]         """Send a request to the server.
[  107s]     
[  107s]         `method' specifies an HTTP request method, e.g. 'GET'.
[  107s]         `url' specifies the object being requested, e.g. '/index.html'.
[  107s]         `skip_host' if True does not add automatically a 'Host:' header
[  107s]         `skip_accept_encoding' if True does not add automatically an
[  107s]            'Accept-Encoding:' header
[  107s]         """
[  107s]     
[  107s]         # if a prior response has been completed, then forget about it.
[  107s]         if self.__response and self.__response.isclosed():
[  107s]             self.__response = None
[  107s]     
[  107s]     
[  107s]         # in certain cases, we cannot issue another request on this connection.
[  107s]         # this occurs when:
[  107s]         #   1) we are in the process of sending a request.   (_CS_REQ_STARTED)
[  107s]         #   2) a response to a previous request has signalled that it is going
[  107s]         #      to close the connection upon completion.
[  107s]         #   3) the headers for the previous response have not been read, thus
[  107s]         #      we cannot determine whether point (2) is true.   (_CS_REQ_SENT)
[  107s]         #
[  107s]         # if there is no prior response, then we can request at will.
[  107s]         #
[  107s]         # if point (2) is true, then we will have passed the socket to the
[  107s]         # response (effectively meaning, "there is no prior response"), and
[  107s]         # will open a new one when a new request is made.
[  107s]         #
[  107s]         # Note: if a prior response exists, then we *can* start a new request.
[  107s]         #       We are not allowed to begin fetching the response to this new
[  107s]         #       request, however, until that prior response is complete.
[  107s]         #
[  107s]         if self.__state == _CS_IDLE:
[  107s]             self.__state = _CS_REQ_STARTED
[  107s]         else:
[  107s]             raise CannotSendRequest(self.__state)
[  107s]     
[  107s]         # Save the method we use, we need it later in the response phase
[  107s]         self._method = method
[  107s]         if not url:
[  107s]             url = '/'
[  107s]         # Prevent CVE-2019-9740.
[  107s]         if _contains_disallowed_url_pchar_re.search(url):
[  107s] >           raise InvalidURL(f"URL can't contain control characters. {url!r}")
[  107s] E           http.client.InvalidURL: URL can't contain control characters. '/static/\x00'
[  107s]

Throwing an exception here is actually completely correct behavior after fixing python/cpython#13044 (CVE-2019-9740).

If the current behavior is a bug, please provide the steps to reproduce and if possible a screenshots and logs of the problem. If you can, show us your code.

Full build log

What is the expected behavior?
Whole test suite should pass.

What is the motivation / use case for changing the behavior?
Packaging python-CherryPy for openSUSE.

Please tell us about your environment:
Linux/openSUSE/Tumbleweed

  • Cheroot version: 6.55
  • CherryPy version: 18.1.1
  • Python version: 3.7.3
  • OS: openSUSE/Tumbleweed
  • Browser: not in the browser

Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, e.g. stackoverflow, gitter, etc.)

@duper51

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Just tried on latest Cheroot and CherryPy, as well as the versions you have specified and I'm not seeing the same effect. That test passed on my machine. What version of urllib are you running?

@mcepl

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

From the already attached build log:

[    5s] [190/193] cumulate python3-urllib3-1.25.3-76.1
@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

This is not because of the urllib or urllib3.

CPython upstream has patched http.client.HTTPConnection.putrequest(): https://github.com/python/cpython/blob/08970cb03c61f62f4f69e7769d0075fa66bb86aa/Lib/http/client.py#L1092-L1095.
This is what our test hit.

The test tries to inject a know malicious sequence into the Request-Line of a test HTTP request and expects the server to reply with an error. There's a similar test in Cheroot as well (https://github.com/cherrypy/cheroot/blob/7558014/cheroot/test/test_core.py#L137-L164).

But with the latest CPython it fails to create such a request because they forbid it now.
This probably means that we need our own patched HTTP client in order to try out various nasty things in tests.

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

OTOH, this test (submitted by @diogobaeder) is probably incorrect and the server should produce 400, not 404. Only ASCII should be allowed in the Request-Line, anything else must be URL-encoded.

@webknjaz webknjaz added the bug label Jun 21, 2019
@diogobaeder

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Hi guys,

@webknjaz I'm curious, what test did I submitted that was incorrect? I couldn't find that in git blame from the test above...

Thanks!

@diogobaeder

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Ah, got it... thanks!

Yeah, indeed, thinking again 400 seems more sensible to me. Thanks for catching that!

Cheers,
Diogo

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

FTR here's an instance of this error in the CI: https://travis-ci.org/cherrypy/cherrypy/jobs/549122435#L558

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

I opened cherrypy/cheroot#208 to address this on the HTTP level.

@jaraco jaraco referenced this issue Jul 2, 2019
7 of 15 tasks complete
@jaraco

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Until this issue can be resolved properly, it's breaking CI for unrelated pull requests. Therefore, I'm going to mark nightly as allowed-failure to unblock other activity.

@jaraco

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

This issue also affects mac builds like this one.

@jaraco

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

There is an upstream issue36274 tracking this use-case.

@tipabu

This comment has been minimized.

Copy link

commented Sep 13, 2019

Well, a similar one, anyway 😉 When I'd originally written it, you could still send null bytes just fine, but not b'\x80'through b'\xff'

I'd love to see an upstream patch that accommodates both use-cases, though!

tipabu added a commit to tipabu/cherrypy that referenced this issue Sep 20, 2019
Fixes cherrypy#1781

Recent versions of CPython have gotten more picky about the sorts of
paths callers can pass to http.client's putrequest. Since we don't have
any control over what clients might send, though, keep coverage of the
invalid-request behavior by just opening up a socket and sending some
bytes.

See also: https://bugs.python.org/issue38216
@jaraco

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

In addition to the patch proposed by tipabu, I believe we may be able to work around this issue by re-writing the regex that intercepts these null bytes.

jaraco added a commit that referenced this issue Sep 21, 2019
jaraco added a commit that referenced this issue Sep 22, 2019
@jaraco jaraco closed this in #1807 Sep 22, 2019
jaraco added a commit that referenced this issue Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.