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

Auth header remains during redirects #1316

Closed
zockons12 opened this issue Jan 17, 2018 · 17 comments
Closed

Auth header remains during redirects #1316

zockons12 opened this issue Jan 17, 2018 · 17 comments

Comments

@zockons12
Copy link

Requests does it: request/request#1184

@sigmavirus24
Copy link
Contributor

Except urllib3 doesn't directly handle authentication the way Requests does. I don't disagree, but there are fundamentally different layers of concern here.

@haikuginger
Copy link
Contributor

I tend to agree with @sigmavirus24. Our header handling is much more agnostic to what those headers actually are. If we implemented authentication directly, that'd be one thing, but we don't.

I would be interested to see what cURL does here in the specific case where it's given -H "Authorization: Basic xxxxxxxxx" as compared to -U "username:password". If it's the case that cURL strips a manually-constructed header on cross-host redirect, then I could be convinced to entertain a PR, although I likely wouldn't spend time on it myself.

@haikuginger haikuginger changed the title You should add support for removal of auth on redirects Auth header remains during redirects Jan 17, 2018
@sigmavirus24
Copy link
Contributor

@haikuginger actually, cURL preserves authentication on redirect in the face of leaking credential. But it also doesn't automagic redirects like requests does. To do that you need to specify -L so sending auth in the header or with -u won't automatically give away credentials unless you manually follow it or specify -L.

@sigmavirus24
Copy link
Contributor

(Since you said you'd be interested)

@shazow
Copy link
Member

shazow commented Jan 17, 2018

urllib3 redirects across hosts by default these days, doesn't it? Sounds like a pretty scary surprise-footgun to have.

At minimum, sounds like the redirect feature needs a big fat warning that all headers including authentication headers will be forwarded along. Ideally it feels like request-specific headers need to be stripped for cross-host by default (maybe a urllib3.Retry object param?).

(Sorry for helicopter-commenting here, just randomly saw this thread in my filter, please feel free to ignore. ❤️)

Edit: Random scary scenario I'm thinking about: You have an API that for some reason can yield a redirect (it's a thing, I've even built one in 2010), then by default the redirect destination is going to get your secret bearer token and all that jazz. Especially scary as a default.

@shazow
Copy link
Member

shazow commented Jan 17, 2018

Btw if we decide it's a good idea and nobody wants to do it, I'll put together a quick PR. Should be 4-5 lines of changes in urlopen and Retry.

@haikuginger
Copy link
Contributor

I agree this is a potential footgun. I'd almost be tempted to turn off redirect following by default.

@zockons12
Copy link
Author

zockons12 commented Jan 18, 2018

If I turn off redirect, the script breaks e.g redirect=False. It needs to redirect just not with auth. This is a big problem if you use say a webhost server and then have to redirect to Amazon. A workaround for now is to use a monkey patch to remove auth.

@haikuginger
Copy link
Contributor

@YodaBear, understandable, but our concerns are for all users, not just for one specific use case. I'd welcome feedback from @Lukasa, @sigmavirus24, and @shazow as to what the Right Thing to Do is considering that we don't have an end-to-end API for HTTP auth. Reopening this issue for that discussion of the broader case.

@haikuginger haikuginger reopened this Jan 18, 2018
@sigmavirus24
Copy link
Contributor

@shazow there is more discussion of potential problems with this over on the relevant Requests issues and I agree with you that it's a problem. I thought urllib3 had redirect handling off-by-default.

I can also toss together a PR for this.

If I turn off redirect, the script breaks e.g redirect=False.

That's surprising but I suspect that if you asked for help on StackOverflow they would help you understand why its breaking and how to fix it while we fix this underlying bug.

@haikuginger
Copy link
Contributor

@sigmavirus24, please go right ahead.

@shazow
Copy link
Member

shazow commented Jan 18, 2018

@sigmavirus24 Right, IMO any changes we make should be configurable back to what we have today via the Retry object. I was imagining something like retries=Retry(forward_headers_across_hosts=True) in the spirit of keeping scary flags annoyingly long and explicit.

@zockons12
Copy link
Author

I doubt I'm the only one who has come across this issue. Anyone who uses a host with auth and has to redirect with no auth has likely encountered this (so likely issue will be asked by someone else in the future if not fixed). I had a look at some SO posts. In the mean time, the only viable method seems to be using monkey patches. Can get quite length depending on how many different hosts you have. Another possibility could be to urlparse out the auth credentials though monkey patch works fine for now so never tested.

@haikuginger
Copy link
Contributor

@sigmavirus24, were you still planning on handling this? If not, I can make the change myself.

@ret2libc
Copy link

ret2libc commented Dec 5, 2018

Are you ok asking MITRE for a CVE for this? It would be a flaw similar to the python-requests one (https://bugzilla.redhat.com/show_bug.cgi?id=1643829). Something like "A credentials-exposure flaw was found in python-urllib3, where if a request with authentication is redirected from an HTTPS endpoint to an HTTP endpoint on the same host, the Authorization header is not stripped by default and the credentials can be read in plain text."

@sethmlarson
Copy link
Member

Of course! Make sure you get the affected versions correct.

@carnil
Copy link

carnil commented Dec 12, 2018

CVE-2018-20060 was assigned for this issue.

dojeda added a commit to quetz-al/quetzal-client that referenced this issue Feb 15, 2019
The authorization header may be propagated when creating a query, which gives a
303 code and requires a redirect, but since urllib3 removes headers, this has to
be done manually.
See urllib3/urllib3#1316
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants