From d4cf10b9d6c5fecff11609a180e23d1d49112bd3 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Thu, 31 Jan 2019 20:17:12 +0000 Subject: [release-branch.go1.10] net/http, net/url: reject control characters in URLs Cherry pick of combined CL 159157 + CL 160178. Fixes #29922 Updates #27302 Updates #22907 Change-Id: I6de92c14284595a58321a4b4d53229285979b872 Reviewed-on: https://go-review.googlesource.com/c/160678 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 6ab122cf82..92aa06f16e 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 b95ca89f40..8bcd49e3b0 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 c9642e55c2..29e066d244 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -528,7 +528,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 eb65b9f736..3daab4b8b7 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 3e12179542..af61156406 100644 --- a/src/net/url/url.go +++ b/src/net/url/url.go @@ -483,6 +483,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") } @@ -1102,3 +1106,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 f2d311a998..3e888aada1 100644 --- a/src/net/url/url_test.go +++ b/src/net/url/url_test.go @@ -1737,8 +1737,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) + } + +} -- cgit v1.2.3-54-g00ecf