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
net/url: malformed urls may lead to an "open redirect" vulnerability #38642
Comments
It's not clear to me that this is the right place for such a fix. There isn't anything currently in net/url that is HTTP-specific. URLs like Can you expand further on the case why net/url is the right place to make this change? Thanks. |
Package net/http did actually check the Let's check out how other programming languages deal with it: $ node
> new URL("https:///www.google.com")
URL {
href: 'https://www.google.com/',
origin: 'https://www.google.com',
protocol: 'https:',
username: '',
password: '',
host: 'www.google.com',
hostname: 'www.google.com',
port: '',
pathname: '/',
search: '',
searchParams: URLSearchParams {},
hash: '' } $ php -a
Interactive mode enabled
php > var_dump(parse_url("https:///www.google.com"));
bool(false) But, Python did in the same way as Golang, of which I think not friendly to programmers |
According to this spec document, I'm finding the same problem @Li4n0 reported, an open redirect because browsers interpret the URL as Edit: the spec document defines specific rules for several schemes, including |
We fell victim to this behaviour: golang/go#38642 The solution in this commit ensures that the URI we redirect to is always a path. The TrimLeft() removes all prefixed forward slashes, which covers the case where a redirect uri starts with two or more slashse. We then always add a single forward slash which will ensure that ParseRequestURI() interprets the URI as a path. Now if someone visits `http://ci.concourse-ci.org/sky/login?redirect_uri=//google.com/` they'll end up redirected to `http://ci.concourse-ci.org/google.com` which will return a 404 and redirect one more time to the homepage. In case the behaviour of ParseRequestURI() changes or is smarter in the future, added check to ensure that if either Host or Scheme is populated that the URI is not used for redirecting. The tests have also been updated so this change in behaviour will cause the tests to fail. Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Esteban Foronda <eforonda@vmware.com>
We fell victim to this behaviour: golang/go#38642 The solution in this commit ensures that the URI we redirect to is always a path. The TrimLeft() removes all prefixed forward slashes, which covers the case where a redirect uri starts with two or more slashse. We then always add a single forward slash which will ensure that ParseRequestURI() interprets the URI as a path. Now if someone visits `http://ci.concourse-ci.org/sky/login?redirect_uri=//google.com/` they'll end up redirected to `http://ci.concourse-ci.org/google.com` which will return a 404 and redirect one more time to the homepage. In case the behaviour of ParseRequestURI() changes or is smarter in the future, added check to ensure that if either Host or Scheme is populated that the URI is not used for redirecting. The tests have also been updated so this change in behaviour will cause the tests to fail. Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Esteban Foronda <eforonda@vmware.com>
We fell victim to this behaviour: golang/go#38642 The solution in this commit ensures that the URI we redirect to is always a path. The TrimLeft() removes all prefixed forward slashes, which covers the case where a redirect uri starts with two or more slashse. We then always add a single forward slash which will ensure that ParseRequestURI() interprets the URI as a path. Now if someone visits `http://ci.concourse-ci.org/sky/login?redirect_uri=//google.com/` they'll end up redirected to `http://ci.concourse-ci.org/google.com` which will return a 404 and redirect one more time to the homepage. In case the behaviour of ParseRequestURI() changes or is smarter in the future, added check to ensure that if either Host or Scheme is populated that the URI is not used for redirecting. The tests have also been updated so this change in behaviour will cause the tests to fail. Signed-off-by: Taylor Silva <tsilva@pivotal.io> Co-authored-by: Esteban Foronda <eforonda@vmware.com>
Timed out in state WaitingForInfo. Closing. (I am just a bot, though. Please speak up if this is a mistake or you have the requested information.) |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
What did you expect to see?
An error
What did you see instead?
Host:
Path: /www.google.com
What harm does it have?
This illegal url will lead to unexpected results when the server responses with 301/302/30x statuses, for our browsers will be very likely to fix it to a normal one. That results in an
open redirect vulnerability
, as the backend treats it like a relative-path jump while the browser treat it as a redirection to other sites.How to fix?
Check url's Scheme and Host as below:
The text was updated successfully, but these errors were encountered: