net/url, net/http: reject control characters in URLs

This is a more conservative version of the reverted CL 99135 (which
was reverted in CL 137716)

The net/url part rejects URLs with ASCII CTLs from being parsed and
the net/http part rejects writing them if a bogus url.URL is
constructed otherwise.

Updates #27302
Updates #22907

Change-Id: I09a2212eb74c63db575223277aec363c55421ed8
Reviewed-on: https://go-review.googlesource.com/c/159157
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@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 624b2cfe6956751bce1e81a7519cdcf510548a77..5c03c16c873cd263167b167868daf758d57f24bd 100644 (file)
--- a/src/net/http/http.go
+++ b/src/net/http/http.go
@@ -59,6 +59,12 @@ func isASCII(s string) bool {
        return true
 }
 
+// isCTL reports whether r is an ASCII control character, including
+// the Extended ASCII control characters included in Unicode.
+func isCTL(r rune) bool {
+       return r < ' ' || 0x7f <= r && r <= 0x9f
+}
+
 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 fb058f9fbf38ff27e6f36802149573303b41317a..01ba1dc1fb1fbe3f724d66952522ac20433b816c 100644 (file)
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -550,7 +550,12 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF
                        ruri = r.URL.Opaque
                }
        }
-       // TODO(bradfitz): escape at least newlines in ruri?
+       if strings.IndexFunc(ruri, isCTL) != -1 {
+               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 7dbf0d4e8a175328e82728835b549d41221f4acc..b110b57b1ab922ec213c10311891c1a2eb6dd9ad 100644 (file)
--- a/src/net/http/requestwrite_test.go
+++ b/src/net/http/requestwrite_test.go
@@ -576,6 +576,17 @@ var reqWriteTests = []reqWriteTest{
                        "User-Agent: Go-http-client/1.1\r\n" +
                        "X-Foo: X-Bar\r\n\r\n",
        },
+
+       25: {
+               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 d84c95adb031899749e8a4d4575f8968a24ef4de..77078ade1bd962623d1a5569315b57c2244f5494 100644 (file)
--- a/src/net/url/url.go
+++ b/src/net/url/url.go
@@ -513,6 +513,10 @@ func parse(rawurl string, viaRequest bool) (*URL, error) {
        var rest string
        var err error
 
+       if strings.IndexFunc(rawurl, isCTL) != -1 {
+               return nil, errors.New("net/url: invalid control character in URL")
+       }
+
        if rawurl == "" && viaRequest {
                return nil, errors.New("empty url")
        }
@@ -1134,3 +1138,9 @@ func validUserinfo(s string) bool {
        }
        return true
 }
+
+// isCTL reports whether r is an ASCII control character, including
+// the Extended ASCII control characters included in Unicode.
+func isCTL(r rune) bool {
+       return r < ' ' || 0x7f <= r && r <= 0x9f
+}

diff --git a/src/net/url/url_test.go b/src/net/url/url_test.go
index 7c4ada245a7a3d9f34501711f367378c66c9e2e2..43d77f090c96adb46e83ee29a74cf9743b5748c7 100644 (file)
--- a/src/net/url/url_test.go
+++ b/src/net/url/url_test.go
@@ -1738,12 +1738,27 @@ 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)
+               }
+       }
+}
+
 var escapeBenchmarks = []struct {
        unescaped string
        query     string