cmd/go: fix command injection in VCS path

Fixes #23867, CVE-2018-7187

Change-Id: I5d0ba4923c9ed354ef76290e149c182447f9dfe2
Reviewed-on: https://go-review.googlesource.com/94656
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

src/cmd/go/internal/get/vcs.go

@@ -809,7 +809,7 @@ func repoRootForImportDynamic(importPath string, security web.SecurityMode) (*re
 		}
 	}
 
-	if err := validateRepoRootScheme(mmi.RepoRoot); err != nil {
+	if err := validateRepoRoot(mmi.RepoRoot); err != nil {
 		return nil, fmt.Errorf("%s: invalid repo root %q: %v", urlStr, mmi.RepoRoot, err)
 	}
 	rr := &repoRoot{
@@ -824,33 +824,16 @@ func repoRootForImportDynamic(importPath string, security web.SecurityMode) (*re
 	return rr, nil
 }
 
-// validateRepoRootScheme returns an error if repoRoot does not seem
-// to have a valid URL scheme. At this point we permit things that
-// aren't valid URLs, although later, if not using -insecure, we will
-// restrict repoRoots to be valid URLs. This is only because we've
-// historically permitted them, and people may depend on that.
-func validateRepoRootScheme(repoRoot string) error {
-	end := strings.Index(repoRoot, "://")
-	if end <= 0 {
-		return errors.New("no scheme")
+// validateRepoRoot returns an error if repoRoot does not seem to be
+// a valid URL with scheme.
+func validateRepoRoot(repoRoot string) error {
+	url, err := url.Parse(repoRoot)
+	if err != nil {
+		return err
 	}
-
-	// RFC 3986 section 3.1.
-	for i := 0; i < end; i++ {
-		c := repoRoot[i]
-		switch {
-		case 'a' <= c && c <= 'z' || 'A' <= c && c <= 'Z':
-			// OK.
-		case '0' <= c && c <= '9' || c == '+' || c == '-' || c == '.':
-			// OK except at start.
-			if i == 0 {
-				return errors.New("invalid scheme")
-			}
-		default:
-			return errors.New("invalid scheme")
-		}
+	if url.Scheme == "" {
+		return errors.New("no scheme")
 	}
-
 	return nil
 }
 

src/cmd/go/internal/get/vcs_test.go

@@ -417,45 +417,46 @@ func TestMatchGoImport(t *testing.T) {
 	}
 }
 
-func TestValidateRepoRootScheme(t *testing.T) {
+func TestValidateRepoRoot(t *testing.T) {
 	tests := []struct {
 		root string
-		err  string
+		ok   bool
 	}{
 		{
 			root: "",
-			err:  "no scheme",
+			ok:   false,
 		},
 		{
 			root: "http://",
-			err:  "",
+			ok:   true,
 		},
 		{
-			root: "a://",
-			err:  "",
+			root: "git+ssh://",
+			ok:   true,
 		},
 		{
-			root: "a#://",
-			err:  "invalid scheme",
+			root: "http#://",
+			ok:   false,
+		},
+		{
+			root: "-config",
+			ok:   false,
 		},
 		{
 			root: "-config://",
-			err:  "invalid scheme",
+			ok:   false,
 		},
 	}
 
 	for _, test := range tests {
-		err := validateRepoRootScheme(test.root)
-		if err == nil {
-			if test.err != "" {
-				t.Errorf("validateRepoRootScheme(%q) = nil, want %q", test.root, test.err)
-			}
-		} else if test.err == "" {
-			if err != nil {
-				t.Errorf("validateRepoRootScheme(%q) = %q, want nil", test.root, test.err)
+		err := validateRepoRoot(test.root)
+		ok := err == nil
+		if ok != test.ok {
+			want := "error"
+			if test.ok {
+				want = "nil"
 			}
-		} else if err.Error() != test.err {
-			t.Errorf("validateRepoRootScheme(%q) = %q, want %q", test.root, err, test.err)
+			t.Errorf("validateRepoRoot(%q) = %q, want %s", test.root, err, want)
 		}
 	}
 }