From cad4e97af8f2e0b9f09b97f67fb3a89ced2e9021 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 18 Jul 2016 06:05:24 +0000 Subject: [PATCH] [release-branch.go1.7] net/http, net/http/cgi: fix for CGI + HTTP_PROXY security issue Because, * The CGI spec defines that incoming request header "Foo: Bar" maps to environment variable HTTP_FOO == "Bar". (see RFC 3875 4.1.18) * The HTTP_PROXY environment variable is conventionally used to configure the HTTP proxy for HTTP clients (and is respected by default for Go's net/http.Client and Transport) That means Go programs running in a CGI environment (as a child process under a CGI host) are vulnerable to an incoming request containing "Proxy: attacker.com:1234", setting HTTP_PROXY, and changing where Go by default proxies all outbound HTTP requests. This is CVE-2016-5386, aka https://httpoxy.org/ Fixes #16405 Change-Id: I6f68ade85421b4807785799f6d98a8b077e871f0 Reviewed-on: https://go-review.googlesource.com/25010 Run-TryBot: Chris Broadfoot TryBot-Result: Gobot Gobot Reviewed-by: Chris Broadfoot Reviewed-on: https://go-review.googlesource.com/25013 --- src/net/http/cgi/host.go | 4 ++++ src/net/http/cgi/host_test.go | 37 +++++++++++++++++++++++++++++++--- src/net/http/transport.go | 3 +++ src/net/http/transport_test.go | 14 ++++++++++++- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/src/net/http/cgi/host.go b/src/net/http/cgi/host.go index 2eea64334b..58e9f7132a 100644 --- a/src/net/http/cgi/host.go +++ b/src/net/http/cgi/host.go @@ -153,6 +153,10 @@ func (h *Handler) ServeHTTP(rw http.ResponseWriter, req *http.Request) { for k, v := range req.Header { k = strings.Map(upperCaseAndUnderscore, k) + if k == "PROXY" { + // See Issue 16405 + continue + } joinStr := ", " if k == "COOKIE" { joinStr = "; " diff --git a/src/net/http/cgi/host_test.go b/src/net/http/cgi/host_test.go index 70c5aff5e2..11213349a7 100644 --- a/src/net/http/cgi/host_test.go +++ b/src/net/http/cgi/host_test.go @@ -35,15 +35,18 @@ func newRequest(httpreq string) *http.Request { return req } -func runCgiTest(t *testing.T, h *Handler, httpreq string, expectedMap map[string]string) *httptest.ResponseRecorder { +func runCgiTest(t *testing.T, h *Handler, + httpreq string, + expectedMap map[string]string, checks ...func(reqInfo map[string]string)) *httptest.ResponseRecorder { rw := httptest.NewRecorder() req := newRequest(httpreq) h.ServeHTTP(rw, req) - runResponseChecks(t, rw, expectedMap) + runResponseChecks(t, rw, expectedMap, checks...) return rw } -func runResponseChecks(t *testing.T, rw *httptest.ResponseRecorder, expectedMap map[string]string) { +func runResponseChecks(t *testing.T, rw *httptest.ResponseRecorder, + expectedMap map[string]string, checks ...func(reqInfo map[string]string)) { // Make a map to hold the test map that the CGI returns. m := make(map[string]string) m["_body"] = rw.Body.String() @@ -81,6 +84,9 @@ readlines: t.Errorf("for key %q got %q; expected %q", key, got, expected) } } + for _, check := range checks { + check(m) + } } var cgiTested, cgiWorks bool @@ -236,6 +242,31 @@ func TestDupHeaders(t *testing.T) { expectedMap) } +// Issue 16405: CGI+http.Transport differing uses of HTTP_PROXY. +// Verify we don't set the HTTP_PROXY environment variable. +// Hope nobody was depending on it. It's not a known header, though. +func TestDropProxyHeader(t *testing.T) { + check(t) + h := &Handler{ + Path: "testdata/test.cgi", + } + expectedMap := map[string]string{ + "env-REQUEST_URI": "/myscript/bar?a=b", + "env-SCRIPT_FILENAME": "testdata/test.cgi", + "env-HTTP_X_FOO": "a", + } + runCgiTest(t, h, "GET /myscript/bar?a=b HTTP/1.0\n"+ + "X-Foo: a\n"+ + "Proxy: should_be_stripped\n"+ + "Host: example.com\n\n", + expectedMap, + func(reqInfo map[string]string) { + if v, ok := reqInfo["env-HTTP_PROXY"]; ok { + t.Errorf("HTTP_PROXY = %q; should be absent", v) + } + }) +} + func TestPathInfoNoRoot(t *testing.T) { check(t) h := &Handler{ diff --git a/src/net/http/transport.go b/src/net/http/transport.go index f7904b4a89..eb54703c8f 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -251,6 +251,9 @@ func ProxyFromEnvironment(req *Request) (*url.URL, error) { } if proxy == "" { proxy = httpProxyEnv.Get() + if proxy != "" && os.Getenv("REQUEST_METHOD") != "" { + return nil, errors.New("net/http: refusing to use HTTP_PROXY value in CGI environment; see golang.org/s/cgihttpproxy") + } } if proxy == "" { return nil, nil diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index d653a5a7fc..72b98f16d7 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -2060,7 +2060,8 @@ type proxyFromEnvTest struct { env string // HTTP_PROXY httpsenv string // HTTPS_PROXY - noenv string // NO_RPXY + noenv string // NO_PROXY + reqmeth string // REQUEST_METHOD want string wanterr error @@ -2084,6 +2085,10 @@ func (t proxyFromEnvTest) String() string { space() fmt.Fprintf(&buf, "no_proxy=%q", t.noenv) } + if t.reqmeth != "" { + space() + fmt.Fprintf(&buf, "request_method=%q", t.reqmeth) + } req := "http://example.com" if t.req != "" { req = t.req @@ -2107,6 +2112,12 @@ var proxyFromEnvTests = []proxyFromEnvTest{ {req: "https://secure.tld/", env: "http.proxy.tld", httpsenv: "secure.proxy.tld", want: "http://secure.proxy.tld"}, {req: "https://secure.tld/", env: "http.proxy.tld", httpsenv: "https://secure.proxy.tld", want: "https://secure.proxy.tld"}, + // Issue 16405: don't use HTTP_PROXY in a CGI environment, + // where HTTP_PROXY can be attacker-controlled. + {env: "http://10.1.2.3:8080", reqmeth: "POST", + want: "", + wanterr: errors.New("net/http: refusing to use HTTP_PROXY value in CGI environment; see golang.org/s/cgihttpproxy")}, + {want: ""}, {noenv: "example.com", req: "http://example.com/", env: "proxy", want: ""}, @@ -2122,6 +2133,7 @@ func TestProxyFromEnvironment(t *testing.T) { os.Setenv("HTTP_PROXY", tt.env) os.Setenv("HTTPS_PROXY", tt.httpsenv) os.Setenv("NO_PROXY", tt.noenv) + os.Setenv("REQUEST_METHOD", tt.reqmeth) ResetCachedEnvironment() reqURL := tt.req if reqURL == "" { -- 2.48.1