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

CRLF injection vulnerability #1553

Closed
hannob opened this issue Mar 18, 2019 · 32 comments

Comments

Projects
None yet
@hannob
Copy link

commented Mar 18, 2019

At https://bugs.python.org/issue36276 there's an issue in Python's urllib that an attacker controlling the request parameter can inject headers by injecting CR/LF chars.

A commenter mentions that the same bug is present in urllib3:
https://bugs.python.org/issue36276#msg337837

So reporting it here to make sure it gets attention.

@haikuginger

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I confirmed that I was not able to reproduce and that the escaped CRLF was present in the querystring as of the current master. Will investigate to see what the bisection point was.

@haikuginger

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Verified as fixed in #1487, but that change has not been released. I recommend we release 1.24.2 with that change as soon as possible, @sethmlarson @theacodes.

@theacodes

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Without #1531 or IRI support in rfc3986 releasing master in it's current state will break backwards compatibility with international URLs.

@haikuginger

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

I've done some checking and requests appears to have handling so indirect consumers via requests should not be vulnerable through us, if that's a useful datum to have.

@vstinner

This comment has been minimized.

Copy link

commented Apr 10, 2019

CVE-2016-5699, CVE-2019-9740 and CVE-2019-9947 have been assigned to HTTP Header Injection vulnerabilities in Python: see https://bugs.python.org/issue30458 for more information.

@Plazmaz

This comment has been minimized.

Copy link

commented Apr 15, 2019

I was able to reproduce this with requests.get using urllib, haven't tested with urllib3 though.

@carnil

This comment has been minimized.

Copy link

commented Apr 15, 2019

CVE-2019-11236 was assigned for this issue.

@rschultheis

This comment has been minimized.

Copy link

commented Apr 17, 2019

👋 Hi! I've noticed that CVE-2019-11236 has been assigned to the CRLF injection issue described here. It seems that the library has been patched in GitHub, but no new release has been made to pypi. Will a new release containing the fix be made to pypi soon? Based on @theacodes comment it seems like a release was going to be made, but I also see her status has her perhaps unavailable. Is someone else perhaps able to cut a new release into pypi?

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@rschultheis Per my comment we cannot cut a release until IRI support is fixed in rfc3986.

@rschultheis

This comment has been minimized.

Copy link

commented Apr 17, 2019

Thank you @sethmlarson , I will watch that PR 🙇

@vstinner

This comment has been minimized.

Copy link

commented Apr 17, 2019

"It seems that the library has been patched in GitHub"

Do you have a reference to the change?

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

@luigino

This comment has been minimized.

Copy link

commented Apr 17, 2019

Is the problem fixed in 1.24.2? I couldn't find any reference to it in the changelog.

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

No it is not, this change will be fixed in 1.25 and is next on my list of work.

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Closed by #1531. Will be released in 1.25

@r1b

This comment has been minimized.

Copy link

commented Apr 23, 2019

I contacted NVD to recommend that they update the CVE to reflect the fact that versions up to and including 1.24.2 are vulnerable - currently they only indicate that 1.24.1 is vulnerable.

See https://nvd.nist.gov/vuln/detail/CVE-2019-11236

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 25, 2019

Hello!

Is there any chance of this landing in a 1.24.3? It would be nice to have a resolution to this CVE without necessarily having to inherit this change in 1.25.x:

#1507

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

If that's the only change you're trying to circumvent and are willing to accept not verifying certificates you can still pass cert_reqs="NONE" into your PoolManager / ConnectionPool.

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 25, 2019

