code review 9766046: net/http: Fix authentication info leakage in Referer he...

net/http: Fix authentication info leakage in Referer header (potential security risk)

http.Client calls URL.String() to fill in the Referer header, which may
contain authentication info. This patch removes authentication info from the Referer header without introducing any API changes.

A new test for net/http is also provided.

[Hierro, 2013-05-31 13:06:15]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-06-02 01:36:00]

If this is worth fixing, I think we should fix it without any API changes.

AnonymousString() seems like a weird API addition.


On Fri, May 31, 2013 at 6:06 AM, <alberto.garcia.hierro@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/http: Fix authentication info leakage in Referer header (potential
> security risk)
>
> http.Client calls URL.String() to fill in the Referer header, which may
> contain authentication info. This patch adds a new method to net/url.URL
> called AnonymousString() which returns the URL as a string without any
> authentication info. It also changes net/http.Client to use that new
> function
> rather than String().
>
> New tests for both net/url and net/http are also provided.
>
> Please review this at
https://codereview.appspot.**com/9766046/<https://codereview.appspot.com/9766...
>
> Affected files:
>   M src/pkg/net/http/client.go
>   M src/pkg/net/http/client_test.**go
>   M src/pkg/net/url/url.go
>   M src/pkg/net/url/url_test.go
>
>
> Index: src/pkg/net/http/client.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client.go
> +++ b/src/pkg/net/http/client.go
> @@ -278,7 +278,7 @@
>                                 // Add the Referer header.
>                                 lastReq := via[len(via)-1]
>                                 if lastReq.URL.Scheme != "https" {
> -                                       req.Header.Set("Referer",
> lastReq.URL.String())
> +                                       req.Header.Set("Referer",
> lastReq.URL.AnonymousString())
>                                 }
>
>                                 err = redirectChecker(req, via)
> Index: src/pkg/net/http/client_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client_**test.go
> +++ b/src/pkg/net/http/client_**test.go
> @@ -735,3 +735,29 @@
>         }
>         defer resp.Body.Close()
>  }
> +
> +func TestPreventRefererAuthLeak(t *testing.T) {
> +       defer afterTest(t)
> +       ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
> *Request) {
> +               if r.URL.Path != "/" {
> +                       Redirect(w, r, "/", StatusFound)
> +                       return
> +               }
> +               referer := r.Header.Get("Referer")
> +               if strings.Contains(referer, "@") {
> +                       t.Errorf("Invalid referer %q, it contains
> authentication info", referer)
> +               }
> +       }))
> +       defer ts.Close()
> +       req, err := NewRequest("GET", ts.URL+"/foo", nil)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       req.URL.User = url.User("golang")
> +       c := &Client{}
> +       resp, err := c.Do(req)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       defer resp.Body.Close()
> +}
> Index: src/pkg/net/url/url.go
> ==============================**==============================**=======
> --- a/src/pkg/net/url/url.go
> +++ b/src/pkg/net/url/url.go
> @@ -439,8 +439,9 @@
>         return
>  }
>
> -// String reassembles the URL into a valid URL string.
> -func (u *URL) String() string {
> +// toString reassembles the URL into a valid URL string,
> +// optionally including user authentication info.
> +func (u *URL) toString(anon bool) string {
>         var buf bytes.Buffer
>         if u.Scheme != "" {
>                 buf.WriteString(u.Scheme)
> @@ -451,9 +452,11 @@
>         } else {
>                 if u.Scheme != "" || u.Host != "" || u.User != nil {
>                         buf.WriteString("//")
> -                       if u := u.User; u != nil {
> -                               buf.WriteString(u.String())
> -                               buf.WriteByte('@')
> +                       if !anon {
> +                               if u := u.User; u != nil {
> +                                       buf.WriteString(u.String())
> +                                       buf.WriteByte('@')
> +                               }
>                         }
>                         if h := u.Host; h != "" {
>                                 buf.WriteString(h)
> @@ -472,6 +475,17 @@
>         return buf.String()
>  }
>
> +// String reassembles the URL into a valid URL string.
> +func (u *URL) String() string {
> +       return u.toString(false)
> +}
> +
> +// AnonymousString reassembles the URL into a valid URL string
> +// omitting any user authentication info (if present).
> +func (u *URL) AnonymousString() string {
> +       return u.toString(true)
> +}
> +
>  // Values maps a string key to a list of values.
>  // It is typically used for query parameters and form values.
>  // Unlike in the http.Header map, the keys in a Values map
> Index: src/pkg/net/url/url_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/url/url_test.go
> +++ b/src/pkg/net/url/url_test.go
> @@ -868,3 +868,14 @@
>                 t.Errorf(`ParseQuery(%q) returned error %q, want something
> containing %q"`, url, errStr, "%gh")
>         }
>  }
> +
> +func TestAnonymous(t *testing.T) {
> +       const url =
"http://go:lang@www.example.**com<http://go:lang@www.example.com>
> "
> +       u, err := Parse(url)
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       if strings.Contains(u.**AnonymousString(), "@") {
> +               t.Errorf("%q contains authentication info",
> u.AnonymousString())
> +       }
> +}
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dsymonds, 2013-06-14 09:02:46]

R=bradfitz

[bradfitz, 2013-06-17 18:35:41]

Q=wait

Hierro, were you going to revise this to not introduce AnonymousString?

[Hierro, 2013-06-20 11:34:29]

On 2013/06/17 18:35:41, bradfitz wrote:
> Q=wait
> 
> Hierro, were you going to revise this to not introduce AnonymousString?

I was under the impression that you considered this issue not worth fixing. I'll
update the code to remove AnonymousString.

Regards,
Alberto

[Hierro, 2013-06-20 11:58:13]

PTAL

[bradfitz, 2013-06-20 20:23:30]

https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:280: if lastReq.URL.Scheme != "https" {
let's pull this out into a function.  then this code looks like:

if ref := refererForURL(lastReq.URL); ref != "" {
    req.Header.Set("Referer", ref)
}

// refererforURL returns the HTTP Referer header to send
// in a request after u was just fetched. If empty, no
// referer should be sent.
func refererforURL(u *url.URL) string {
...
}

And then you can easily write a table-driven test in client_test.go for that
function.

[Hierro, 2013-07-15 13:57:03]

On 2013/06/20 20:23:30, bradfitz wrote:
> https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go
> File src/pkg/net/http/client.go (right):
> 
>
https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go#...
> src/pkg/net/http/client.go:280: if lastReq.URL.Scheme != "https" {
> let's pull this out into a function.  then this code looks like:
> 
> if ref := refererForURL(lastReq.URL); ref != "" {
>     req.Header.Set("Referer", ref)
> }
> 
> // refererforURL returns the HTTP Referer header to send
> // in a request after u was just fetched. If empty, no
> // referer should be sent.
> func refererforURL(u *url.URL) string {
> ...
> }
> 
> And then you can easily write a table-driven test in client_test.go for that
> function.

Hi Brad,

client_test.go has its package declared as http_test, so it can't include
references to non-exported functions. What would be the most appropriate file in
net/http to put this new test?

Regards,
Alberto

[bradfitz, 2013-07-22 21:57:51]

On Mon, Jul 15, 2013 at 6:57 AM, <alberto.garcia.hierro@gmail.com> wrote:

> On 2013/06/20 20:23:30, bradfitz wrote:
>
> https://codereview.appspot.**com/9766046/diff/13001/src/**
>
pkg/net/http/client.go<https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go>
>
>> File src/pkg/net/http/client.go (right):
>>
>
>
> https://codereview.appspot.**com/9766046/diff/13001/src/**
>
pkg/net/http/client.go#**newcode280<https://codereview.appspot.com/9766046/diff/13001/src/pkg/net/http/client.go#newcode280>
>
>> src/pkg/net/http/client.go:**280: if lastReq.URL.Scheme != "https" {
>> let's pull this out into a function.  then this code looks like:
>>
>
>  if ref := refererForURL(lastReq.URL); ref != "" {
>>      req.Header.Set("Referer", ref)
>> }
>>
>
>  // refererforURL returns the HTTP Referer header to send
>> // in a request after u was just fetched. If empty, no
>> // referer should be sent.
>> func refererforURL(u *url.URL) string {
>> ...
>> }
>>
>
>  And then you can easily write a table-driven test in client_test.go
>>
> for that
>
>> function.
>>
>
> Hi Brad,
>
> client_test.go has its package declared as http_test, so it can't
> include references to non-exported functions. What would be the most
> appropriate file in net/http to put this new test?
>

See the file export_test.go --- it's in package http (so can access
unexported stuff) but is only compiled when in test mode, so it can export
things just for testing.

You can add a line below and similar to this one:

     var DefaultUserAgent = defaultUserAgent

Then you can add a test in client_test.go.

[Hierro, 2013-07-26 22:06:51]

> You can add a line below and similar to this one:
> 
>      var DefaultUserAgent = defaultUserAgent
> 
> Then you can add a test in client_test.go.

Thanks for the clarification. I've updated the patchset accordingly. PTAL.

Regards,
Alberto

[Hierro, 2013-10-17 14:16:08]

PING

[bradfitz, 2013-10-17 16:04:03]

Too late for Go 1.2  Please file a bug so we can track this for Go 1.3.
 This should've had an associated bug the whole time.




On Thu, Oct 17, 2013 at 4:16 AM, <alberto.garcia.hierro@gmail.com> wrote:

> PING
>
>
>
https://codereview.appspot.**com/9766046/<https://codereview.appspot.com/9766...
>

[Hierro, 2013-10-17 17:49:08]

On 2013/10/17 16:04:03, bradfitz wrote:
> Too late for Go 1.2  Please file a bug so we can track this for Go 1.3.
>  This should've had an associated bug the whole time.
> 
> 
> 
> 
> On Thu, Oct 17, 2013 at 4:16 AM, <mailto:alberto.garcia.hierro@gmail.com>
wrote:
> 
> > PING
> >
> >
> >
>
https://codereview.appspot.**com/9766046/%3Chttps://codereview.appspot.com/97...>
> >

 Noted. I'll file a bug and I'll bring this up again after Go 1.2.

Regards,
Alberto

[gobot, 2013-12-20 16:11:03]

Replacing golang-dev with golang-codereviews.

[dave_cheney.net, 2014-01-31 00:40:11]

On 2013/12/20 16:11:03, gobot wrote:
> Replacing golang-dev with golang-codereviews.

R=close

[bradfitz, 2014-07-23 20:09:41]

I see this never went in.

I've opened https://code.google.com/p/go/issues/detail?id=8417 to track it at
least.

Feel free to polish it up. In particular, I don't think the https part is
exactly correct yet: https still should send redirects to other https sites.

Perhaps remove that part for now and only address the username:password@ part
for now?