From df523969435b8945d939c7e2a849b50910ef4c25 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Tue, 4 Dec 2018 22:23:22 -0500 Subject: [PATCH 1/8] [release-branch.go1.11-security] crypto/x509: limit number of signature checks for each verification That number grows quadratically with the number of intermediate certificates in certain pathological cases (for example if they all have the same Subject) leading to a CPU DoS. Set a fixed budget that should fit all real world chains, given we only look at intermediates provided by the peer. The algorithm can be improved, but that's left for follow-up CLs: * the cache logic should be reviewed for correctness, as it seems to override the entire chain with the cached one * the equality check should compare Subject and public key, not the whole certificate * certificates with the right SKID but the wrong Subject should not be considered, and in particular should not take priority over certificates with the right Subject Change-Id: Ib257c12cd5563df7723f9c81231d82b882854213 Reviewed-on: https://team-review.git.corp.google.com/c/370475 Reviewed-by: Andrew Bonventre (cherry picked from commit 09d57361bc99cbbfb9755ee30ddcb42ff5a9d7d6) Reviewed-on: https://team-review.git.corp.google.com/c/372858 Reviewed-by: Dmitri Shuralyov --- src/crypto/x509/cert_pool.go | 28 ++------ src/crypto/x509/verify.go | 94 +++++++++++++++----------- src/crypto/x509/verify_test.go | 119 +++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 61 deletions(-) diff --git a/src/crypto/x509/cert_pool.go b/src/crypto/x509/cert_pool.go index a1646b9826..61107f86b8 100644 --- a/src/crypto/x509/cert_pool.go +++ b/src/crypto/x509/cert_pool.go @@ -65,32 +65,16 @@ func SystemCertPool() (*CertPool, error) { return loadSystemRoots() } -// findVerifiedParents attempts to find certificates in s which have signed the -// given certificate. If any candidates were rejected then errCert will be set -// to one of them, arbitrarily, and err will contain the reason that it was -// rejected. -func (s *CertPool) findVerifiedParents(cert *Certificate) (parents []int, errCert *Certificate, err error) { +// findPotentialParents returns the indexes of certificates in s which might +// have signed cert. The caller must not modify the returned slice. +func (s *CertPool) findPotentialParents(cert *Certificate) []int { if s == nil { - return + return nil } - var candidates []int - if len(cert.AuthorityKeyId) > 0 { - candidates = s.bySubjectKeyId[string(cert.AuthorityKeyId)] + return s.bySubjectKeyId[string(cert.AuthorityKeyId)] } - if len(candidates) == 0 { - candidates = s.byName[string(cert.RawIssuer)] - } - - for _, c := range candidates { - if err = cert.CheckSignatureFrom(s.certs[c]); err == nil { - parents = append(parents, c) - } else { - errCert = s.certs[c] - } - } - - return + return s.byName[string(cert.RawIssuer)] } func (s *CertPool) contains(cert *Certificate) bool { diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index 0b75778a03..d84272ecb4 100644 --- a/src/crypto/x509/verify.go +++ b/src/crypto/x509/verify.go @@ -765,7 +765,7 @@ func (c *Certificate) Verify(opts VerifyOptions) (chains [][]*Certificate, err e if opts.Roots.contains(c) { candidateChains = append(candidateChains, []*Certificate{c}) } else { - if candidateChains, err = c.buildChains(make(map[int][][]*Certificate), []*Certificate{c}, &opts); err != nil { + if candidateChains, err = c.buildChains(nil, []*Certificate{c}, nil, &opts); err != nil { return nil, err } } @@ -802,58 +802,74 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate return n } -func (c *Certificate) buildChains(cache map[int][][]*Certificate, currentChain []*Certificate, opts *VerifyOptions) (chains [][]*Certificate, err error) { - possibleRoots, failedRoot, rootErr := opts.Roots.findVerifiedParents(c) -nextRoot: - for _, rootNum := range possibleRoots { - root := opts.Roots.certs[rootNum] +// maxChainSignatureChecks is the maximum number of CheckSignatureFrom calls +// that an invocation of buildChains will (tranistively) make. Most chains are +// less than 15 certificates long, so this leaves space for multiple chains and +// for failed checks due to different intermediates having the same Subject. +const maxChainSignatureChecks = 100 +func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, currentChain []*Certificate, sigChecks *int, opts *VerifyOptions) (chains [][]*Certificate, err error) { + var ( + hintErr error + hintCert *Certificate + ) + + considerCandidate := func(certType int, candidate *Certificate) { for _, cert := range currentChain { - if cert.Equal(root) { - continue nextRoot + if cert.Equal(candidate) { + return } } - err = root.isValid(rootCertificate, currentChain, opts) - if err != nil { - continue + if sigChecks == nil { + sigChecks = new(int) + } + *sigChecks++ + if *sigChecks > maxChainSignatureChecks { + err = errors.New("x509: signature check attempts limit reached while verifying certificate chain") + return + } + + if err := c.CheckSignatureFrom(candidate); err != nil { + if hintErr == nil { + hintErr = err + hintCert = candidate + } + return + } + + err = candidate.isValid(certType, currentChain, opts) + if err != nil { + return + } + + switch certType { + case rootCertificate: + chains = append(chains, appendToFreshChain(currentChain, candidate)) + case intermediateCertificate: + if cache == nil { + cache = make(map[*Certificate][][]*Certificate) + } + childChains, ok := cache[candidate] + if !ok { + childChains, err = candidate.buildChains(cache, appendToFreshChain(currentChain, candidate), sigChecks, opts) + cache[candidate] = childChains + } + chains = append(chains, childChains...) } - chains = append(chains, appendToFreshChain(currentChain, root)) } - possibleIntermediates, failedIntermediate, intermediateErr := opts.Intermediates.findVerifiedParents(c) -nextIntermediate: - for _, intermediateNum := range possibleIntermediates { - intermediate := opts.Intermediates.certs[intermediateNum] - for _, cert := range currentChain { - if cert.Equal(intermediate) { - continue nextIntermediate - } - } - err = intermediate.isValid(intermediateCertificate, currentChain, opts) - if err != nil { - continue - } - var childChains [][]*Certificate - childChains, ok := cache[intermediateNum] - if !ok { - childChains, err = intermediate.buildChains(cache, appendToFreshChain(currentChain, intermediate), opts) - cache[intermediateNum] = childChains - } - chains = append(chains, childChains...) + for _, rootNum := range opts.Roots.findPotentialParents(c) { + considerCandidate(rootCertificate, opts.Roots.certs[rootNum]) + } + for _, intermediateNum := range opts.Intermediates.findPotentialParents(c) { + considerCandidate(intermediateCertificate, opts.Intermediates.certs[intermediateNum]) } if len(chains) > 0 { err = nil } - if len(chains) == 0 && err == nil { - hintErr := rootErr - hintCert := failedRoot - if hintErr == nil { - hintErr = intermediateErr - hintCert = failedIntermediate - } err = UnknownAuthorityError{c, hintErr, hintCert} } diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go index 0e24d3b5da..85f4703d4c 100644 --- a/src/crypto/x509/verify_test.go +++ b/src/crypto/x509/verify_test.go @@ -5,10 +5,15 @@ package x509 import ( + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "crypto/x509/pkix" "encoding/pem" "errors" "fmt" + "math/big" "runtime" "strings" "testing" @@ -1889,3 +1894,117 @@ func TestValidHostname(t *testing.T) { } } } + +func generateCert(cn string, isCA bool, issuer *Certificate, issuerKey crypto.PrivateKey) (*Certificate, crypto.PrivateKey, error) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + return nil, nil, err + } + + serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, _ := rand.Int(rand.Reader, serialNumberLimit) + + template := &Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{CommonName: cn}, + NotBefore: time.Now().Add(-1 * time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + + KeyUsage: KeyUsageKeyEncipherment | KeyUsageDigitalSignature | KeyUsageCertSign, + ExtKeyUsage: []ExtKeyUsage{ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: isCA, + } + if issuer == nil { + issuer = template + issuerKey = priv + } + + derBytes, err := CreateCertificate(rand.Reader, template, issuer, priv.Public(), issuerKey) + if err != nil { + return nil, nil, err + } + cert, err := ParseCertificate(derBytes) + if err != nil { + return nil, nil, err + } + + return cert, priv, nil +} + +func TestPathologicalChain(t *testing.T) { + if testing.Short() { + t.Skip("skipping generation of a long chain of certificates in short mode") + } + + // Build a chain where all intermediates share the same subject, to hit the + // path building worst behavior. + roots, intermediates := NewCertPool(), NewCertPool() + + parent, parentKey, err := generateCert("Root CA", true, nil, nil) + if err != nil { + t.Fatal(err) + } + roots.AddCert(parent) + + for i := 1; i < 100; i++ { + parent, parentKey, err = generateCert("Intermediate CA", true, parent, parentKey) + if err != nil { + t.Fatal(err) + } + intermediates.AddCert(parent) + } + + leaf, _, err := generateCert("Leaf", false, parent, parentKey) + if err != nil { + t.Fatal(err) + } + + start := time.Now() + _, err = leaf.Verify(VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }) + t.Logf("verification took %v", time.Since(start)) + + if err == nil || !strings.Contains(err.Error(), "signature check attempts limit") { + t.Errorf("expected verification to fail with a signature checks limit error; got %v", err) + } +} + +func TestLongChain(t *testing.T) { + if testing.Short() { + t.Skip("skipping generation of a long chain of certificates in short mode") + } + + roots, intermediates := NewCertPool(), NewCertPool() + + parent, parentKey, err := generateCert("Root CA", true, nil, nil) + if err != nil { + t.Fatal(err) + } + roots.AddCert(parent) + + for i := 1; i < 15; i++ { + name := fmt.Sprintf("Intermediate CA #%d", i) + parent, parentKey, err = generateCert(name, true, parent, parentKey) + if err != nil { + t.Fatal(err) + } + intermediates.AddCert(parent) + } + + leaf, _, err := generateCert("Leaf", false, parent, parentKey) + if err != nil { + t.Fatal(err) + } + + start := time.Now() + if _, err := leaf.Verify(VerifyOptions{ + Roots: roots, + Intermediates: intermediates, + }); err != nil { + t.Error(err) + } + t.Logf("verification took %v", time.Since(start)) +} From 8954addb3294a5e664a9833354bafa58f163fe8f Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 30 Nov 2018 14:04:35 -0500 Subject: [PATCH 2/8] [release-branch.go1.11-security] cmd/go: reject 'get' of paths containing leading dots or unsupported characters On some platforms, directories beginning with dot are treated as hidden files, and filenames containing unusual characters can be confusing for users to manipulate (and delete). Change-Id: Ia1f5a65b9cff4eeb51cc4dba3ff7c7afabc343f2 Reviewed-on: https://team-review.git.corp.google.com/c/368442 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/internal/get/get.go | 4 + src/cmd/go/internal/get/path.go | 172 ++++++++++++++++++++ src/cmd/go/testdata/script/get_brace.txt | 45 +++++ src/cmd/go/testdata/script/get_dotfiles.txt | 57 +++++++ 4 files changed, 278 insertions(+) create mode 100644 src/cmd/go/internal/get/path.go create mode 100644 src/cmd/go/testdata/script/get_brace.txt create mode 100644 src/cmd/go/testdata/script/get_dotfiles.txt diff --git a/src/cmd/go/internal/get/get.go b/src/cmd/go/internal/get/get.go index e4148bceb0..f4b969fcb2 100644 --- a/src/cmd/go/internal/get/get.go +++ b/src/cmd/go/internal/get/get.go @@ -402,6 +402,10 @@ func downloadPackage(p *load.Package) error { security = web.Insecure } + if err := CheckImportPath(p.ImportPath); err != nil { + return fmt.Errorf("%s: invalid import path: %v", p.ImportPath, err) + } + if p.Internal.Build.SrcRoot != "" { // Directory exists. Look for checkout along path to src. vcs, rootPath, err = vcsFromDir(p.Dir, p.Internal.Build.SrcRoot) diff --git a/src/cmd/go/internal/get/path.go b/src/cmd/go/internal/get/path.go new file mode 100644 index 0000000000..2920fc2085 --- /dev/null +++ b/src/cmd/go/internal/get/path.go @@ -0,0 +1,172 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package get + +import ( + "fmt" + "strings" + "unicode" + "unicode/utf8" +) + +// The following functions are copied verbatim from cmd/go/internal/module/module.go. +// +// TODO(bcmills): After the call site for this function is backported, +// consolidate this back down to a single copy. + +// CheckImportPath checks that an import path is valid. +func CheckImportPath(path string) error { + if err := checkPath(path, false); err != nil { + return fmt.Errorf("malformed import path %q: %v", path, err) + } + return nil +} + +// checkPath checks that a general path is valid. +// It returns an error describing why but not mentioning path. +// Because these checks apply to both module paths and import paths, +// the caller is expected to add the "malformed ___ path %q: " prefix. +// fileName indicates whether the final element of the path is a file name +// (as opposed to a directory name). +func checkPath(path string, fileName bool) error { + if !utf8.ValidString(path) { + return fmt.Errorf("invalid UTF-8") + } + if path == "" { + return fmt.Errorf("empty string") + } + if strings.Contains(path, "..") { + return fmt.Errorf("double dot") + } + if strings.Contains(path, "//") { + return fmt.Errorf("double slash") + } + if path[len(path)-1] == '/' { + return fmt.Errorf("trailing slash") + } + elemStart := 0 + for i, r := range path { + if r == '/' { + if err := checkElem(path[elemStart:i], fileName); err != nil { + return err + } + elemStart = i + 1 + } + } + if err := checkElem(path[elemStart:], fileName); err != nil { + return err + } + return nil +} + +// checkElem checks whether an individual path element is valid. +// fileName indicates whether the element is a file name (not a directory name). +func checkElem(elem string, fileName bool) error { + if elem == "" { + return fmt.Errorf("empty path element") + } + if strings.Count(elem, ".") == len(elem) { + return fmt.Errorf("invalid path element %q", elem) + } + if elem[0] == '.' && !fileName { + return fmt.Errorf("leading dot in path element") + } + if elem[len(elem)-1] == '.' { + return fmt.Errorf("trailing dot in path element") + } + charOK := pathOK + if fileName { + charOK = fileNameOK + } + for _, r := range elem { + if !charOK(r) { + return fmt.Errorf("invalid char %q", r) + } + } + + // Windows disallows a bunch of path elements, sadly. + // See https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file + short := elem + if i := strings.Index(short, "."); i >= 0 { + short = short[:i] + } + for _, bad := range badWindowsNames { + if strings.EqualFold(bad, short) { + return fmt.Errorf("disallowed path element %q", elem) + } + } + return nil +} + +// pathOK reports whether r can appear in an import path element. +// Paths can be ASCII letters, ASCII digits, and limited ASCII punctuation: + - . _ and ~. +// This matches what "go get" has historically recognized in import paths. +// TODO(rsc): We would like to allow Unicode letters, but that requires additional +// care in the safe encoding (see note below). +func pathOK(r rune) bool { + if r < utf8.RuneSelf { + return r == '+' || r == '-' || r == '.' || r == '_' || r == '~' || + '0' <= r && r <= '9' || + 'A' <= r && r <= 'Z' || + 'a' <= r && r <= 'z' + } + return false +} + +// fileNameOK reports whether r can appear in a file name. +// For now we allow all Unicode letters but otherwise limit to pathOK plus a few more punctuation characters. +// If we expand the set of allowed characters here, we have to +// work harder at detecting potential case-folding and normalization collisions. +// See note about "safe encoding" below. +func fileNameOK(r rune) bool { + if r < utf8.RuneSelf { + // Entire set of ASCII punctuation, from which we remove characters: + // ! " # $ % & ' ( ) * + , - . / : ; < = > ? @ [ \ ] ^ _ ` { | } ~ + // We disallow some shell special characters: " ' * < > ? ` | + // (Note that some of those are disallowed by the Windows file system as well.) + // We also disallow path separators / : and \ (fileNameOK is only called on path element characters). + // We allow spaces (U+0020) in file names. + const allowed = "!#$%&()+,-.=@[]^_{}~ " + if '0' <= r && r <= '9' || 'A' <= r && r <= 'Z' || 'a' <= r && r <= 'z' { + return true + } + for i := 0; i < len(allowed); i++ { + if rune(allowed[i]) == r { + return true + } + } + return false + } + // It may be OK to add more ASCII punctuation here, but only carefully. + // For example Windows disallows < > \, and macOS disallows :, so we must not allow those. + return unicode.IsLetter(r) +} + +// badWindowsNames are the reserved file path elements on Windows. +// See https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file +var badWindowsNames = []string{ + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", +} diff --git a/src/cmd/go/testdata/script/get_brace.txt b/src/cmd/go/testdata/script/get_brace.txt new file mode 100644 index 0000000000..36414d7b55 --- /dev/null +++ b/src/cmd/go/testdata/script/get_brace.txt @@ -0,0 +1,45 @@ +[!exec:git] skip + +# Set up some empty repositories. +cd $WORK/_origin/foo +exec git init +exec git commit --allow-empty -m 'create master branch' + +cd $WORK +cd '_origin/{confusing}' +exec git init +exec git commit --allow-empty -m 'create master branch' + +# Clone the empty repositories into GOPATH. +# This tells the Go command where to find them: it takes the place of a user's meta-tag redirector. +mkdir $GOPATH/src/example.com +cd $GOPATH/src/example.com +exec git clone $WORK/_origin/foo +exec git clone $WORK/_origin/{confusing} + +# Commit contents to the repositories. +cd $WORK/_origin/foo +exec git add main.go +exec git commit -m 'add main' + +cd $WORK +cd '_origin/{confusing}' +exec git add confusing.go +exec git commit -m 'just try to delete this!' + +# 'go get' should refuse to download or update the confusingly-named repo. +cd $GOPATH/src/example.com/foo +! go get -u 'example.com/{confusing}' +stderr 'invalid char' +! go get -u example.com/foo +stderr 'invalid import path' +! exists example.com/{confusing} + +-- $WORK/_origin/foo/main.go -- +package main +import _ "example.com/{confusing}" + +func main() {} + +-- $WORK/_origin/{confusing}/confusing.go -- +package confusing diff --git a/src/cmd/go/testdata/script/get_dotfiles.txt b/src/cmd/go/testdata/script/get_dotfiles.txt new file mode 100644 index 0000000000..c09da8beeb --- /dev/null +++ b/src/cmd/go/testdata/script/get_dotfiles.txt @@ -0,0 +1,57 @@ +[!exec:git] skip + +# Set up a benign repository and a repository with a dotfile name. +cd $WORK/_origin/foo +exec git init +exec git commit --allow-empty -m 'create master branch' + +cd $WORK/_origin/.hidden +exec git init +exec git commit --allow-empty -m 'create master branch' + +# Clone the empty repositories into GOPATH. +# This tells the Go command where to find them: it takes the place of a user's meta-tag redirector. +mkdir $GOPATH/src/example.com +cd $GOPATH/src/example.com +exec git clone $WORK/_origin/foo +exec git clone $WORK/_origin/.hidden + +# Add a benign commit. +cd $WORK/_origin/foo +cp _ok/main.go main.go +exec git add main.go +exec git commit -m 'add ok' + +# 'go get' should install the benign commit. +cd $GOPATH +go get -u example.com/foo + +# Now sneak in an import of a dotfile path. +cd $WORK/_origin/.hidden +exec git add hidden.go +exec git commit -m 'nothing to see here, move along' + +cd $WORK/_origin/foo +cp _sneaky/main.go main.go +exec git add main.go +exec git commit -m 'fix typo (heh heh heh)' + +# 'go get -u' should refuse to download or update the dotfile-named repo. +cd $GOPATH/src/example.com/foo +! go get -u example.com/foo +stderr 'leading dot' +! exists example.com/.hidden/hidden.go + +-- $WORK/_origin/foo/_ok/main.go -- +package main + +func main() {} + +-- $WORK/_origin/foo/_sneaky/main.go -- +package main +import _ "example.com/.hidden" + +func main() {} + +-- $WORK/_origin/.hidden/hidden.go -- +package hidden From 5aedc8af94c0a8ffc58cbd09993192dea9b238db Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 4 Dec 2018 14:37:39 -0500 Subject: [PATCH 3/8] [release-branch.go1.11-security] cmd/go/internal/get: reject Windows shortnames as path components Change-Id: Ia32d8ec1fc0c4e242f50d8871c0ef3ce315f3c65 Reviewed-on: https://team-review.git.corp.google.com/c/370572 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/internal/get/path.go | 21 ++++++++++++++++++++- src/cmd/go/testdata/script/get_tilde.txt | 21 +++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 src/cmd/go/testdata/script/get_tilde.txt diff --git a/src/cmd/go/internal/get/path.go b/src/cmd/go/internal/get/path.go index 2920fc2085..c8072b25fd 100644 --- a/src/cmd/go/internal/get/path.go +++ b/src/cmd/go/internal/get/path.go @@ -11,7 +11,8 @@ import ( "unicode/utf8" ) -// The following functions are copied verbatim from cmd/go/internal/module/module.go. +// The following functions are copied verbatim from cmd/go/internal/module/module.go, +// with one change to additionally reject Windows short-names. // // TODO(bcmills): After the call site for this function is backported, // consolidate this back down to a single copy. @@ -76,6 +77,7 @@ func checkElem(elem string, fileName bool) error { if elem[len(elem)-1] == '.' { return fmt.Errorf("trailing dot in path element") } + charOK := pathOK if fileName { charOK = fileNameOK @@ -97,6 +99,23 @@ func checkElem(elem string, fileName bool) error { return fmt.Errorf("disallowed path element %q", elem) } } + + // Reject path components that look like Windows short-names. + // Those usually end in a tilde followed by one or more ASCII digits. + if tilde := strings.LastIndexByte(short, '~'); tilde >= 0 && tilde < len(short)-1 { + suffix := short[tilde+1:] + suffixIsDigits := true + for _, r := range suffix { + if r < '0' || r > '9' { + suffixIsDigits = false + break + } + } + if suffixIsDigits { + return fmt.Errorf("trailing tilde and digits in path element") + } + } + return nil } diff --git a/src/cmd/go/testdata/script/get_tilde.txt b/src/cmd/go/testdata/script/get_tilde.txt new file mode 100644 index 0000000000..08289ca405 --- /dev/null +++ b/src/cmd/go/testdata/script/get_tilde.txt @@ -0,0 +1,21 @@ +# Paths containing windows short names should be rejected before attempting to fetch. +! go get example.com/longna~1.dir/thing +stderr 'trailing tilde and digits' +! go get example.com/longna~1/thing +stderr 'trailing tilde and digits' +! go get example.com/~9999999/thing +stderr 'trailing tilde and digits' + +# A path containing an element that is just a tilde, or a tilde followed by non-digits, +# should attempt to resolve. +! go get example.com/~glenda/notfound +! stderr 'trailing tilde and digits' +stderr 'unrecognized import path' + +! go get example.com/~glenda2/notfound +! stderr 'trailing tilde and digits' +stderr 'unrecognized import path' + +! go get example.com/~/notfound +! stderr 'trailing tilde and digits' +stderr 'unrecognized import path' From d01ccd8ee871d6eaaaed52851c6ce8b49993f33a Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 4 Dec 2018 15:42:32 -0500 Subject: [PATCH 4/8] [release-branch.go1.11-security] cmd/go/internal/get: use a strings.Replacer in expand This should be a no-op, but produces deterministic (and more correct) behavior if we have accidentally failed to sanitize one of the inputs. Change-Id: I1271d0ffd01a691ec8c84906c4e02d9e2be19c72 Reviewed-on: https://team-review.git.corp.google.com/c/372705 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/internal/get/vcs.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cmd/go/internal/get/vcs.go b/src/cmd/go/internal/get/vcs.go index 5cd164f2ff..ff4f9d12ef 100644 --- a/src/cmd/go/internal/get/vcs.go +++ b/src/cmd/go/internal/get/vcs.go @@ -966,10 +966,14 @@ func matchGoImport(imports []metaImport, importPath string) (metaImport, error) // expand rewrites s to replace {k} with match[k] for each key k in match. func expand(match map[string]string, s string) string { + // We want to replace each match exactly once, and the result of expansion + // must not depend on the iteration order through the map. + // A strings.Replacer has exactly the properties we're looking for. + oldNew := make([]string, 0, 2*len(match)) for k, v := range match { - s = strings.Replace(s, "{"+k+"}", v, -1) + oldNew = append(oldNew, "{"+k+"}", v) } - return s + return strings.NewReplacer(oldNew...).Replace(s) } // vcsPaths defines the meaning of import paths referring to From d39cd4d6d73ffc8f47c8f3cec7dbe57a4f3f4980 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 4 Dec 2018 17:00:19 -0500 Subject: [PATCH 5/8] [release-branch.go1.11-security] cmd/go/internal/get: relax pathOK check to allow any letter This fixes a regression of #18660 with the new path checks. Change-Id: I2dd9adab999e7f810e0e746ad8b75ea9622f56e7 Reviewed-on: https://team-review.git.corp.google.com/c/372706 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/internal/get/path.go | 13 +++++----- src/cmd/go/testdata/script/get_unicode.txt | 28 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 src/cmd/go/testdata/script/get_unicode.txt diff --git a/src/cmd/go/internal/get/path.go b/src/cmd/go/internal/get/path.go index c8072b25fd..d443bd28a9 100644 --- a/src/cmd/go/internal/get/path.go +++ b/src/cmd/go/internal/get/path.go @@ -12,10 +12,13 @@ import ( ) // The following functions are copied verbatim from cmd/go/internal/module/module.go, -// with one change to additionally reject Windows short-names. +// with a change to additionally reject Windows short-names, +// and one to accept arbitrary letters (golang.org/issue/29101). // // TODO(bcmills): After the call site for this function is backported, // consolidate this back down to a single copy. +// +// NOTE: DO NOT MERGE THESE UNTIL WE DECIDE ABOUT ARBITRARY LETTERS IN MODULE MODE. // CheckImportPath checks that an import path is valid. func CheckImportPath(path string) error { @@ -120,10 +123,8 @@ func checkElem(elem string, fileName bool) error { } // pathOK reports whether r can appear in an import path element. -// Paths can be ASCII letters, ASCII digits, and limited ASCII punctuation: + - . _ and ~. -// This matches what "go get" has historically recognized in import paths. -// TODO(rsc): We would like to allow Unicode letters, but that requires additional -// care in the safe encoding (see note below). +// +// NOTE: This function DIVERGES from module mode pathOK by accepting Unicode letters. func pathOK(r rune) bool { if r < utf8.RuneSelf { return r == '+' || r == '-' || r == '.' || r == '_' || r == '~' || @@ -131,7 +132,7 @@ func pathOK(r rune) bool { 'A' <= r && r <= 'Z' || 'a' <= r && r <= 'z' } - return false + return unicode.IsLetter(r) } // fileNameOK reports whether r can appear in a file name. diff --git a/src/cmd/go/testdata/script/get_unicode.txt b/src/cmd/go/testdata/script/get_unicode.txt new file mode 100644 index 0000000000..a30802b999 --- /dev/null +++ b/src/cmd/go/testdata/script/get_unicode.txt @@ -0,0 +1,28 @@ +[!exec:git] skip + +cd $WORK/_origin/example.com/unicode +exec git init +exec git add unicode.go +exec git commit -m 'add unicode.go' + +mkdir $GOPATH/src/example.com/unicode +cd $GOPATH/src/example.com/unicode +exec git clone $WORK/_origin/example.com/unicode . + +cd $WORK/_origin/example.com/испытание +exec git init +exec git add испытание.go +exec git commit -m 'add испытание.go' + +mkdir $GOPATH/src/example.com/испытание +cd $GOPATH/src/example.com/испытание +exec git clone $WORK/_origin/example.com/испытание . + +cd $GOPATH +go get -u example.com/unicode + +-- $WORK/_origin/example.com/unicode/unicode.go -- +package unicode +import _ "example.com/испытание" +-- $WORK/_origin/example.com/испытание/испытание.go -- +package испытание From 914efab0ebb5e6ae471c2f464cf465b995fe2df2 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Wed, 12 Dec 2018 13:35:19 -0500 Subject: [PATCH 6/8] [release-branch.go1.11-security] doc: document Go 1.11.3 and Go 1.10.6 Change-Id: I3fe44887a84586d73be01df78a9cbb002c1fc9c5 Reviewed-on: https://team-review.git.corp.google.com/c/376466 Reviewed-by: Filippo Valsorda --- doc/devel/release.html | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/doc/devel/release.html b/doc/devel/release.html index b02efed501..4e1f7cbc50 100644 --- a/doc/devel/release.html +++ b/doc/devel/release.html @@ -49,6 +49,13 @@ See the Go 1.11.2 milestone on our issue tracker for details.

+

+go1.11.3 (released 2018/12/12) includes three security fixes to "go get" and +the crypto/x509 package. +See the Go +1.11.3 milestone on our issue tracker for details. +

+

go1.10 (released 2018/02/16)

@@ -98,6 +105,14 @@ See the Go 1.10.5 milestone on our issue tracker for details.

+

+go1.10.6 (released 2018/12/12) includes three security fixes to "go get" and +the crypto/x509 package. +It contains the same fixes as Go 1.11.3 and was released at the same time. +See the Go +1.10.6 milestone on our issue tracker for details. +

+

go1.9 (released 2017/08/24)

From 897b44e439e6407018fa27af82aca62ea7179287 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 13 Dec 2018 09:23:25 -0500 Subject: [PATCH 7/8] [release-branch.go1.11-security] cmd/go: set user and email in test-local git repos Some of the builders cannot infer user and email from the builder hostname. Change-Id: I6f343ae41ca7d984797e595867c8210b404b782f Reviewed-on: https://team-review.git.corp.google.com/c/376740 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/testdata/script/get_brace.txt | 4 ++++ src/cmd/go/testdata/script/get_dotfiles.txt | 4 ++++ src/cmd/go/testdata/script/get_unicode.txt | 9 +++++++++ 3 files changed, 17 insertions(+) diff --git a/src/cmd/go/testdata/script/get_brace.txt b/src/cmd/go/testdata/script/get_brace.txt index 36414d7b55..be81d8f487 100644 --- a/src/cmd/go/testdata/script/get_brace.txt +++ b/src/cmd/go/testdata/script/get_brace.txt @@ -3,11 +3,15 @@ # Set up some empty repositories. cd $WORK/_origin/foo exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git commit --allow-empty -m 'create master branch' cd $WORK cd '_origin/{confusing}' exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git commit --allow-empty -m 'create master branch' # Clone the empty repositories into GOPATH. diff --git a/src/cmd/go/testdata/script/get_dotfiles.txt b/src/cmd/go/testdata/script/get_dotfiles.txt index c09da8beeb..1876114362 100644 --- a/src/cmd/go/testdata/script/get_dotfiles.txt +++ b/src/cmd/go/testdata/script/get_dotfiles.txt @@ -3,10 +3,14 @@ # Set up a benign repository and a repository with a dotfile name. cd $WORK/_origin/foo exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git commit --allow-empty -m 'create master branch' cd $WORK/_origin/.hidden exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git commit --allow-empty -m 'create master branch' # Clone the empty repositories into GOPATH. diff --git a/src/cmd/go/testdata/script/get_unicode.txt b/src/cmd/go/testdata/script/get_unicode.txt index a30802b999..31edcdb9f6 100644 --- a/src/cmd/go/testdata/script/get_unicode.txt +++ b/src/cmd/go/testdata/script/get_unicode.txt @@ -1,23 +1,32 @@ [!exec:git] skip +# Construct a repository that imports a non-ASCII path. cd $WORK/_origin/example.com/unicode exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git add unicode.go exec git commit -m 'add unicode.go' +# Clone the repo into GOPATH so that 'go get -u' can find it. mkdir $GOPATH/src/example.com/unicode cd $GOPATH/src/example.com/unicode exec git clone $WORK/_origin/example.com/unicode . +# Construct the imported repository. cd $WORK/_origin/example.com/испытание exec git init +exec git config user.name 'Nameless Gopher' +exec git config user.email 'nobody@golang.org' exec git add испытание.go exec git commit -m 'add испытание.go' +# Clone that repo into GOPATH too. mkdir $GOPATH/src/example.com/испытание cd $GOPATH/src/example.com/испытание exec git clone $WORK/_origin/example.com/испытание . +# Upgrading the importer should pull from the non-ASCII repo. cd $GOPATH go get -u example.com/unicode From 90c896448691b5edb0ab11110f37234f63cd28ed Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Thu, 13 Dec 2018 12:07:14 -0500 Subject: [PATCH 8/8] [release-branch.go1.11-security] go1.11.3 Change-Id: I0933c8d2f635e987db1a36030ef330f77b5ef8a8 Reviewed-on: https://team-review.git.corp.google.com/c/377323 Reviewed-by: Dmitri Shuralyov --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index ae7717dfe4..aa1dbdc189 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.11.2 \ No newline at end of file +go1.11.3 \ No newline at end of file