Blame SOURCES/reject-control-chars.patch

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