Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Redirect can expose netrc password #1885

Closed
eriol opened this Issue Jan 27, 2014 · 20 comments

Comments

Projects
None yet
4 participants
Contributor

eriol commented Jan 27, 2014

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

Owner

Lukasa commented Jan 27, 2014

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?

Owner

sigmavirus24 commented Jan 27, 2014

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.

Owner

Lukasa commented Jan 27, 2014

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

Owner

sigmavirus24 commented Jan 27, 2014

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

Owner

Lukasa commented Jan 27, 2014

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.

Owner

sigmavirus24 commented Jan 27, 2014

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).

Contributor

eriol commented Jan 27, 2014

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

@Lukasa I agree!

Owner

sigmavirus24 commented Jan 27, 2014

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.

Contributor

eriol commented Jan 27, 2014

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.

Owner

sigmavirus24 commented Jan 27, 2014

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?

Owner

Lukasa commented Jan 28, 2014

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.

Owner

Lukasa commented Jan 29, 2014

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

Owner

sigmavirus24 commented Jan 31, 2014

@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?

Contributor

eriol commented Jan 31, 2014

@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.

Owner

sigmavirus24 commented Jan 31, 2014

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.

Owner

Lukasa commented Jan 31, 2014

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. #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.

Owner

sigmavirus24 commented Jan 31, 2014

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.

Owner

Lukasa commented Jan 31, 2014

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

@rassie rassie referenced this issue in pypa/pip Feb 7, 2014

Closed

Proxy issues with requests #1541

Contributor

blueyed commented Sep 12, 2014

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)

Owner

sigmavirus24 commented Sep 12, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment