Skip to content

Commit 4f5cd0c

Browse files
Roberto ClapisFiloSottile
Roberto Clapis
authored and
FiloSottile
committed
net/http/cgi,net/http/fcgi: add Content-Type detection
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 #40928 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> Reviewed-on: https://go-review.googlesource.com/c/go/+/252179 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org>
1 parent 66e66e7 commit 4f5cd0c

File tree

5 files changed

+216
-22
lines changed

5 files changed

+216
-22
lines changed

‎src/net/http/cgi/child.go‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,12 @@ func Serve(handler http.Handler) error {
166166
}
167167

168168
type response struct {
169-
req *http.Request
170-
header http.Header
171-
bufw *bufio.Writer
172-
headerSent bool
169+
req *http.Request
170+
header http.Header
171+
code int
172+
wroteHeader bool
173+
wroteCGIHeader bool
174+
bufw *bufio.Writer
173175
}
174176

175177
func (r *response) Flush() {
@@ -181,26 +183,38 @@ func (r *response) Header() http.Header {
181183
}
182184

183185
func (r *response) Write(p []byte) (n int, err error) {
184-
if !r.headerSent {
186+
if !r.wroteHeader {
185187
r.WriteHeader(http.StatusOK)
186188
}
189+
if !r.wroteCGIHeader {
190+
r.writeCGIHeader(p)
191+
}
187192
return r.bufw.Write(p)
188193
}
189194

190195
func (r *response) WriteHeader(code int) {
191-
if r.headerSent {
196+
if r.wroteHeader {
192197
// Note: explicitly using Stderr, as Stdout is our HTTP output.
193198
fmt.Fprintf(os.Stderr, "CGI attempted to write header twice on request for %s", r.req.URL)
194199
return
195200
}
196-
r.headerSent = true
197-
fmt.Fprintf(r.bufw, "Status: %d %s\r\n", code, http.StatusText(code))
201+
r.wroteHeader = true
202+
r.code = code
203+
}
198204

199-
// Set a default Content-Type
205+
// writeCGIHeader finalizes the header sent to the client and writes it to the output.
206+
// p is not written by writeHeader, but is the first chunk of the body
207+
// that will be written. It is sniffed for a Content-Type if none is
208+
// set explicitly.
209+
func (r *response) writeCGIHeader(p []byte) {
210+
if r.wroteCGIHeader {
211+
return
212+
}
213+
r.wroteCGIHeader = true
214+
fmt.Fprintf(r.bufw, "Status: %d %s\r\n", r.code, http.StatusText(r.code))
200215
if _, hasType := r.header["Content-Type"]; !hasType {
201-
r.header.Add("Content-Type", "text/html; charset=utf-8")
216+
r.header.Set("Content-Type", http.DetectContentType(p))
202217
}
203-
204218
r.header.Write(r.bufw)
205219
r.bufw.WriteString("\r\n")
206220
r.bufw.Flush()

‎src/net/http/cgi/child_test.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@
77
package cgi
88

99
import (
10+
"bufio"
11+
"bytes"
12+
"net/http"
13+
"net/http/httptest"
14+
"strings"
1015
"testing"
1116
)
1217

@@ -148,3 +153,56 @@ func TestRequestWithoutRemotePort(t *testing.T) {
148153
t.Errorf("RemoteAddr: got %q; want %q", g, e)
149154
}
150155
}
156+
157+
func TestResponse(t *testing.T) {
158+
var tests = []struct {
159+
name string
160+
body string
161+
wantCT string
162+
}{
163+
{
164+
name: "no body",
165+
wantCT: "text/plain; charset=utf-8",
166+
},
167+
{
168+
name: "html",
169+
body: "<html><head><title>test page</title></head><body>This is a body</body></html>",
170+
wantCT: "text/html; charset=utf-8",
171+
},
172+
{
173+
name: "text",
174+
body: strings.Repeat("gopher", 86),
175+
wantCT: "text/plain; charset=utf-8",
176+
},
177+
{
178+
name: "jpg",
179+
body: "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
180+
wantCT: "image/jpeg",
181+
},
182+
}
183+
for _, tt := range tests {
184+
t.Run(tt.name, func(t *testing.T) {
185+
var buf bytes.Buffer
186+
resp := response{
187+
req: httptest.NewRequest("GET", "/", nil),
188+
header: http.Header{},
189+
bufw: bufio.NewWriter(&buf),
190+
}
191+
n, err := resp.Write([]byte(tt.body))
192+
if err != nil {
193+
t.Errorf("Write: unexpected %v", err)
194+
}
195+
if want := len(tt.body); n != want {
196+
t.Errorf("reported short Write: got %v want %v", n, want)
197+
}
198+
resp.writeCGIHeader(nil)
199+
resp.Flush()
200+
if got := resp.Header().Get("Content-Type"); got != tt.wantCT {
201+
t.Errorf("wrong content-type: got %q, want %q", got, tt.wantCT)
202+
}
203+
if !bytes.HasSuffix(buf.Bytes(), []byte(tt.body)) {
204+
t.Errorf("body was not correctly written")
205+
}
206+
})
207+
}
208+
}

‎src/net/http/cgi/integration_test.go‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ import (
1616
"io"
1717
"net/http"
1818
"net/http/httptest"
19+
"net/url"
1920
"os"
21+
"strings"
2022
"testing"
2123
"time"
2224
)
@@ -52,7 +54,7 @@ func TestHostingOurselves(t *testing.T) {
5254
}
5355
replay := runCgiTest(t, h, "GET /test.go?foo=bar&a=b HTTP/1.0\nHost: example.com\n\n", expectedMap)
5456

55-
if expected, got := "text/html; charset=utf-8", replay.Header().Get("Content-Type"); got != expected {
57+
if expected, got := "text/plain; charset=utf-8", replay.Header().Get("Content-Type"); got != expected {
5658
t.Errorf("got a Content-Type of %q; expected %q", got, expected)
5759
}
5860
if expected, got := "X-Test-Value", replay.Header().Get("X-Test-Header"); got != expected {
@@ -169,6 +171,51 @@ func TestNilRequestBody(t *testing.T) {
169171
_ = runCgiTest(t, h, "POST /test.go?nil-request-body=1 HTTP/1.0\nHost: example.com\nContent-Length: 0\n\n", expectedMap)
170172
}
171173

174+
func TestChildContentType(t *testing.T) {
175+
testenv.MustHaveExec(t)
176+
177+
h := &Handler{
178+
Path: os.Args[0],
179+
Root: "/test.go",
180+
Args: []string{"-test.run=TestBeChildCGIProcess"},
181+
}
182+
var tests = []struct {
183+
name string
184+
body string
185+
wantCT string
186+
}{
187+
{
188+
name: "no body",
189+
wantCT: "text/plain; charset=utf-8",
190+
},
191+
{
192+
name: "html",
193+
body: "<html><head><title>test page</title></head><body>This is a body</body></html>",
194+
wantCT: "text/html; charset=utf-8",
195+
},
196+
{
197+
name: "text",
198+
body: strings.Repeat("gopher", 86),
199+
wantCT: "text/plain; charset=utf-8",
200+
},
201+
{
202+
name: "jpg",
203+
body: "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
204+
wantCT: "image/jpeg",
205+
},
206+
}
207+
for _, tt := range tests {
208+
t.Run(tt.name, func(t *testing.T) {
209+
expectedMap := map[string]string{"_body": tt.body}
210+
req := fmt.Sprintf("GET /test.go?exact-body=%s HTTP/1.0\nHost: example.com\n\n", url.QueryEscape(tt.body))
211+
replay := runCgiTest(t, h, req, expectedMap)
212+
if got := replay.Header().Get("Content-Type"); got != tt.wantCT {
213+
t.Errorf("got a Content-Type of %q; expected it to start with %q", got, tt.wantCT)
214+
}
215+
})
216+
}
217+
}
218+
172219
// golang.org/issue/7198
173220
func Test500WithNoHeaders(t *testing.T) { want500Test(t, "/immediate-disconnect") }
174221
func Test500WithNoContentType(t *testing.T) { want500Test(t, "/no-content-type") }
@@ -224,6 +271,10 @@ func TestBeChildCGIProcess(t *testing.T) {
224271
if req.FormValue("no-body") == "1" {
225272
return
226273
}
274+
if eb, ok := req.Form["exact-body"]; ok {
275+
io.WriteString(rw, eb[0])
276+
return
277+
}
227278
if req.FormValue("write-forever") == "1" {
228279
io.Copy(rw, neverEnding('a'))
229280
for {

‎src/net/http/fcgi/child.go‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,12 @@ func (r *request) parseParams() {
7474

7575
// response implements http.ResponseWriter.
7676
type response struct {
77-
req *request
78-
header http.Header
79-
w *bufWriter
80-
wroteHeader bool
77+
req *request
78+
header http.Header
79+
code int
80+
wroteHeader bool
81+
wroteCGIHeader bool
82+
w *bufWriter
8183
}
8284

8385
func newResponse(c *child, req *request) *response {
@@ -92,34 +94,49 @@ func (r *response) Header() http.Header {
9294
return r.header
9395
}
9496

95-
func (r *response) Write(data []byte) (int, error) {
97+
func (r *response) Write(p []byte) (n int, err error) {
9698
if !r.wroteHeader {
9799
r.WriteHeader(http.StatusOK)
98100
}
99-
return r.w.Write(data)
101+
if !r.wroteCGIHeader {
102+
r.writeCGIHeader(p)
103+
}
104+
return r.w.Write(p)
100105
}
101106

102107
func (r *response) WriteHeader(code int) {
103108
if r.wroteHeader {
104109
return
105110
}
106111
r.wroteHeader = true
112+
r.code = code
107113
if code == http.StatusNotModified {
108114
// Must not have body.
109115
r.header.Del("Content-Type")
110116
r.header.Del("Content-Length")
111117
r.header.Del("Transfer-Encoding")
112-
} else if r.header.Get("Content-Type") == "" {
113-
r.header.Set("Content-Type", "text/html; charset=utf-8")
114118
}
115-
116119
if r.header.Get("Date") == "" {
117120
r.header.Set("Date", time.Now().UTC().Format(http.TimeFormat))
118121
}
122+
}
119123

120-
fmt.Fprintf(r.w, "Status: %d %s\r\n", code, http.StatusText(code))
124+
// writeCGIHeader finalizes the header sent to the client and writes it to the output.
125+
// p is not written by writeHeader, but is the first chunk of the body
126+
// that will be written. It is sniffed for a Content-Type if none is
127+
// set explicitly.
128+
func (r *response) writeCGIHeader(p []byte) {
129+
if r.wroteCGIHeader {
130+
return
131+
}
132+
r.wroteCGIHeader = true
133+
fmt.Fprintf(r.w, "Status: %d %s\r\n", r.code, http.StatusText(r.code))
134+
if _, hasType := r.header["Content-Type"]; r.code != http.StatusNotModified && !hasType {
135+
r.header.Set("Content-Type", http.DetectContentType(p))
136+
}
121137
r.header.Write(r.w)
122138
r.w.WriteString("\r\n")
139+
r.w.Flush()
123140
}
124141

125142
func (r *response) Flush() {
@@ -293,6 +310,8 @@ func (c *child) serveRequest(req *request, body io.ReadCloser) {
293310
httpReq = httpReq.WithContext(envVarCtx)
294311
c.handler.ServeHTTP(r, httpReq)
295312
}
313+
// Make sure we serve something even if nothing was written to r
314+
r.Write(nil)
296315
r.Close()
297316
c.mu.Lock()
298317
delete(c.requests, req.reqId)

‎src/net/http/fcgi/fcgi_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"io"
1111
"io/ioutil"
1212
"net/http"
13+
"strings"
1314
"testing"
1415
)
1516

@@ -344,3 +345,54 @@ func TestChildServeReadsEnvVars(t *testing.T) {
344345
<-done
345346
}
346347
}
348+
349+
func TestResponseWriterSniffsContentType(t *testing.T) {
350+
var tests = []struct {
351+
name string
352+
body string
353+
wantCT string
354+
}{
355+
{
356+
name: "no body",
357+
wantCT: "text/plain; charset=utf-8",
358+
},
359+
{
360+
name: "html",
361+
body: "<html><head><title>test page</title></head><body>This is a body</body></html>",
362+
wantCT: "text/html; charset=utf-8",
363+
},
364+
{
365+
name: "text",
366+
body: strings.Repeat("gopher", 86),
367+
wantCT: "text/plain; charset=utf-8",
368+
},
369+
{
370+
name: "jpg",
371+
body: "\xFF\xD8\xFF" + strings.Repeat("B", 1024),
372+
wantCT: "image/jpeg",
373+
},
374+
}
375+
for _, tt := range tests {
376+
t.Run(tt.name, func(t *testing.T) {
377+
input := make([]byte, len(streamFullRequestStdin))
378+
copy(input, streamFullRequestStdin)
379+
rc := nopWriteCloser{bytes.NewBuffer(input)}
380+
done := make(chan bool)
381+
var resp *response
382+
c := newChild(rc, http.HandlerFunc(func(
383+
w http.ResponseWriter,
384+
r *http.Request,
385+
) {
386+
io.WriteString(w, tt.body)
387+
resp = w.(*response)
388+
done <- true
389+
}))
390+
defer c.cleanUp()
391+
go c.serve()
392+
<-done
393+
if got := resp.Header().Get("Content-Type"); got != tt.wantCT {
394+
t.Errorf("got a Content-Type of %q; expected it to start with %q", got, tt.wantCT)
395+
}
396+
})
397+
}
398+
}

0 commit comments

Comments
 (0)