From eb0f2b3d27a896e4b832f2450490a2bbf72fbb6c Mon Sep 17 00:00:00 2001

From: Brad Fitzpatrick <bradfitz@golang.org>

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 <bradfitz@golang.org>

TryBot-Result: Gobot Gobot <gobot@golang.org>

Reviewed-by: Ian Lance Taylor <iant@golang.org>

---

 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)

+	}

+

+}