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 <cbro@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>

diff --git a/src/net/http/cgi/host.go b/src/net/http/cgi/host.go
index 2eea64334b63c14328d2db658dd21aae4fe76dcc..58e9f7132a86cbffdf569b196a0ae38049f9e045 100644 (file)
--- 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 70c5aff5e29932912461edf0101552c481d8577f..11213349a71b1be9a672e8be48a55812290258e9 100644 (file)
--- 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 0c81b55e12b6728840b295c4a5336b62668cd389..9164d0d827c0017af81673c450f009622681e3e3 100644 (file)
--- 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 d653a5a7fc1e4bbf4b98b7cace9ac998e72c69de..72b98f16d7eaa0ca9b726034c807e5fd0daa598a 100644 (file)
--- 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:    "<nil>",
+               wanterr: errors.New("net/http: refusing to use HTTP_PROXY value in CGI environment; see golang.org/s/cgihttpproxy")},
+
        {want: "<nil>"},
 
        {noenv: "example.com", req: "http://example.com/", env: "proxy", want: "<nil>"},
@@ -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 == "" {