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

net/url: malformed urls may lead to an "open redirect" vulnerability #38642

Closed
Li4n0 opened this issue Apr 24, 2020 · 4 comments
Closed

net/url: malformed urls may lead to an "open redirect" vulnerability #38642

Li4n0 opened this issue Apr 24, 2020 · 4 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@Li4n0
Copy link

Li4n0 commented Apr 24, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOARCH="amd64"

What did you do?

package main

import (
	"fmt"
	"net/url"
)

func main() {
	u,err := url.Parse("https:///www.google.com")
	if err != nil{
		fmt.Println(err)
	}
	fmt.Printf("Host:%s\nPath:%s",u.Host,u.Path)
}

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:

if (u.Scheme  == "http"  || u.Scheme == "https" ) && u.Host == ""{
     return errors.New("wrong url")
}
@ianlancetaylor ianlancetaylor changed the title net/url An incorrect way of parsing malformed urls may lead to an "open redirect" vulnerability in some situations net/url: malformed urls may lead to an "open redirect" vulnerability Apr 24, 2020
@ianlancetaylor
Copy link
Contributor

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 file:///home/www/x.html are valid uses of a missing Host. The net/http package already checks for an empty Host field (https://golang.org/src/net/http/transport.go#L527).

Can you expand further on the case why net/url is the right place to make this change? Thanks.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 24, 2020
@Li4n0
Copy link
Author

Li4n0 commented Apr 25, 2020

Package net/http did actually check the host field, but not in http.Redirect. When dealing with such a malformed URL, it could cause some unexpected results. The reason why we should fix it in net/url instead of http is that, net/url is more essential. If we just fix it in http then the guys using non-builtin http client, like go-curl, will still be afftected by this issue.

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

@jgimenez
Copy link

jgimenez commented Feb 12, 2021

According to this spec document, https:///www.google.com is an invalid URL but can be interpreted as https://www.google.com///. I would expect that parsing this URL would either return a validation error (probably simpler) or an URL struct that represents the interpreted version (more powerful).

I'm finding the same problem @Li4n0 reported, an open redirect because browsers interpret the URL as https://www.google.com///.

Edit: the spec document defines specific rules for several schemes, including http/https but also others, which makes me think the right place to fix this would be in net/url.

taylorsilva added a commit to concourse/concourse that referenced this issue Aug 25, 2021
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>
taylorsilva added a commit to concourse/concourse that referenced this issue Aug 25, 2021
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>
taylorsilva added a commit to concourse/concourse that referenced this issue Aug 27, 2021
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>
@seankhliao seankhliao added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Aug 27, 2022
@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants