Redirect can expose netrc password

[eriol]

Hello,
Jakub Wilk reported this on the Debian Bug Tracker[¹]:

If site A redirects to site B, and user had a password for site A in 
their ~/.netrc, then requests would send authorization information both 
to site A and to site B.

Jakub wrote also some tests to show the issue, you can find them attached on the Debian Bug Tracker.

I have already updated requests Debian package (it's ready for the upload) and I can confirm that the issue is present also in requests 2.2.1.

Cheers!

[¹] http://bugs.debian.org/733108

[Lukasa]

Thanks for this!

Right now we don't strip authentication information of any kind on redirects. We could, but we can't ask the user for new credentials, we can only really go to `~/. Thoughts @sigmavirus24?

[sigmavirus24]

We delete any Cookies set. I'm not sure I see the benefit in sending the old credentials to the site we're redirected to. Is there anything in the RFC about this? I would guess it could be considered safe if we're redirecting to the same domain, but otherwise, I don't see a point in sending it along as well.

That said, we would be breaking backwards compatibility. Someone relying on this will need to do extra work.

[Lukasa]

Agreed, this is backwards incompatible. I'd rather be secure(r) by default, though. Open question: do we reapply ~/.netrc auth?

[sigmavirus24]

The more accurate question is: Do we del headers['Authorization']?

[Lukasa]

Yes, absolutely yes. What about Proxy-Authorization?

EDIT: To be clear, we should conditionally delete the Authorization header, only if we're being redirected to a new host.

[sigmavirus24]

What about Proxy-Authorization?

Ehhhhhhhh maybe? That's a grey area. My typical mental model of a proxy is on
your end so you shouldn't be concerned about redirects and shouldn't be
concerned about your proxy authentication being broken on redirects.

One thing that's nice is we don't have to worry about times when authorization
is specified in the URL itself (e.g., http://user:pass@example.com).

[eriol]

EDIT: To be clear, we should conditionally delete the Authorization header, only if we're being redirected to a new host.

@Lukasa I agree!

[sigmavirus24]

To be clear, we should conditionally delete the Authorization header, only if we're being redirected to a new host.

This is going to be tricky. Some domains will not want you to keep them when redirecting to sub-domains. It's a safer thing to do than to preserve authorization for redirecting to different domains but it may still be tricky.

[eriol]

Some domains will not want you to keep them when redirecting to sub-domains.

I think we should consider a sub-domain as a new host[¹]. Maybe we can check if user's .netrc has a stanza about the sub-domain when redirecting. Only if we find a stanza about the sub-domain, we send the credentials for the sub-domain we are redirected to. What do you think?

[¹] For example, think about *.neocities.org: we can't presume if the owner of two sub-domains is the same.

[sigmavirus24]

Yeah I talked to some of the security guys at Bendyworks about this and they agree with you @EriolV. So a simple check of

original_parsed = urlparse(respone.request.url)
redirect_parsed = urlparse(url)
if original_parsed.host != redirect_parsed.host:
    del headers['Authorization']

Have we decided about Proxy-Authorization?

[Lukasa]

Agreed, subdomains have to be treated as new hosts.

We can be smart about proxy auth as well, I think. First thing to note is that our proxy model uses one proxy per scheme: if you don't get redirected to a different scheme you can safely keep sending Proxy-Authorization headers. Unfortunately, I don't think we can be smarter than that. I've considered whether we could attach the correct auth string for any proxy the user configures for the other scheme but I don't think we can: we need to know what came in on the original Session.request() call to be sure.

[Lukasa]

Ok, take a look at #1892 for a possible fix.

[sigmavirus24]

@EriolV do you have experience with CVEs? This was reported to debian but is not a debian specific issue. This is our responsibility to report to MITRE, right?

[eriol]

@sigmavirus24 unfortunately I don't have experience with CVEs, but it's well described here:
http://people.redhat.com/kseifrie/CVE-OpenSource-Request-HOWTO.html

Sending a request to oss-security@lists.openwall.com should be enough,
but maybe security folks at Red Hat made a request for a CVE since they are following the issue too: https://bugzilla.redhat.com/show_bug.cgi?id=1046626

As you can see Endi Sukma Dewata also asked me if there are tests for the Proxy-Authorization case.
I'm going to reply on Red Hat tracker asking if they already sent a request for a CVE.

[sigmavirus24]

Thanks for managing everything @EriolV. I'm going to guess none of you have submitted a CVE so I'm going to request one from MITRE since the CVE suggests just asking them.

FWIW, if you would like you can tell Endi that we're going to address the Proxy-Authorization case before the next release but in a separate PR probably.

[Lukasa]

Ok, let's summarise the Proxy-Authorization case while we're here and talking about it.

Problem: Proxy-Authorization is never re-evaluated when we are redirected. This means that if we're redirected to a new proxy, or away from proxies entirely, we'll keep sending the Proxy-Authorization header. This is bad.

Interesting Notes: A few things.

1. In the primary Requests API, proxies are scheme-specific: that is, they apply to all requests to a given scheme. In principle this allows us to be clever and avoid doing anything unless a redirect takes us across a scheme.
2. Unfortunately, our use of proxy environment variables (and particularly the NO_PROXY variable) breaks that limitation.
3. Proxy configuration is not re-evaluated on redirects #1727 is tracking the fact that we don't re-evaluate NO_PROXY on redirects. A fix for this is not yet made, but requires us re-evaluating our proxy configuration each time we're redirected. Since we expect proxy auth information to be in the proxy URL, we're probably able to fix both bugs in one move.

Interim Fix: If we need to rush a 'fix' out the door, we can start by unconditionally removing the Proxy-Authorization header in all cases on redirect. This covers all cases where we don't hit NO_PROXY.

Better Fix: Comes in two parts:

1. Remove Proxy-Authorization during redirect.
2. Re-evaluate proxy environment variables, fixing up the proxy dictionary as required.

That should be all we need: the HTTPAdapter will handle regenerating the Proxy-Authorization header as needed.

[sigmavirus24]

I would like to have everything fixed in one release but if we have to. Stripping out the headers on redirect is enough for me. That at least removes the exposure and makes our users safer even if it temporarily inconveniences them. I mean it's a freaking open source library. Not having a file parsed and used to authenticate for you is the very definition of a First World Problem and for that I'm a bit unsympathetic. I'm also grumpy after a conversation on twitter, so feel free to ignore my bitterness.

[Lukasa]

I'm pretty convinced re-evaluating the proxy configuration is a minor amount of work, I'll take a swing at it this weekend.

[blueyed]

Is this issue done, given that re-evaluating is in place now, or is there something missing that has been discussed?
(https://github.com/kennethreitz/requests/blob/master/requests/sessions.py#L175-177)

[sigmavirus24]

Good catch @blueyed. This was fixed in v2.3.0.