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>

diff --git a/src/cmd/go/internal/get/vcs.go b/src/cmd/go/internal/get/vcs.go
index dced0ed8db5731081a29291a3b89e103f9486e0c..45fc69a7f38e1415866b77782cb2c7d6d40b28dd 100644 (file)
--- a/src/cmd/go/internal/get/vcs.go
+++ b/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
 }
 

diff --git a/src/cmd/go/internal/get/vcs_test.go b/src/cmd/go/internal/get/vcs_test.go
index ece78b563ceb8fb4388ecb802fa9e6dcec2a80d0..1ce9b739b11ff9b38cbdb63bc92a640d0d3a95f5 100644 (file)
--- a/src/cmd/go/internal/get/vcs_test.go
+++ b/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)
                }
        }
 }