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

[ekeric13]

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]

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

[rsc]

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.

[ekeric13]

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?

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

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.

[phenixrizen]

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]

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]

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