From eb0f2b3d27a896e4b832f2450490a2bbf72fbb6c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 31 Jan 2019 20:17:12 +0000 Subject: [PATCH] [release-branch.go1.11] net/http, net/url: reject control characters in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29923 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160798 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/net/http/fs_test.go | 15 +++++++++++---- src/net/http/http.go | 11 +++++++++++ src/net/http/request.go | 7 ++++++- src/net/http/requestwrite_test.go | 11 +++++++++++ src/net/url/url.go | 15 +++++++++++++++ src/net/url/url_test.go | 23 ++++++++++++++++++++++- 6 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go index 255d215f3cf..762e88b05ff 100644 --- a/src/net/http/fs_test.go +++ b/src/net/http/fs_test.go @@ -583,16 +583,23 @@ func TestFileServerZeroByte(t *testing.T) { ts := httptest.NewServer(FileServer(Dir("."))) defer ts.Close() - res, err := Get(ts.URL + "/..\x00") + c, err := net.Dial("tcp", ts.Listener.Addr().String()) if err != nil { t.Fatal(err) } - b, err := ioutil.ReadAll(res.Body) + defer c.Close() + _, err = fmt.Fprintf(c, "GET /..\x00 HTTP/1.0\r\n\r\n") + if err != nil { + t.Fatal(err) + } + var got bytes.Buffer + bufr := bufio.NewReader(io.TeeReader(c, &got)) + res, err := ReadResponse(bufr, nil) if err != nil { - t.Fatal("reading Body:", err) + t.Fatal("ReadResponse: ", err) } if res.StatusCode == 200 { - t.Errorf("got status 200; want an error. Body is:\n%s", string(b)) + t.Errorf("got status 200; want an error. Body is:\n%s", got.Bytes()) } } diff --git a/src/net/http/http.go b/src/net/http/http.go index ce0eceb1de3..07ca78dbc84 100644 --- a/src/net/http/http.go +++ b/src/net/http/http.go @@ -59,6 +59,17 @@ func isASCII(s string) bool { return true } +// stringContainsCTLByte reports whether s contains any ASCII control character. +func stringContainsCTLByte(s string) bool { + for i := 0; i < len(s); i++ { + b := s[i] + if b < ' ' || b == 0x7f { + return true + } + } + return false +} + func hexEscapeNonASCII(s string) string { newLen := 0 for i := 0; i < len(s); i++ { diff --git a/src/net/http/request.go b/src/net/http/request.go index a40b0a3cb83..e352386b083 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -545,7 +545,12 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF // CONNECT requests normally give just the host and port, not a full URL. ruri = host } - // TODO(bradfitz): escape at least newlines in ruri? + if stringContainsCTLByte(ruri) { + return errors.New("net/http: can't write control character in Request.URL") + } + // TODO: validate r.Method too? At least it's less likely to + // come from an attacker (more likely to be a constant in + // code). // Wrap the writer in a bufio Writer if it's not already buffered. // Don't always call NewWriter, as that forces a bytes.Buffer diff --git a/src/net/http/requestwrite_test.go b/src/net/http/requestwrite_test.go index eb65b9f736f..3daab4b8b7b 100644 --- a/src/net/http/requestwrite_test.go +++ b/src/net/http/requestwrite_test.go @@ -512,6 +512,17 @@ var reqWriteTests = []reqWriteTest{ "User-Agent: Go-http-client/1.1\r\n" + "\r\n", }, + + 21: { + Req: Request{ + Method: "GET", + URL: &url.URL{ + Host: "www.example.com", + RawQuery: "new\nline", // or any CTL + }, + }, + WantError: errors.New("net/http: can't write control character in Request.URL"), + }, } func TestRequestWrite(t *testing.T) { diff --git a/src/net/url/url.go b/src/net/url/url.go index 80eb7a86c8d..8d2a8566998 100644 --- a/src/net/url/url.go +++ b/src/net/url/url.go @@ -494,6 +494,10 @@ func parse(rawurl string, viaRequest bool) (*URL, error) { var rest string var err error + if stringContainsCTLByte(rawurl) { + return nil, errors.New("net/url: invalid control character in URL") + } + if rawurl == "" && viaRequest { return nil, errors.New("empty url") } @@ -1114,3 +1118,14 @@ func validUserinfo(s string) bool { } return true } + +// stringContainsCTLByte reports whether s contains any ASCII control character. +func stringContainsCTLByte(s string) bool { + for i := 0; i < len(s); i++ { + b := s[i] + if b < ' ' || b == 0x7f { + return true + } + } + return false +} diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go index 9043a844e88..369ea6cbd25 100644 --- a/src/net/url/url_test.go +++ b/src/net/url/url_test.go @@ -1738,8 +1738,29 @@ func TestNilUser(t *testing.T) { } func TestInvalidUserPassword(t *testing.T) { - _, err := Parse("http://us\ner:pass\nword@foo.com/") + _, err := Parse("http://user^:passwo^rd@foo.com/") if got, wantsub := fmt.Sprint(err), "net/url: invalid userinfo"; !strings.Contains(got, wantsub) { t.Errorf("error = %q; want substring %q", got, wantsub) } } + +func TestRejectControlCharacters(t *testing.T) { + tests := []string{ + "http://foo.com/?foo\nbar", + "http\r://foo.com/", + "http://foo\x7f.com/", + } + for _, s := range tests { + _, err := Parse(s) + const wantSub = "net/url: invalid control character in URL" + if got := fmt.Sprint(err); !strings.Contains(got, wantSub) { + t.Errorf("Parse(%q) error = %q; want substring %q", s, got, wantSub) + } + } + + // But don't reject non-ASCII CTLs, at least for now: + if _, err := Parse("http://foo.com/ctl\x80"); err != nil { + t.Errorf("error parsing URL with non-ASCII control byte: %v", err) + } + +}