@sethmlarson that works so long as your code is creating the manager/pool and you have direct influence over it (and your library isn't using some third-party dependency that itself imports and uses urllib3).

I work on a few projects (https://github.com/ansible/ansible and especially https://github.com/ansible/awx) where community or user-generated custom plugins are a very common use case (code which may itself have been written by an employee/coworker years ago, using urllib3), and it's likely that inheriting this new cert_reqs default behavior (whether it's a reasonable default or not - it clearly is 😉) may mean a potentially breaking behavioral change for our users in a minor patch release.

Currently, we're debating making a choice for our users on older stable releases. Do we address a CVE but also introduce a breaking change in a .z release which would force users to either update custom code somebody wrote, or go through infrastructure updates (even if they are wise updates to make)?

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 25, 2019

@haikuginger I've also done some digging, too, and I agree: I don't see a way that requests' interfaces themselves are vulnerable to this exploit:

It looks like this specific code in requests foils the exploit by quoting the full URI:

(Pdb) bt
  <stdin>(1)<module>()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/api.py(75)get()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/api.py(60)request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/sessions.py(519)request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/sessions.py(462)prepare_request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/models.py(313)prepare()
> /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/models.py(438)prepare_url()
(Pdb) l 435
430
431  	        enc_params = self._encode_params(params)
432  	        if enc_params:
433  	            if query:
434  	                query = '%s&%s' % (query, enc_params)
435  	            else:
436  	                query = enc_params
437
438  ->	        url = requote_uri(urlunparse([scheme, netloc, path, None, query, fragment]))
439  	        self.url = url
440
(Pdb) scheme, netloc, path, query, fragment
('http',
 'localhost:8000',
 '/ HTTP/1.1\r\nHEADER: INJECTED\r\nIgnore:',
 None,
 None)

Specifically:

(Pdb) bt
  <stdin>(1)<module>()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/api.py(75)get()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/api.py(60)request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/sessions.py(519)request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/sessions.py(462)prepare_request()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/models.py(313)prepare()
  /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/models.py(438)prepare_url()
> /home/ryanpetrello/venvs/tmp-56c32be26cf4d208/requests/requests/utils.py(603)requote_uri()
(Pdb) l 600
595  	    :rtype: str
596  	    """
597  	    safe_with_percent = "!#$%&'()*+,/:;=?@[]~"
598  	    safe_without_percent = "!#$&'()*+,/:;=?@[]~"
599  	    try:
600  	        # Unquote only the unreserved characters
601  	        # Then quote only illegal characters (do not quote reserved,
602  	        # unreserved, or '%')
603  ->	        return quote(unquote_unreserved(uri), safe=safe_with_percent)
604  	    except InvalidURL:
605  	        # We couldn't unquote the given URI, so let's try quoting it, but
(Pdb) uri
'http://localhost:8000/ HTTP/1.1\r\nHEADER: INJECTED\r\nIgnore:'
(Pdb) quote(unquote_unreserved(uri), safe=safe_with_percent)
'http://localhost:8000/%20HTTP/1.1%0D%0AHEADER:%20INJECTED%0D%0AIgnore:'

https://github.com/kennethreitz/requests/blob/v2.21.0/requests/utils.py#L603

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

If we released a change that only checked the authority, path, and query for unquoted control characters would that be sufficient? The problem is is that pre-1.25 the parse_url function is really dumb, basically just splits on delimiters and goes with whatever.

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 25, 2019

@sethmlarson if something like that prevented the exploit described in CVE-2019-11236, it would be very much appreciated.

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I'm pretty sure that's what h11 did for their fix. I'll look at what they did and put a patch together.

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 26, 2019

Thanks @sethmlarson!

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 30, 2019

@sethmlarson do you have a link to said h11 fix? I'd be glad to take a look and adapt a PR for urllib3.

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@ryanpetrello python-hyper/h11#69. I'll have to create a branch for 1.24-series as the fix doesn't make sense to apply to master.

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 30, 2019

@sethmlarson I'm not sure which branch I'd want to submit a PR to (sounds like I'd need you to make a 1.24-series branch), but how do you feel about this in general?

ryanpetrello@4cfdff4

>>> urllib3.PoolManager().request(
...     'GET',
...     'http://localhost:8000/ HTTP/1.1\r\nHEADER: INJECTED\r\nIgnore:'
... )
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/home/ryanpetrello/dev/urllib3/src/urllib3/request.py", line 68, in request
    **urlopen_kw)
  File "/home/ryanpetrello/dev/urllib3/src/urllib3/request.py", line 89, in request_encode_url
    return self.urlopen(method, url, **extra_kw)
  File "/home/ryanpetrello/dev/urllib3/src/urllib3/poolmanager.py", line 312, in urlopen
    u = parse_url(url)
  File "/home/ryanpetrello/dev/urllib3/src/urllib3/util/url.py", line 162, in parse_url
    raise ValueError("URL can't contain control characters. {}".format(url))
ValueError: URL can't contain control characters. http://localhost:8000/ HTTP/1.1
HEADER: INJECTED
Ignore:
@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I would raise a LocationParseError instead of raising a ValueError but besides that it looks fine. Maybe we'd want to put a note that this is taken from cpython as well. We'd also need some test cases for each control character.

@ryanpetrello

This comment has been minimized.

Copy link

commented Apr 30, 2019

@sethmlarson if you'll create a 1.24 series branch, I'll open a PR based on this (with some tests).

@sethmlarson

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@ryanpetrello

This comment has been minimized.

Copy link

commented May 2, 2019

An FYI for those in this thread: a resolution has also been merged into the 1.24 series and released as 1.24.3:

#1593
https://github.com/urllib3/urllib3/releases/tag/1.24.3

zabbal added a commit to zabbal/python-hereos-ingestion that referenced this issue May 21, 2019

Bump urllib3 version requirements
Fix CVE-2019-11236 - see urllib3/urllib3#1553 for details.

zabbal added a commit to zabbal/python-hereos-ingestion that referenced this issue May 21, 2019

Bump urllib3 version requirements
Fix CVE-2019-11236 - see urllib3/urllib3#1553 for details.

Signed-off-by: Max <max.suraev@here.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.