cmd/go/internal/web: reject insecure redirects from secure origins

We rely on SSL certificates to verify the identity of origin servers.
If an HTTPS server redirects through a plain-HTTP URL, that hop can be
compromised. We should allow it only if the user set the -insecure
flag explicitly.

Fixes #29591

Change-Id: I00639541cca2ca034c01c464385a43b3aa8ee84f
Reviewed-on: https://go-review.googlesource.com/c/go/+/156838
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: Bryan C. Mills

src/cmd/go/go_test.go

@@ -3407,22 +3407,6 @@ func TestGoGetDotSlashDownload(t *testing.T) {
 	tg.run("get", "./pprof_mac_fix")
 }
 
-// Issue 13037: Was not parsing <meta> tags in 404 served over HTTPS
-func TestGoGetHTTPS404(t *testing.T) {
-	testenv.MustHaveExternalNetwork(t)
-	switch runtime.GOOS {
-	case "darwin", "linux", "freebsd":
-	default:
-		t.Skipf("test case does not work on %s", runtime.GOOS)
-	}
-
-	tg := testgo(t)
-	defer tg.cleanup()
-	tg.tempDir("src")
-	tg.setenv("GOPATH", tg.path("."))
-	tg.run("get", "bazil.org/fuse/fs/fstestutil")
-}
-
 // Test that you cannot import a main package.
 // See golang.org/issue/4210 and golang.org/issue/17475.
 func TestImportMain(t *testing.T) {

src/cmd/go/internal/web/http.go

@@ -25,10 +25,6 @@ import (
 	"cmd/internal/browser"
 )
 
-// httpClient is the default HTTP client, but a variable so it can be
-// changed by tests, without modifying http.DefaultClient.
-var httpClient = http.DefaultClient
-
 // impatientInsecureHTTPClient is used in -insecure mode,
 // when we're connecting to https servers that might not be there
 // or might be using self-signed certificates.
@@ -42,6 +38,18 @@ var impatientInsecureHTTPClient = &http.Client{
 	},
 }
 
+// securityPreservingHTTPClient is like the default HTTP client, but rejects
+// redirects to plain-HTTP URLs if the original URL was secure.
+var securityPreservingHTTPClient = &http.Client{
+	CheckRedirect: func(req *http.Request, via []*http.Request) error {
+		if len(via) > 0 && via[0].URL.Scheme == "https" && req.URL.Scheme != "https" {
+			lastHop := via[len(via)-1].URL
+			return fmt.Errorf("redirected from secure URL %s to insecure URL %s", lastHop, req.URL)
+		}
+		return nil
+	},
+}
+
 type HTTPError struct {
 	status     string
 	StatusCode int
@@ -54,7 +62,7 @@ func (e *HTTPError) Error() string {
 
 // Get returns the data from an HTTP GET request for the given URL.
 func Get(url string) ([]byte, error) {
-	resp, err := httpClient.Get(url)
+	resp, err := securityPreservingHTTPClient.Get(url)
 	if err != nil {
 		return nil, err
 	}
@@ -87,7 +95,7 @@ func GetMaybeInsecure(importPath string, security SecurityMode) (urlStr string,
 		if security == Insecure && scheme == "https" { // fail earlier
 			res, err = impatientInsecureHTTPClient.Get(urlStr)
 		} else {
-			res, err = httpClient.Get(urlStr)
+			res, err = securityPreservingHTTPClient.Get(urlStr)
 		}
 		return
 	}

src/cmd/go/testdata/script/get_insecure_redirect.txt

@@ -0,0 +1,11 @@
+# golang.org/issue/13037: 'go get' was not parsing <meta> tags in 404 served over HTTPS.
+# golang.org/issue/29591: 'go get' was following plain-HTTP redirects even without -insecure.
+
+[!net] skip
+
+env GOPROXY=
+
+! go get -d vcs-test.golang.org/insecure/go/insecure
+stderr 'redirected .* to insecure URL'
+
+go get -d -insecure vcs-test.golang.org/insecure/go/insecure