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: in Parse, strip password when copying input into Error #53993

Closed
ekeric13 opened this issue Jul 22, 2022 · 8 comments
Closed

net/url: in Parse, strip password when copying input into Error #53993

ekeric13 opened this issue Jul 22, 2022 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ekeric13
Copy link

Slightly different iteration of this issue: #24572
Just using a different string

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

go1.18.4

Does this issue reproduce with the latest release?

With the latest stable release

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

Reproduced in go playground

What did you do?

https://play.golang.com/p/8GXuAZ66w3V

	myUrl := "mysql://user:password@example.com:3306/db\n"
	_, err := url.Parse(myUrl)
	fmt.Println(err)

What did you expect to see?

parse "mysql://user:***@example.com:3306/db\n": net/url: invalid control character in URL

What did you see instead?

parse "mysql://user:password@example.com:3306/db\n": net/url: invalid control character in URL
@mengzhuo
Copy link
Contributor

Maybe we should add a SafeParse to net.url that masked all contents before @?

# SafeParse will mask all user/password existed in url
func SafeParse(rawURL string, mask string) (*URL, error) {
	u, e := Parse(rawURL)
	if err != nil {
		ci := strings.Index(err.URL, "://")
		if ci < 0 {
			ci = 0
		} else {
			ci += 3
		}
		si := strings.IndexByte(err.URL, '@')
		if si < ci {
			return u, err
		}
		err.URL = err.URL[:ci] + mask + err.URL[si:]
	}
	return u, err
}

cc @neild

@mengzhuo mengzhuo added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 22, 2022
@rsc
Copy link
Contributor

rsc commented Jul 22, 2022

I don't believe SafeParse is the answer. Perhaps we should do something about this, but perhaps not. There are lots of ways to provide malformed input that might leak passwords. What if someone does url.Parse("mysql://user:password2example.com:3306/db") instead?
Or url.Parse("mysql://user/password@example.com:3306/db")?
The list goes on.

@rsc rsc changed the title net/url: omit URL's password when stringifying URL in Error net/url: in Parse, strip password when copying input into Error Jul 22, 2022
@ekeric13
Copy link
Author

Is there another way to avoid getting your password leaked and still use this library?
I have tried this:

myUrl := &url.URL{
	User: url.UserPassword("user", "password"),
	Scheme: "mysql",
	Host: "example.com",
	Path: "/db\n",
}

https://play.golang.com/p/9AAL2wAZ0_t
But that simply ignores the control character.

There does seem to be some precedent of having specific parses for special cases:
https://pkg.go.dev/net/url#ParseRequestURI
Though I don't know the history of this package as well as you do.

You mention as an example mysql://user:password2example.com:3306/db, but I thought that some assumptions were made about the url format, like the ampersand:
https://github.com/golang/go/blob/master/src/net/url/url.go#L345

// The general form represented is:
//
//	[scheme:][//[userinfo@]host][/]path[?query][#fragment]
//

Assumptions seemingly in line with this section of the RFC that the url package follows.:
https://www.rfc-editor.org/rfc/rfc3986.html#section-3.2

authority   = [ userinfo "@" ] host [ ":" port ]

Given what is said on the user information section:
https://www.rfc-editor.org/rfc/rfc3986.html#section-3.2.1
Specifically:

Use of the format "user:password" in the userinfo field is
   deprecated.  Applications should not render as clear text any data
   after the first colon (":") character found within a userinfo
   subcomponent unless the data

It is clear that it is not fair to assume a user:password, but does seem to imply that if there is one, the password should be redacted.
Maybe it is best to just redact all the userinfo, aka everything between the scheme and the host?

@tdakkota
Copy link

tdakkota commented Aug 1, 2022

@ekeric13 You should not print the original error, if you worry about password leak.

You can unwrap the original url.Error and get the error message without parse %q part (playground)

	parsed, err := url.Parse(rawURL)
	if err != nil {
		if uerr := new(url.Error); errors.As(err, &uerr) {
			return nil, uerr.Err
		}
		return nil, errors.New("invalid URL")
	}

But still, there is no guarantee that it's impossible to manage parser to get the password from the input.

@ekeric13
Copy link
Author

ekeric13 commented Aug 3, 2022

thanks @tdakkota . That is a more refined approach of what I am currently doing, which is avoiding printing the error. But I didn't realize you can pull out only part of the error message.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@phenixrizen
Copy link

phenixrizen commented Mar 29, 2023

I don't believe SafeParse is the answer. Perhaps we should do something about this, but perhaps not. There are lots of ways to provide malformed input that might leak passwords. What if someone does url.Parse("mysql://user:password2example.com:3306/db") instead? Or url.Parse("mysql://user/password@example.com:3306/db")? The list goes on.

@rsc This is true but why return the URL in the error? The caller knows the rawURL so if they want to be insecure and return a URL in their error let them do that. Golang should not be returning a password in a error this is horrible security and a really bad choice.

As a workaround I do this in my code but I can't guarantee other packages are as responsible that use url.Parse():

// Validate the URL.
_, err := url.Parse(rawURL)
if err != nil {
	// Unwrap the error to get the original error.
	// So that creds are not in the returned error.
	err := errors.Unwrap(err)
	return "", fmt.Errorf("cannot parse url: %w", err)
}

@neild
Copy link
Contributor

neild commented Mar 29, 2023

If you don't want to information about the (un)parsed URL in the error, then the simplest and most robust thing to do is to replace the error returned by url.Parse:

_, err := url.Parse(rawURL)
if err != nil {
  return errors.New("invalid URL")
}

Parse shouldn't try to strip sensitive information from the URL in the error. By definition, the URL is unparseable; how can we reliably remove information from it when we can't be sure where that information is?

Perhaps Parse shouldn't return the URL in the error text at all. We could change url.Error.Error to omit the URL. This would be inconsistent with other parsing functions in the standard library, such as the ones in strconv, which generally return the unparsable input text.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

In Go, errors include information about what failed. Effective Go discusses why this is a good thing (see the part about the error returned from os.Open - the same considerations apply to strconv.Atoi or url.Parse or any other function).

@rsc rsc closed this as completed Mar 29, 2023
@rsc rsc closed this as not planned Won't fix, can't repro, duplicate, stale Mar 29, 2023
@golang golang locked and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants