[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>

diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index 255d215f3cfe07baf174040e468d6270751f6c03..762e88b05ff30cf81b395a9b0a2bbc4a7dbf6bc0 100644 (file)
--- 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 ce0eceb1de303b67602f32235f756f011d21cc2a..07ca78dbc8406bbdcbcb8a3de876bfa608332df3 100644 (file)
--- 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 a40b0a3cb83855d551397f01e5a373d18bec6a06..e352386b083527c060a3b0bbf3f340773c5d2d03 100644 (file)
--- 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 eb65b9f736f5ba81e923f1b33ce837b757e1bc5b..3daab4b8b7bc66d0d0dfe4c9153cf1a88a500c62 100644 (file)
--- 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 80eb7a86c8de26f3e3110c1f0da58a4281d8b2ef..8d2a85669985f3829f56be1564c2e29aeb985d07 100644 (file)
--- 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 9043a844e88f800e8d5b206fba665ccbc2187efe..369ea6cbd25920e033686c50f98898bc637d29ff 100644 (file)
--- 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)
+       }
+
+}