From a357d15e9ee36a1232ae071d9968c4cf10a672b4 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 18 Jul 2016 06:05:24 +0000 Subject: [PATCH] [release-branch.go1.6] 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/25012 --- 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 9b4d875..3f1600b 100644 --- a/src/net/http/cgi/host.go +++ b/src/net/http/cgi/host.go @@ -145,6 +145,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 3327764..2783fe1 100644 --- a/src/net/http/cgi/host_test.go +++ b/src/net/http/cgi/host_test.go @@ -34,15 +34,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() @@ -80,6 +83,9 @@ readlines: t.Errorf("for key %q got %q; expected %q", key, got, expected) } } + for _, check := range checks { + check(m) + } } var cgiTested, cgiWorks bool @@ -235,6 +241,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 1e3ea11..794b786 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -216,6 +216,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 d9da078..381432e 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -1985,7 +1985,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 @@ -2009,6 +2010,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 @@ -2032,6 +2037,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: ""}, @@ -2047,6 +2058,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 == "" {