From: Go Compiler Team Date: Sat, 13 Mar 2021 14:48:57 +0000 (+0000) Subject: CVE-2019-9741 X-Git-Tag: archive/raspbian/1.7.4-2+rpi1+deb9u3^2~4 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=cd7e77dc3aa118933cee75009b561d5ec08850ab;p=golang-1.7.git CVE-2019-9741 Origin: https://github.com/golang/go/commit/829c5df58694b3345cb5ea41206783c8ccf5c3ca Origin: https://github.com/golang/go/commit/f1d662f34788f4a5f087581d0951cdf4e0f6e708 Reviewed-by: Sylvain Beucler Last-Update: 2021-03-12 From 829c5df58694b3345cb5ea41206783c8ccf5c3ca Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick 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 TryBot-Result: Gobot Gobot Reviewed-by: Filippo Valsorda Gbp-Pq: Name CVE-2019-9741.patch --- diff --git a/src/net/http/fs_test.go b/src/net/http/fs_test.go index c811891..72aed3e 100644 --- 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 b34ae41..783b30b 100644 --- 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 dc55592..23d05be 100644 --- 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 2545f6f..bb8785a 100644 --- 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 30e9277..38aef64 100644 --- 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 7560f22..c90e7ee 100644 --- 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) + } + } +}