Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Repopulate ~/.netrc auth. #1892

Merged
merged 3 commits into from

5 participants

@Lukasa
Collaborator

This should be a fix for #1885.

@sigmavirus24, can you give me some code review? =)

requests/sessions.py
@@ -158,6 +158,23 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None,
prepared_request._cookies.update(self.cookies)
prepared_request.prepare_cookies(prepared_request._cookies)
+ # If we get redirected to a new host, we should strip out any
+ # authentication headers.
+ original_parsed = urlparse(resp.request.url)
+ redirect_parsed = urlparse(url)
+
+ if original_parsed.hostname != redirect_parsed.hostname:
@sigmavirus24 Collaborator

What about:

if (original_parsed.hostname != redirect_parsed.hostname and 
        'Authorization' in headers):
    del headers['Authorization']
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sigmavirus24
Collaborator

One comment otherwise LGTM.

You don't need to write a test for this, but I have an idea that I'd like to test it with.

@Lukasa
Collaborator

Done and done.

@sigmavirus24
Collaborator

:shipit:

@kennethreitz

I need to think about this.

@sigmavirus24
Collaborator

@kennethreitz we absolutely cannot continue reusing authorizations on redirects to sites that are not the same host. With that in mind we almost certainly need to issue a CVE. I'll happily work on that though.

We've been leaking credentials and we need to at least address that. Whether we repopulate the auth after stopping the leak or not is more of a feature decision. I'm sure one of the security experts, like @dstufft would back up @Lukasa and I on that.

@Stephn-R

@kennethreitz , call me the outsider but @sigmavirus24 got a point. As the new guy 'round these parts. You have 1.1million downloads. Even if half that still use, we are talking about a major security failure in the code base. :no_good:

@Lukasa
Collaborator

I'm happy to remove the repopulation of ~/.netrc, but the only reason this shouldn't get merged I just fixed (I wasn't respecting Session.trust_env).

@Stephn-R

@Lukasa So in that sense, are you suggesting that the session itself may be invalidated and then a restart is required?

@Lukasa
Collaborator

No. =)

All I was saying is that the previous version of this fix didn't respect the flag that controls whether we should look at ~/.netrc. Now it does.

The barest minimum we have to do is remove Authorization headers on redirects to new hosts. That's pretty much mandatory. Everything else is gravy.

@Stephn-R

Oh well alrighty then! :) :+1:

@sigmavirus24
Collaborator

@Sirenity it isn't a big deal but requests should have far more than 8 million downloads last I checked (we're probably upwards of 9 million) and that's only from PyPI. We have no way of measuring how many downloads installations came from distribution repositories (i.e., people installing aptitude, yum, or pacman).

@kennethreitz

@sigmavirus24 this was an explicit design decision, and it has been stated as such many times before. I've considered implementing a patch much like many times before, and this was a minor concern in the back of my mind when I did "the big refactor". As I said, I need to think about it.

Further comments about are neither helpful nor welcome. :)

@dstufft

Oof, I just noticed this. This really should change FWIW, carrying authentication data across access boundaries is a bad idea generally. It'd be up to Mitre but a CVE is probably appropriate as well. If you do get a CVE and you do change this, include the CVE number in the changelog please :]

@kennethreitz

This simple patch is not a technical change, but a fairly large philosophical one. Traditionally, I have recommended that all security-concious users disable redirection support, and follow them on their own. This patch would make security the something that Requests does for you (something that I already did for the Python world when I verified SSL by default). This would take it to the next level.

I'm not judging it as either good or bad. I just need need to think about it.

Again, no further input needed. Will be back to you shortly.

@kennethreitz kennethreitz merged commit f1893c8 into kennethreitz:master
@Lukasa Lukasa deleted the Lukasa:netrcauth branch
@kennethreitz

Also, if we could refactor this, it would be great. You ruined my beautiful code. :)

@dstufft
@sigmavirus24
Collaborator

I have a CVE Identifier for this when we release the next version. I also have one for the Proxy-Authorization exposure bug.

@dstufft
@sigmavirus24
Collaborator

For this one: CVE-2014-1829

Edit Just got it from MITRE this morning so it may not show up in searches yet.

@kennethreitz

I cleaned up the ugly code. :P

@Lukasa
Collaborator

We've learned a valuable lesson today: the easiest way to get Kenneth to write more code is to put really ugly code in front of him and let his natural instincts take over. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 30, 2014
  1. @Lukasa

    Repopulate ~/.netrc auth.

    Lukasa authored
Commits on Jan 31, 2014
  1. @Lukasa

    Better layout for checking.

    Lukasa authored
  2. @Lukasa
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 0 deletions.
  1. +15 −0 requests/sessions.py
View
15 requests/sessions.py
@@ -158,6 +158,21 @@ def resolve_redirects(self, resp, req, stream=False, timeout=None,
prepared_request._cookies.update(self.cookies)
prepared_request.prepare_cookies(prepared_request._cookies)
+ # If we get redirected to a new host, we should strip out any
+ # authentication headers.
+ original_parsed = urlparse(resp.request.url)
+ redirect_parsed = urlparse(url)
+
+ if (original_parsed.hostname != redirect_parsed.hostname and
+ 'Authorization' in headers):
+ del headers['Authorization']
+
+ # However, .netrc might have more auth for us. Let's get it if it
+ # does.
+ new_auth = get_netrc_auth(url) if self.trust_env else None
+ if new_auth is not None:
+ prepared_request.prepare_auth(new_auth)
+
resp = self.send(
prepared_request,
stream=stream,
Something went wrong with that request. Please try again.