CVE-2019-9741

Origin: https://github.com/golang/go/commit/829c5df58694b3345cb5ea41206783c8ccf5c3ca
Origin: https://github.com/golang/go/commit/f1d662f34788f4a5f087581d0951cdf4e0f6e708
Reviewed-by: Sylvain Beucler <beuc@debian.org>
Last-Update: 2021-03-12

From 829c5df58694b3345cb5ea41206783c8ccf5c3ca Mon Sep 17 00:00:00 2001
From: Brad Fitzpatrick <bradfitz@golang.org>
Date: Wed, 23 Jan 2019 19:09:07 +0000
Subject: [PATCH] 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>
Gbp-Pq: Name CVE-2019-9741.patch

diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go
index c811891e871958f862d9bdcb15d49a825c3c0849..72aed3e9e652f5917ebdf81aea43c050eaf2b6fd 100644 (file)
--- a/src/net/http/fs_test.go
+++ b/src/net/http/fs_test.go
@@ -580,16 +580,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 b34ae41ec517b8e9ff34b8f5872f1e70cb37f2af..783b30babfa106814fc81545c5bfd0291feae9a2 100644 (file)
--- a/src/net/http/http.go
+++ b/src/net/http/http.go
@@ -41,3 +41,14 @@ func removeEmptyPort(host string) string {
 func isNotToken(r rune) bool {
        return !httplex.IsTokenRune(r)
 }
+
+// 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/http/request.go b/src/net/http/request.go
index dc5559282d0f1af771b9d6cf8d116b2284d99f16..23d05bec108ffc674562f947f7f56f711118c13f 100644 (file)
--- a/src/net/http/request.go
+++ b/src/net/http/request.go
@@ -477,7 +477,12 @@ func (req *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, wai
                // 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 2545f6f4c22f25b93facc306807b1378459e8b08..bb8785a2b49e2f40d3373f3a7f175cbd0d037384 100644 (file)
--- a/src/net/http/requestwrite_test.go
+++ b/src/net/http/requestwrite_test.go
@@ -486,6 +486,17 @@ var reqWriteTests = []reqWriteTest{
                        "User-Agent: Go-http-client/1.1\r\n" +
                        "\r\n",
        },
+
+       {
+               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 30e92779370393e4f425a379c18225ef8c971f63..38aef647421b808b8682a7c8f79466a0edb816a7 100644 (file)
--- a/src/net/url/url.go
+++ b/src/net/url/url.go
@@ -447,6 +447,10 @@ func ParseRequestURI(rawurl string) (*URL, error) {
 func parse(rawurl string, viaRequest bool) (url *URL, err error) {
        var rest string
 
+       if stringContainsCTLByte(rawurl) {
+               return nil, errors.New("net/url: invalid control character in URL")
+       }
+
        if rawurl == "" && viaRequest {
                err = errors.New("empty url")
                goto Error
@@ -929,3 +933,14 @@ func (u *URL) RequestURI() string {
        }
        return result
 }
+
+// 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 7560f22c4a1e2a7f258b7d4f2f256a6e02859938..c90e7ee2021b3e6dd63f1bb7d23e8c91bc77a07c 100644 (file)
--- a/src/net/url/url_test.go
+++ b/src/net/url/url_test.go
@@ -1388,6 +1388,12 @@ func TestShouldEscape(t *testing.T) {
                        t.Errorf("shouldEscape(%q, %v) returned %v; expected %v", tt.in, tt.mode, !tt.escape, tt.escape)
                }
        }
+
+       // 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)
+       }
+
 }
 
 type timeoutError struct {
@@ -1470,3 +1476,18 @@ func TestURLErrorImplementsNetError(t *testing.T) {
                }
        }
 }
+
+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)
+               }
+       }
+}