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

Jump to bottom

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

Closed
ejholmes opened this issue Mar 28, 2018 · 11 comments
Closed

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

ejholmes opened this issue Mar 28, 2018 · 11 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Milestone
Unplanned

Comments

@ejholmes
Copy link

ejholmes commented Mar 28, 2018 •
edited

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.
@gopherbot gopherbot added this to the Proposal milestone Mar 28, 2018
@jimmyfrasche
Copy link
Member

cc @bradfitz

@bradfitz
Copy link
Contributor

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.

@bradfitz bradfitz added Proposal-Accepted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 28, 2018
@bradfitz bradfitz modified the milestones: Proposal, Unplanned Mar 28, 2018
@bradfitz bradfitz changed the title Proposal: Sanitize username/password from url.Error net/url: omit URL's password when stringifying URL in Error Mar 28, 2018
@ALTree ALTree removed the Proposal label Mar 28, 2018
@gopherbot
Copy link

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

@adi93
Copy link

adi93 commented Mar 31, 2018 •
edited

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
Copy link
Member

ALTree commented Mar 31, 2018

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

@adi93
Copy link

adi93 commented Mar 31, 2018

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

@adamdecaf adamdecaf mentioned this issue Apr 13, 2018
Open
@adamdecaf adamdecaf mentioned this issue May 17, 2018
Merged
adamdecaf added a commit to adamdecaf/prometheus that referenced this issue May 17, 2018
@adamdecaf
discovery/marathon: include url in fetchApps error
2c766ee 
This was previously part of a larger PR, but that was closed.

prometheus#4048 (comment)

This change could include auth information in the URL. That's been
fixed in upstream go, but not until Go 1.11. See: golang/go#24572

Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
brian-brazil pushed a commit to prometheus/prometheus that referenced this issue May 18, 2018
@adamdecaf @brian-brazil
discovery/marathon: include url in fetchApps error (#4171)
a22e173 
This was previously part of a larger PR, but that was closed.

#4048 (comment)

This change could include auth information in the URL. That's been
fixed in upstream go, but not until Go 1.11. See: golang/go#24572

Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
@stevenh
Copy link
Contributor

stevenh commented Jun 7, 2018

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

@bradfitz
Copy link
Contributor

bradfitz commented Jun 7, 2018

@stevenh, file a new bug to request backports.

@adamdecaf adamdecaf mentioned this issue Jul 23, 2018
Closed
gouthamve pushed a commit to gouthamve/prometheus that referenced this issue Aug 1, 2018
@adamdecaf @gouthamve
discovery/marathon: include url in fetchApps error (prometheus#4171)
e39aae9 
This was previously part of a larger PR, but that was closed.

prometheus#4048 (comment)

This change could include auth information in the URL. That's been
fixed in upstream go, but not until Go 1.11. See: golang/go#24572

Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
@cheynearista
Copy link

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

@arthrarnld
Copy link

arthrarnld commented May 2, 2019 •
edited

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
Copy link
Contributor

bradfitz commented May 2, 2019

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

@arthrarnld arthrarnld mentioned this issue May 2, 2019
Closed
@golang golang locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
9 participants
@bradfitz @ejholmes @stevenh @jimmyfrasche @ALTree @arthrarnld @adi93 @gopherbot @cheynearista