Strip Authorization header whenever root URL changes #4718

bmerry · 2018-06-28T14:48:34Z

Previously the header was stripped only if the hostname changed, but in
an https -> http redirect that can leak the credentials on the wire
(#4716). Based on with RFC 7235 section 2.2, the header is now stripped
if the "canonical root URL" (scheme+authority) has changed.

Closes #4716.

codecov-io · 2018-06-28T15:02:43Z

Codecov Report

Merging #4718 into master will decrease coverage by 0.02%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #4718      +/-   ##
==========================================
- Coverage   66.79%   66.77%   -0.03%     
==========================================
  Files          15       15              
  Lines        1563     1568       +5     
==========================================
+ Hits         1044     1047       +3     
- Misses        519      521       +2

Impacted Files	Coverage Δ
requests/sessions.py	`76.34% <90%> (+0.06%)`	⬆️
requests/models.py	`77.35% <0%> (-0.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33b41c7...2f24e02. Read the comment docs.

nateprewitt

Hi @bmerry, thanks for contributing to Requests! I agree that this behavior does seem wrong and we ideally shouldn’t be sending credentials across the wire in clear view. I took a quick look but was surprisingly unable to find this discussed before. While I’m inclined to merge this, there’s likely some concern about breaking users relying on us not doing the right thing here. I’d like @sigmavirus24 to weigh in with his thoughts before we move forward.

In the meantime, I’ve left a couple minor notes on some of the changes.

nateprewitt · 2018-06-28T18:08:00Z

requests/sessions.py

@@ -239,10 +239,10 @@ def rebuild_auth(self, prepared_request, response):
        if 'Authorization' in headers:
            # If we get redirected to a new host, we should strip out any
            # authentication headers.
-            original_parsed = urlparse(response.request.url)
-            redirect_parsed = urlparse(url)
+            original_root = urljoin(response.request.url, '/')


We may want to do the explicit check on scheme, hostname, and port here. Otherwise a url with credentials that’s redirected without credentials may have the Authorization header unnecessarily stripped.

Fair enough, I hadn't spotted that the username and password are still present in response.request.url.

nateprewitt · 2018-06-28T18:09:13Z

tests/test_requests.py

+            verify=httpbin_ca_bundle
+        )
+        assert r.history[0].request.headers['Authorization']
+        assert not r.request.headers.get('Authorization', '')


We should be explicit here and validate that Authorization is not in the request headers rather than checking if the value is falsey.

I was just copying the test above it, but I agree that would be an improvement. Will fix it tomorrow.

bmerry · 2018-06-28T18:21:30Z

While I’m inclined to merge this, there’s likely some concern about breaking users relying on us not doing the right thing here.

Totally understood. There is always someone.

sethmlarson · 2018-06-28T19:25:55Z

I'll just leave this here: urllib3/urllib3#1346

nateprewitt · 2018-06-28T20:31:40Z

@SethMichaelLarson we currently perform the redirects manually because we’ve got a few more steps we perform. I’ve looked at moving to urllib3 with 1.23 but don’t know if I see an easy route that guarantees we maintain our 2.X expectations.

sethmlarson · 2018-06-28T20:44:06Z

@nateprewitt TIL! Thanks for teaching me that. :)

bmerry · 2018-06-29T07:11:01Z

@nateprewitt I've made the changes you suggested.

@SethMichaelLarson so what behaviour does urllib3 have on https->http redirects to the same host?

sigmavirus24 · 2018-06-29T17:16:58Z

@nateprewitt this shouldn't break too many people. The major breaking change already happened years ago. I'm 100% 👍 on this. I haven't reviewed the code though.

sigmavirus24 · 2018-06-29T17:17:46Z

tests/test_requests.py

+            verify=httpbin_ca_bundle
+        )
+        assert r.history[0].request.headers['Authorization']
+        assert 'Authorization' not in r.request.headers


r.json()['headers'] should also be a way to verify that it wasn't sent

sigmavirus24 · 2018-06-29T17:18:39Z

requests/sessions.py

-            if (original_parsed.hostname != redirect_parsed.hostname):
+            if (original_parsed.hostname != redirect_parsed.hostname
+                    or original_parsed.port != redirect_parsed.port
+                    or original_parsed.scheme != redirect_parsed.scheme):


I think this might need to be a bit different. I think if someone sent credentials over HTTP and we're being redirected to the same domain but it's HTTPS we should allow that. That's a secure upgrade. The opposite should not be allowed.

I did it this way based on RFC 7235:

A protection space is defined by the canonical root URI (the scheme
and authority components of the effective request URI; see Section
5.5 of [RFC7230]) of the server being accessed, in combination with
the realm value if present.

...

The protection space determines the domain over which credentials can
be automatically applied. If a prior request has been authorized,
the user agent MAY reuse the same credentials for all other requests
within that protection space for a period of time determined by the
authentication scheme, parameters, and/or user preferences (such as a
configurable inactivity timeout).

While it's not explicit, the implication is that a user agent may not automatically reuse the credentials outside the protection space, and a change from http -> https is a change in protection space.

@bmerry, just to be clear, the spec you’re referencing is specifically about the realm attribute, which is different that what we’re doing here.

If we look at browsers for reference here, upgrade redirects maintain Authorization headers. Breaking this will make a common pattern pretty painful for most users.

Strange, I thought I'd tested the http -> https case with Firefox and Chromium and neither had sent the Authorization header. I'll take a closer look at my test when I'm in the office tomorrow.

Just to check that I'm testing the same thing that you're referring to: my test is a localhost Apache server listening on http and https. On http it is set up to require basic auth, and on some path it is set up to send a 302 redirect to somewhere on https. I point the browser at the http port, I get a WWW-Authenticate challenge and I type in username+password, I then get redirected to https and the browser either does or doesn't automatically send the same credentials immediately without another WWW-Authenticate challenge?

I've redone my test, using openssl s_server to see the request headers that the browser sends to the https port after the redirect. Firefox definitely isn't preserving the Authorization header across the redirect. Let me know if you'd like more details to reproduce the test (httpd.conf etc) using Docker.

Yes

>>> print(urllib.parse.urlparse('http://example.com/').port) None >>> print(urllib.parse.urlparse('http://example.com:80/').port) 80

So we'd have to account for that in our comparison above as we want http://example.com:80 -> https://example.com to work the same as http://example.com -> https://example.com.

At this point there’s enough complexity in this implementation we should probably move it into it’s own check function.

I think Seth’s assessment is right here. We want to check the following:

The hostnames match.

The schemes match...

Except in the case of http->https

If scheme change is http->https, origin port may be 80 or None and destination port may be 443 or None.

Ports match, except in above scenario.

Same scheme and hostname but varied port results in stripped auth.

Sounds sensible to me. I've implemented that in the latest push.

I've left the logic in a public method of SessionRedirectMixin, because that gives the user the option to override it in a subclass, but let me know if I should move it to utils.

Ping? I think I've addressed the comments so far.

bmerry · 2018-09-05T08:05:57Z

@nateprewitt is there anything still outstanding here?

nateprewitt

Ok, I think this is set to go. Thanks for driving this, @bmerry!

nateprewitt · 2018-09-05T15:29:43Z

Actually sorry @bmerry, it looks like this isn’t rebased onto master. Would you mind rebasing really quickly and then we can get this merged.

bmerry · 2018-09-05T16:09:33Z

Ok, I've rebased.

Previously the header was stripped only if the hostname changed, but in an https -> http redirect that can leak the credentials on the wire (psf#4716). Based on with RFC 7235 section 2.2, the header is now stripped if the "canonical root URL" (scheme+authority) has changed, by checking scheme, hostname and port.

The exception for http->https upgrade now requires the standard HTTP(S) ports to be used, either implicitly (no port specified) or explicitly.

bmerry · 2018-09-14T08:45:16Z

And I've rebased again.

nateprewitt

Ok, this still looks good. Thanks again for driving this @bmerry!

ofek · 2018-10-11T20:00:10Z

Are there plans to release this security fix soon?

nateprewitt · 2018-10-11T20:04:58Z

@ofek, it’d be preferable to coordinate with urllib3 to avoid having to do multiple releases. I don’t think we have a timeline to commit to, but we may be able to do something in the next 3-8 weeks. If it’s going to go beyond that, we can look at cutting a Requests release without urllib3.

sethmlarson · 2018-10-11T20:18:15Z

@nateprewitt feel free to ping one of us if ya'll want to make a release with us, we've been due for a release as well. Reminder: the next release of urllib3 will have dropped Python 2.6 support.

nateprewitt · 2018-10-11T20:45:47Z

@SethMichaelLarson Yep, we’ll make sure that’s surfaced in the release notes. I’ll ping you and/or Thea this weekend and we can work out a timeline.

kbabioch · 2018-10-12T09:48:03Z

This has been assigned CVE-2018-18074

psf/requests#4718

as per GitHub advise re CVE-2018-20060 re https://github.com/kennethreitz/requests/pull/4718

nateprewitt requested changes Jun 28, 2018

View reviewed changes

bmerry force-pushed the fix-4716 branch from f8b45f0 to 3fbdd88 Compare June 29, 2018 07:09

sigmavirus24 reviewed Jun 29, 2018

View reviewed changes

sethmlarson previously approved these changes Sep 5, 2018

View reviewed changes

nateprewitt previously approved these changes Sep 5, 2018

View reviewed changes

bmerry dismissed stale reviews from nateprewitt and sethmlarson via 2f24e02 September 5, 2018 16:09

bmerry force-pushed the fix-4716 branch from 548a0e0 to 2f24e02 Compare September 5, 2018 16:09

nateprewitt previously approved these changes Sep 5, 2018

View reviewed changes

bmerry added 2 commits September 14, 2018 10:44

Rework authorization stripping logic as discussed

857e9b7

The exception for http->https upgrade now requires the standard HTTP(S) ports to be used, either implicitly (no port specified) or explicitly.

bmerry dismissed nateprewitt’s stale review via 857e9b7 September 14, 2018 08:45

bmerry force-pushed the fix-4716 branch from 2f24e02 to 857e9b7 Compare September 14, 2018 08:45

nateprewitt approved these changes Sep 14, 2018

View reviewed changes

nateprewitt merged commit c45d7c4 into psf:master Sep 14, 2018

webmaven mentioned this pull request Oct 16, 2018

CVE URLs update: www sub-subdomain no longer valid #4827

Merged

ofek mentioned this pull request Oct 17, 2018

Allow urllib3 1.24 #4836

Closed

nateprewitt mentioned this pull request Oct 29, 2018

proper handling for default ports in auth stripping #4851

Merged

paicha added a commit to paicha/gxgk-wechat-server that referenced this pull request Oct 30, 2018

Update requirements.txt

1e44e16

psf/requests#4718

nateprewitt mentioned this pull request Dec 12, 2018

Address CVE-2018-20060 #4907

Closed

artemv referenced this pull request in artemv/papertrail-python-cli Feb 12, 2019

chore(deps): upgrade requests

ed3c879

as per GitHub advise re CVE-2018-20060 re https://github.com/kennethreitz/requests/pull/4718

artemv referenced this pull request in artemv/papertrail-python-cli Feb 14, 2019

chore(deps): upgrade requests

b644968

as per GitHub advise re CVE-2018-20060 re https://github.com/kennethreitz/requests/pull/4718

github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip Authorization header whenever root URL changes #4718

Strip Authorization header whenever root URL changes #4718

bmerry commented Jun 28, 2018

codecov-io commented Jun 28, 2018 •

edited

Loading

nateprewitt left a comment

nateprewitt Jun 28, 2018

bmerry Jun 28, 2018

nateprewitt Jun 28, 2018

bmerry Jun 28, 2018

bmerry commented Jun 28, 2018

sethmlarson commented Jun 28, 2018

nateprewitt commented Jun 28, 2018

sethmlarson commented Jun 28, 2018

bmerry commented Jun 29, 2018

sigmavirus24 commented Jun 29, 2018

sigmavirus24 Jun 29, 2018

sigmavirus24 Jun 29, 2018

bmerry Jun 29, 2018

nateprewitt Jul 2, 2018

bmerry Jul 2, 2018

bmerry Jul 3, 2018

bmerry Aug 13, 2018

sethmlarson Aug 13, 2018

nateprewitt Aug 13, 2018

bmerry Aug 14, 2018

bmerry Aug 23, 2018

bmerry commented Sep 5, 2018

nateprewitt left a comment

nateprewitt commented Sep 5, 2018

bmerry commented Sep 5, 2018

bmerry commented Sep 14, 2018

nateprewitt left a comment

ofek commented Oct 11, 2018

nateprewitt commented Oct 11, 2018

sethmlarson commented Oct 11, 2018

nateprewitt commented Oct 11, 2018

kbabioch commented Oct 12, 2018

Strip Authorization header whenever root URL changes #4718

Strip Authorization header whenever root URL changes #4718

Conversation

bmerry commented Jun 28, 2018

codecov-io commented Jun 28, 2018 • edited Loading

Codecov Report

nateprewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmerry commented Jun 28, 2018

sethmlarson commented Jun 28, 2018

nateprewitt commented Jun 28, 2018

sethmlarson commented Jun 28, 2018

bmerry commented Jun 29, 2018

sigmavirus24 commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmerry commented Sep 5, 2018

nateprewitt left a comment

Choose a reason for hiding this comment

nateprewitt commented Sep 5, 2018

bmerry commented Sep 5, 2018

bmerry commented Sep 14, 2018

nateprewitt left a comment

Choose a reason for hiding this comment

ofek commented Oct 11, 2018

nateprewitt commented Oct 11, 2018

sethmlarson commented Oct 11, 2018

nateprewitt commented Oct 11, 2018

kbabioch commented Oct 12, 2018

codecov-io commented Jun 28, 2018 •

edited

Loading