]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.15-security] net/http/cgi,net/http/fcgi: add Content-Type detection
authorRoberto Clapis <roberto@golang.org>
Wed, 26 Aug 2020 06:53:03 +0000 (08:53 +0200)
committerFilippo Valsorda <valsorda@google.com>
Tue, 1 Sep 2020 12:31:45 +0000 (12:31 +0000)
This CL ensures that responses served via CGI and FastCGI
have a Content-Type header based on the content of the
response if not explicitly set by handlers.

If the implementers of the handler did not explicitly
specify a Content-Type both CGI implementations would default
to "text/html", potentially causing cross-site scripting.

Thanks to RedTeam Pentesting GmbH for reporting this.

Fixes CVE-2020-24553

Change-Id: I82cfc396309b5ab2e8d6e9a87eda8ea7e3799473
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/823217
Reviewed-by: Russ Cox <rsc@google.com>
(cherry picked from commit 23d675d07fdc56aafd67c0a0b63d5b7e14708ff0)
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/835311
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
src/net/http/cgi/child.go
src/net/http/cgi/child_test.go
src/net/http/cgi/integration_test.go
src/net/http/fcgi/child.go
src/net/http/fcgi/fcgi_test.go

index 9474175f1791eba414d2443909580c16ed0c7405..61de6165f6199917bd796bd62a88af4cbd3bfad5 100644 (file)
@@ -163,10 +163,12 @@ func Serve(handler http.Handler) error {
 }
 
 type response struct {
-       req        *http.Request
-       header     http.Header
-       bufw       *bufio.Writer
-       headerSent bool
+       req            *http.Request
+       header         http.Header
+       code           int
+       wroteHeader    bool
+       wroteCGIHeader bool
+       bufw           *bufio.Writer
 }
 
 func (r *response) Flush() {
@@ -178,26 +180,38 @@ func (r *response) Header() http.Header {
 }
 
 func (r *response) Write(p []byte) (n int, err error) {
-       if !r.headerSent {
+       if !r.wroteHeader {
                r.WriteHeader(http.StatusOK)
        }
+       if !r.wroteCGIHeader {
+               r.writeCGIHeader(p)
+       }
        return r.bufw.Write(p)
 }
 
 func (r *response) WriteHeader(code int) {
-       if r.headerSent {
+       if r.wroteHeader {
                // Note: explicitly using Stderr, as Stdout is our HTTP output.
                fmt.Fprintf(os.Stderr, "CGI attempted to write header twice on request for %s", r.req.URL)
                return
        }
-       r.headerSent = true
-       fmt.Fprintf(r.bufw, "Status: %d %s\r\n", code, http.StatusText(code))
+       r.wroteHeader = true
+       r.code = code
+}
 
-       // Set a default Content-Type
+// writeCGIHeader finalizes the header sent to the client and writes it to the output.
+// p is not written by writeHeader, but is the first chunk of the body
+// that will be written. It is sniffed for a Content-Type if none is
+// set explicitly.
+func (r *response) writeCGIHeader(p []byte) {
+       if r.wroteCGIHeader {
+               return
+       }
+       r.wroteCGIHeader = true
+       fmt.Fprintf(r.bufw, "Status: %d %s\r\n", r.code, http.StatusText(r.code))
        if _, hasType := r.header["Content-Type"]; !hasType {
-               r.header.Add("Content-Type", "text/html; charset=utf-8")
+               r.header.Set("Content-Type", http.DetectContentType(p))
        }
-
        r.header.Write(r.bufw)
        r.bufw.WriteString("\r\n")
        r.bufw.Flush()
index 14e0af475f5af90903a8f4668d44a173697eacea..f6ecb6eb80f24d3e163dea5d046c984f8590c4ce 100644 (file)
@@ -7,6 +7,11 @@
 package cgi
 
 import (
+       "bufio"
+       "bytes"
+       "net/http"
+       "net/http/httptest"
+       "strings"
        "testing"
 )
 
@@ -148,3 +153,67 @@ func TestRequestWithoutRemotePort(t *testing.T) {
                t.Errorf("RemoteAddr: got %q; want %q", g, e)
        }
 }
+
+type countingWriter int
+
+func (c *countingWriter) Write(p []byte) (int, error) {
+       *c += countingWriter(len(p))
+       return len(p), nil
+}
+func (c *countingWriter) WriteString(p string) (int, error) {
+       *c += countingWriter(len(p))
+       return len(p), nil
+}
+
+func TestResponse(t *testing.T) {
+       var tests = []struct {
+               name   string
+               body   string
+               wantCT string
+       }{
+               {
+                       name:   "no body",
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "html",
+                       body:   "<html><head><title>test page</title></head><body>This is a body</body></html>",
+                       wantCT: "text/html; charset=utf-8",
+               },
+               {
+                       name:   "text",
+                       body:   strings.Repeat("gopher", 86),
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "jpg",
+                       body:   "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
+                       wantCT: "image/jpeg",
+               },
+       }
+       for _, tt := range tests {
+               t.Run(tt.name, func(t *testing.T) {
+                       var buf bytes.Buffer
+                       resp := response{
+                               req:    httptest.NewRequest("GET", "/", nil),
+                               header: http.Header{},
+                               bufw:   bufio.NewWriter(&buf),
+                       }
+                       n, err := resp.Write([]byte(tt.body))
+                       if err != nil {
+                               t.Errorf("Write: unexpected %v", err)
+                       }
+                       if want := len(tt.body); n != want {
+                               t.Errorf("reported short Write: got %v want %v", n, want)
+                       }
+                       resp.writeCGIHeader(nil)
+                       resp.Flush()
+                       if got := resp.Header().Get("Content-Type"); got != tt.wantCT {
+                               t.Errorf("wrong content-type: got %q, want %q", got, tt.wantCT)
+                       }
+                       if !bytes.HasSuffix(buf.Bytes(), []byte(tt.body)) {
+                               t.Errorf("body was not correctly written")
+                       }
+               })
+       }
+}
index 32d59c09a3c52be726386978852960b3677c45b0..295c3b82d404ccd9bc520a0fb5cea2d90bfd5e67 100644 (file)
@@ -16,7 +16,9 @@ import (
        "io"
        "net/http"
        "net/http/httptest"
+       "net/url"
        "os"
+       "strings"
        "testing"
        "time"
 )
@@ -52,7 +54,7 @@ func TestHostingOurselves(t *testing.T) {
        }
        replay := runCgiTest(t, h, "GET /test.go?foo=bar&a=b HTTP/1.0\nHost: example.com\n\n", expectedMap)
 
-       if expected, got := "text/html; charset=utf-8", replay.Header().Get("Content-Type"); got != expected {
+       if expected, got := "text/plain; charset=utf-8", replay.Header().Get("Content-Type"); got != expected {
                t.Errorf("got a Content-Type of %q; expected %q", got, expected)
        }
        if expected, got := "X-Test-Value", replay.Header().Get("X-Test-Header"); got != expected {
@@ -152,6 +154,51 @@ func TestChildOnlyHeaders(t *testing.T) {
        }
 }
 
+func TestChildContentType(t *testing.T) {
+       testenv.MustHaveExec(t)
+
+       h := &Handler{
+               Path: os.Args[0],
+               Root: "/test.go",
+               Args: []string{"-test.run=TestBeChildCGIProcess"},
+       }
+       var tests = []struct {
+               name   string
+               body   string
+               wantCT string
+       }{
+               {
+                       name:   "no body",
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "html",
+                       body:   "<html><head><title>test page</title></head><body>This is a body</body></html>",
+                       wantCT: "text/html; charset=utf-8",
+               },
+               {
+                       name:   "text",
+                       body:   strings.Repeat("gopher", 86),
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "jpg",
+                       body:   "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
+                       wantCT: "image/jpeg",
+               },
+       }
+       for _, tt := range tests {
+               t.Run(tt.name, func(t *testing.T) {
+                       expectedMap := map[string]string{"_body": tt.body}
+                       req := fmt.Sprintf("GET /test.go?exact-body=%s HTTP/1.0\nHost: example.com\n\n", url.QueryEscape(tt.body))
+                       replay := runCgiTest(t, h, req, expectedMap)
+                       if got := replay.Header().Get("Content-Type"); got != tt.wantCT {
+                               t.Errorf("got a Content-Type of %q; expected it to start with %q", got, tt.wantCT)
+                       }
+               })
+       }
+}
+
 // golang.org/issue/7198
 func Test500WithNoHeaders(t *testing.T)     { want500Test(t, "/immediate-disconnect") }
 func Test500WithNoContentType(t *testing.T) { want500Test(t, "/no-content-type") }
@@ -203,6 +250,10 @@ func TestBeChildCGIProcess(t *testing.T) {
                if req.FormValue("no-body") == "1" {
                        return
                }
+               if eb, ok := req.Form["exact-body"]; ok {
+                       io.WriteString(rw, eb[0])
+                       return
+               }
                if req.FormValue("write-forever") == "1" {
                        io.Copy(rw, neverEnding('a'))
                        for {
index 30a6b2ce2dfe8335efce24b38c274bc0476fd8ed..a31273b3eca7a64216da830dae33fb0fc9005ab1 100644 (file)
@@ -74,10 +74,12 @@ func (r *request) parseParams() {
 
 // response implements http.ResponseWriter.
 type response struct {
-       req         *request
-       header      http.Header
-       w           *bufWriter
-       wroteHeader bool
+       req            *request
+       header         http.Header
+       code           int
+       wroteHeader    bool
+       wroteCGIHeader bool
+       w              *bufWriter
 }
 
 func newResponse(c *child, req *request) *response {
@@ -92,11 +94,14 @@ func (r *response) Header() http.Header {
        return r.header
 }
 
-func (r *response) Write(data []byte) (int, error) {
+func (r *response) Write(p []byte) (n int, err error) {
        if !r.wroteHeader {
                r.WriteHeader(http.StatusOK)
        }
-       return r.w.Write(data)
+       if !r.wroteCGIHeader {
+               r.writeCGIHeader(p)
+       }
+       return r.w.Write(p)
 }
 
 func (r *response) WriteHeader(code int) {
@@ -104,22 +109,34 @@ func (r *response) WriteHeader(code int) {
                return
        }
        r.wroteHeader = true
+       r.code = code
        if code == http.StatusNotModified {
                // Must not have body.
                r.header.Del("Content-Type")
                r.header.Del("Content-Length")
                r.header.Del("Transfer-Encoding")
-       } else if r.header.Get("Content-Type") == "" {
-               r.header.Set("Content-Type", "text/html; charset=utf-8")
        }
-
        if r.header.Get("Date") == "" {
                r.header.Set("Date", time.Now().UTC().Format(http.TimeFormat))
        }
+}
 
-       fmt.Fprintf(r.w, "Status: %d %s\r\n", code, http.StatusText(code))
+// writeCGIHeader finalizes the header sent to the client and writes it to the output.
+// p is not written by writeHeader, but is the first chunk of the body
+// that will be written. It is sniffed for a Content-Type if none is
+// set explicitly.
+func (r *response) writeCGIHeader(p []byte) {
+       if r.wroteCGIHeader {
+               return
+       }
+       r.wroteCGIHeader = true
+       fmt.Fprintf(r.w, "Status: %d %s\r\n", r.code, http.StatusText(r.code))
+       if _, hasType := r.header["Content-Type"]; r.code != http.StatusNotModified && !hasType {
+               r.header.Set("Content-Type", http.DetectContentType(p))
+       }
        r.header.Write(r.w)
        r.w.WriteString("\r\n")
+       r.w.Flush()
 }
 
 func (r *response) Flush() {
@@ -290,6 +307,8 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
                httpReq = httpReq.WithContext(envVarCtx)
                c.handler.ServeHTTP(r, httpReq)
        }
+       // Make sure we serve something even if nothing was written to r
+       r.Write(nil)
        r.Close()
        c.mu.Lock()
        delete(c.requests, req.reqId)
index e9d2b34023c81a392c6b1662eaff5a162f20cbfb..4a27a12c35a99e0cdb03d974161dd54deb65c0e7 100644 (file)
@@ -10,6 +10,7 @@ import (
        "io"
        "io/ioutil"
        "net/http"
+       "strings"
        "testing"
 )
 
@@ -344,3 +345,54 @@ func TestChildServeReadsEnvVars(t *testing.T) {
                <-done
        }
 }
+
+func TestResponseWriterSniffsContentType(t *testing.T) {
+       var tests = []struct {
+               name   string
+               body   string
+               wantCT string
+       }{
+               {
+                       name:   "no body",
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "html",
+                       body:   "<html><head><title>test page</title></head><body>This is a body</body></html>",
+                       wantCT: "text/html; charset=utf-8",
+               },
+               {
+                       name:   "text",
+                       body:   strings.Repeat("gopher", 86),
+                       wantCT: "text/plain; charset=utf-8",
+               },
+               {
+                       name:   "jpg",
+                       body:   "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
+                       wantCT: "image/jpeg",
+               },
+       }
+       for _, tt := range tests {
+               t.Run(tt.name, func(t *testing.T) {
+                       input := make([]byte, len(streamFullRequestStdin))
+                       copy(input, streamFullRequestStdin)
+                       rc := nopWriteCloser{bytes.NewBuffer(input)}
+                       done := make(chan bool)
+                       var resp *response
+                       c := newChild(rc, http.HandlerFunc(func(
+                               w http.ResponseWriter,
+                               r *http.Request,
+                       ) {
+                               io.WriteString(w, tt.body)
+                               resp = w.(*response)
+                               done <- true
+                       }))
+                       defer c.cleanUp()
+                       go c.serve()
+                       <-done
+                       if got := resp.Header().Get("Content-Type"); got != tt.wantCT {
+                               t.Errorf("got a Content-Type of %q; expected it to start with %q", got, tt.wantCT)
+                       }
+               })
+       }
+}