net/url: omit URL's password when stringifying URL in Error

[ejholmes]

It's fairly common practice to pack basic auth credentials in a URL string:

http://user:pass@localhost:1234

When basic authentication credentials are provided in this manner, net/http will automatically set the Authorization header before sending the request, however, net/http leaves the original URL string un-modified.

This has the potential to create nasty little security problems when errors occur. Consider the following program:

package main

import (
	"log"
	"net/http"
)

func main() {
	_, err := http.Get("http://user:pass@localhost:12345")
	log.Fatal(err)
}

When ran, you'll see the following output:

2018/03/27 19:44:44 Get http://user:pass@localhost:12345: dial tcp [::1]:12345: getsockopt: connection refused
exit status 1

It's fairly common practice in Go programs to bubble errors up the call stack. In many cases, these errors can be shown to untrusted parties if care isn't taken. Obviously, there's a lot of things that can be done to mitigate this (don't pass creds in the URL string, don't bubble up internal errors to untrusted parties, use http signatures instead of long lived creds, etc), but I think Go could provide a better secure default here. Equivalent versions in ruby and python will not display the credentials.

I'd propose that this block be changed so that, if the condition matches, the basic auth creds are stripped from the raw URL string, so that they won't end up in url.Error's.

[jimmyfrasche]

cc @bradfitz

[bradfitz]

It's fairly common practice to pack basic auth credentials in a URL string

It's really not that common, and it's been dying out for years, but this proposal sounds fine to me.

[gopherbot]

Change https://golang.org/cl/102855 mentions this issue: net/http: strip password from error message

[adi93]

Hi, I am a new contributor, can I start working on this?

I am thinking of modifying "urlStr" after line 521 in the block given by @ejholmes so that if it contains strings like "pass=" or "password=", then it replaces the password by "<PASSWORD>" string.

For example, the above error:
2018/03/27 19:44:44 Get http://user:pass@localhost:12345: dial tcp [::1]:12345: getsockopt: connection refused exit status 1
will now appear as:
2018/03/27 19:44:44 Get http://user:<PASSWORD>@localhost:12345: dial tcp [::1]:12345: getsockopt: connection refused exit status 1

[ALTree]

@adi93 It appears that someone has already sent a patch to fix this. It's linked in the message above yours.

[adi93]

My mistake, I didn't notice that, and his approach is much better than mine too. Still, plenty of help wanted tags here.

[stevenh]

Any chance of getting a30d24f cherry picked into the next release?

[bradfitz]

@stevenh, file a new bug to request backports.

[cheynearista]

Stumbled upon this case not covered.
https://username:password%40123@localhost:1234

[arthrarnld]

As @cheynearista pointed out, the issue persists when there are URL-encoded characters in the password. See this example: https://play.golang.org/p/1MA3niQ8-sG.

[bradfitz]

@cheynearista, @arthurpaimarnold, please file a new bug. This bug is closed and therefore not tracked anywhere on our dashboards or release process.

You can reference this bug from your new bug.