From 7490c2547ec57448be2c74bb91d441083edc583d Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Fri, 8 Jan 2021 03:56:58 +0100 Subject: [release-branch.go1.14-security] crypto/elliptic: fix P-224 field reduction This patch fixes two independent bugs in p224Contract, the function that performs the final complete reduction in the P-224 field. Incorrect outputs due to these bugs were observable from a high-level P224().ScalarMult() call. The first bug was in the calculation of out3GT. That mask was supposed to be all ones if the third limb of the value is greater than the third limb of P (out[3] > 0xffff000). Instead, it was also set if they are equal. That meant that if the third limb was equal, the value was always considered greater than or equal to P, even when the three bottom limbs were all zero. There is exactly one affected value, P - 1, which would trigger the subtraction by P even if it's lower than P already. The second bug was more easily hit, and is the one that caused the known high-level incorrect output: after the conditional subtraction by P, a potential underflow of the lowest limb was not handled. Any values that trigger the subtraction by P (values between P and 2^224-1, and P - 1 due to the bug above) but have a zero lowest limb would produce invalid outputs. Those conditions apply to the intermediate representation before the subtraction, so they are hard to trace to precise inputs. This patch also adds a test suite for the P-224 field arithmetic, including a custom fuzzer that automatically explores potential edge cases by combining limb values that have various meanings in the code. contractMatchesBigInt in TestP224Contract finds the second bug in less than a second without being tailored to it, and could eventually find the first one too by combining 0, (1 << 28) - 1, and the difference of (1 << 28) and (1 << 12). The incorrect P224().ScalarMult() output was found by the elliptic-curve-differential-fuzzer project running on OSS-Fuzz and reported by Philippe Antoine (Catena cyber). Fixes CVE-2021-3114 Change-Id: I50176602d544de3da854270d66a293bcaca57ad7 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/947792 Reviewed-by: Katie Hockman (cherry picked from commit 5fa534e9c7eaeaf875e53b98eac9342b0855b283) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955313 --- src/crypto/elliptic/p224.go | 41 +++--- src/crypto/elliptic/p224_test.go | 277 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 298 insertions(+), 20 deletions(-) diff --git a/src/crypto/elliptic/p224.go b/src/crypto/elliptic/p224.go index 2ea63f3f0c..8c76021464 100644 --- a/src/crypto/elliptic/p224.go +++ b/src/crypto/elliptic/p224.go @@ -386,10 +386,11 @@ func p224Invert(out, in *p224FieldElement) { // p224Contract converts a FieldElement to its unique, minimal form. // // On entry, in[i] < 2**29 -// On exit, in[i] < 2**28 +// On exit, out[i] < 2**28 and out < p func p224Contract(out, in *p224FieldElement) { copy(out[:], in[:]) + // First, carry the bits above 28 to the higher limb. for i := 0; i < 7; i++ { out[i+1] += out[i] >> 28 out[i] &= bottom28Bits @@ -397,10 +398,13 @@ func p224Contract(out, in *p224FieldElement) { top := out[7] >> 28 out[7] &= bottom28Bits + // Use the reduction identity to carry the overflow. + // + // a + top * 2²²⁴ = a + top * 2⁹⁶ - top out[0] -= top out[3] += top << 12 - // We may just have made out[i] negative. So we carry down. If we made + // We may just have made out[0] negative. So we carry down. If we made // out[0] negative then we know that out[3] is sufficiently positive // because we just added to it. for i := 0; i < 3; i++ { @@ -425,13 +429,12 @@ func p224Contract(out, in *p224FieldElement) { // There are two cases to consider for out[3]: // 1) The first time that we eliminated top, we didn't push out[3] over // 2**28. In this case, the partial carry chain didn't change any values - // and top is zero. + // and top is now zero. // 2) We did push out[3] over 2**28 the first time that we eliminated top. - // The first value of top was in [0..16), therefore, prior to eliminating - // the first top, 0xfff1000 <= out[3] <= 0xfffffff. Therefore, after - // overflowing and being reduced by the second carry chain, out[3] <= - // 0xf000. Thus it cannot have overflowed when we eliminated top for the - // second time. + // The first value of top was in [0..2], therefore, after overflowing + // and being reduced by the second carry chain, out[3] <= 2<<12 - 1. + // In both cases, out[3] cannot have overflowed when we eliminated top for + // the second time. // Again, we may just have made out[0] negative, so do the same carry down. // As before, if we made out[0] negative then we know that out[3] is @@ -470,12 +473,11 @@ func p224Contract(out, in *p224FieldElement) { bottom3NonZero |= bottom3NonZero >> 1 bottom3NonZero = uint32(int32(bottom3NonZero<<31) >> 31) - // Everything depends on the value of out[3]. - // If it's > 0xffff000 and top4AllOnes != 0 then the whole value is >= p - // If it's = 0xffff000 and top4AllOnes != 0 and bottom3NonZero != 0, - // then the whole value is >= p + // Assuming top4AllOnes != 0, everything depends on the value of out[3]. + // If it's > 0xffff000 then the whole value is > p + // If it's = 0xffff000 and bottom3NonZero != 0, then the whole value is >= p // If it's < 0xffff000, then the whole value is < p - n := out[3] - 0xffff000 + n := 0xffff000 - out[3] out3Equal := n out3Equal |= out3Equal >> 16 out3Equal |= out3Equal >> 8 @@ -484,8 +486,8 @@ func p224Contract(out, in *p224FieldElement) { out3Equal |= out3Equal >> 1 out3Equal = ^uint32(int32(out3Equal<<31) >> 31) - // If out[3] > 0xffff000 then n's MSB will be zero. - out3GT := ^uint32(int32(n) >> 31) + // If out[3] > 0xffff000 then n's MSB will be one. + out3GT := uint32(int32(n) >> 31) mask := top4AllOnes & ((out3Equal & bottom3NonZero) | out3GT) out[0] -= 1 & mask @@ -494,6 +496,15 @@ func p224Contract(out, in *p224FieldElement) { out[5] -= 0xfffffff & mask out[6] -= 0xfffffff & mask out[7] -= 0xfffffff & mask + + // Do one final carry down, in case we made out[0] negative. One of + // out[0..3] needs to be positive and able to absorb the -1 or the value + // would have been < p, and the subtraction wouldn't have happened. + for i := 0; i < 3; i++ { + mask := uint32(int32(out[i]) >> 31) + out[i] += (1 << 28) & mask + out[i+1] -= 1 & mask + } } // Group element functions. diff --git a/src/crypto/elliptic/p224_test.go b/src/crypto/elliptic/p224_test.go index 8b4fa0483b..eeb24d97ea 100644 --- a/src/crypto/elliptic/p224_test.go +++ b/src/crypto/elliptic/p224_test.go @@ -6,7 +6,11 @@ package elliptic import ( "math/big" + "math/bits" + "math/rand" + "reflect" "testing" + "testing/quick" ) var toFromBigTests = []string{ @@ -21,16 +25,16 @@ func p224AlternativeToBig(in *p224FieldElement) *big.Int { ret := new(big.Int) tmp := new(big.Int) - for i := uint(0); i < 8; i++ { + for i := len(in) - 1; i >= 0; i-- { + ret.Lsh(ret, 28) tmp.SetInt64(int64(in[i])) - tmp.Lsh(tmp, 28*i) ret.Add(ret, tmp) } - ret.Mod(ret, p224.P) + ret.Mod(ret, P224().Params().P) return ret } -func TestToFromBig(t *testing.T) { +func TestP224ToFromBig(t *testing.T) { for i, test := range toFromBigTests { n, _ := new(big.Int).SetString(test, 16) var x p224FieldElement @@ -41,7 +45,270 @@ func TestToFromBig(t *testing.T) { } q := p224AlternativeToBig(&x) if n.Cmp(q) != 0 { - t.Errorf("#%d: %x != %x (alternative)", i, n, m) + t.Errorf("#%d: %x != %x (alternative)", i, n, q) } } } + +// quickCheckConfig32 will make each quickcheck test run (32 * -quickchecks) +// times. The default value of -quickchecks is 100. +var quickCheckConfig32 = &quick.Config{MaxCountScale: 32} + +// weirdLimbs can be combined to generate a range of edge-case field elements. +var weirdLimbs = [...]uint32{ + 0, 1, (1 << 29) - 1, + (1 << 12), (1 << 12) - 1, + (1 << 28), (1 << 28) - 1, +} + +func generateLimb(rand *rand.Rand) uint32 { + const bottom29Bits = 0x1fffffff + n := rand.Intn(len(weirdLimbs) + 3) + switch n { + case len(weirdLimbs): + // Random value. + return uint32(rand.Int31n(1 << 29)) + case len(weirdLimbs) + 1: + // Sum of two values. + k := generateLimb(rand) + generateLimb(rand) + return k & bottom29Bits + case len(weirdLimbs) + 2: + // Difference of two values. + k := generateLimb(rand) - generateLimb(rand) + return k & bottom29Bits + default: + return weirdLimbs[n] + } +} + +func (p224FieldElement) Generate(rand *rand.Rand, size int) reflect.Value { + return reflect.ValueOf(p224FieldElement{ + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + weirdLimbs[rand.Intn(len(weirdLimbs))], + }) +} + +func isInBounds(x *p224FieldElement) bool { + return bits.Len32(x[0]) <= 29 && + bits.Len32(x[1]) <= 29 && + bits.Len32(x[2]) <= 29 && + bits.Len32(x[3]) <= 29 && + bits.Len32(x[4]) <= 29 && + bits.Len32(x[5]) <= 29 && + bits.Len32(x[6]) <= 29 && + bits.Len32(x[7]) <= 29 +} + +func TestP224Mul(t *testing.T) { + mulMatchesBigInt := func(a, b, out p224FieldElement) bool { + var tmp p224LargeFieldElement + p224Mul(&out, &a, &b, &tmp) + + exp := new(big.Int).Mul(p224AlternativeToBig(&a), p224AlternativeToBig(&b)) + exp.Mod(exp, P224().Params().P) + got := p224AlternativeToBig(&out) + if exp.Cmp(got) != 0 || !isInBounds(&out) { + t.Logf("a = %x", a) + t.Logf("b = %x", b) + t.Logf("p224Mul(a, b) = %x = %v", out, got) + t.Logf("a * b = %v", exp) + return false + } + + return true + } + + a := p224FieldElement{0xfffffff, 0xfffffff, 0xf00ffff, 0x20f, 0x0, 0x0, 0x0, 0x0} + b := p224FieldElement{1, 0, 0, 0, 0, 0, 0, 0} + if !mulMatchesBigInt(a, b, p224FieldElement{}) { + t.Fail() + } + + if err := quick.Check(mulMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224Square(t *testing.T) { + squareMatchesBigInt := func(a, out p224FieldElement) bool { + var tmp p224LargeFieldElement + p224Square(&out, &a, &tmp) + + exp := p224AlternativeToBig(&a) + exp.Mul(exp, exp) + exp.Mod(exp, P224().Params().P) + got := p224AlternativeToBig(&out) + if exp.Cmp(got) != 0 || !isInBounds(&out) { + t.Logf("a = %x", a) + t.Logf("p224Square(a, b) = %x = %v", out, got) + t.Logf("a * a = %v", exp) + return false + } + + return true + } + + if err := quick.Check(squareMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224Add(t *testing.T) { + addMatchesBigInt := func(a, b, out p224FieldElement) bool { + p224Add(&out, &a, &b) + + exp := new(big.Int).Add(p224AlternativeToBig(&a), p224AlternativeToBig(&b)) + exp.Mod(exp, P224().Params().P) + got := p224AlternativeToBig(&out) + if exp.Cmp(got) != 0 { + t.Logf("a = %x", a) + t.Logf("b = %x", b) + t.Logf("p224Add(a, b) = %x = %v", out, got) + t.Logf("a + b = %v", exp) + return false + } + + return true + } + + if err := quick.Check(addMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224Reduce(t *testing.T) { + reduceMatchesBigInt := func(a p224FieldElement) bool { + out := a + // TODO: generate higher values for functions like p224Reduce that are + // expected to work with higher input bounds. + p224Reduce(&out) + + exp := p224AlternativeToBig(&a) + got := p224AlternativeToBig(&out) + if exp.Cmp(got) != 0 || !isInBounds(&out) { + t.Logf("a = %x = %v", a, exp) + t.Logf("p224Reduce(a) = %x = %v", out, got) + return false + } + + return true + } + + if err := quick.Check(reduceMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224Contract(t *testing.T) { + contractMatchesBigInt := func(a, out p224FieldElement) bool { + p224Contract(&out, &a) + + exp := p224AlternativeToBig(&a) + got := p224AlternativeToBig(&out) + if exp.Cmp(got) != 0 { + t.Logf("a = %x = %v", a, exp) + t.Logf("p224Contract(a) = %x = %v", out, got) + return false + } + + // Check that out < P. + for i := range p224P { + k := 8 - i - 1 + if out[k] > p224P[k] { + t.Logf("p224Contract(a) = %x", out) + return false + } + if out[k] < p224P[k] { + return true + } + } + t.Logf("p224Contract(a) = %x", out) + return false + } + + if !contractMatchesBigInt(p224P, p224FieldElement{}) { + t.Error("p224Contract(p) is broken") + } + pMinus1 := p224FieldElement{0, 0, 0, 0xffff000, 0xfffffff, 0xfffffff, 0xfffffff, 0xfffffff} + if !contractMatchesBigInt(pMinus1, p224FieldElement{}) { + t.Error("p224Contract(p - 1) is broken") + } + // Check that we can handle input above p, but lowest limb zero. + a := p224FieldElement{0, 1, 0, 0xffff000, 0xfffffff, 0xfffffff, 0xfffffff, 0xfffffff} + if !contractMatchesBigInt(a, p224FieldElement{}) { + t.Error("p224Contract(p + 2²⁸) is broken") + } + // Check that we can handle input above p, but lowest three limbs zero. + b := p224FieldElement{0, 0, 0, 0xffff001, 0xfffffff, 0xfffffff, 0xfffffff, 0xfffffff} + if !contractMatchesBigInt(b, p224FieldElement{}) { + t.Error("p224Contract(p + 2⁸⁴) is broken") + } + + if err := quick.Check(contractMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224IsZero(t *testing.T) { + if got := p224IsZero(&p224FieldElement{}); got != 1 { + t.Errorf("p224IsZero(0) = %d, expected 1", got) + } + if got := p224IsZero((*p224FieldElement)(&p224P)); got != 1 { + t.Errorf("p224IsZero(p) = %d, expected 1", got) + } + if got := p224IsZero(&p224FieldElement{1}); got != 0 { + t.Errorf("p224IsZero(1) = %d, expected 0", got) + } + + isZeroMatchesBigInt := func(a p224FieldElement) bool { + isZero := p224IsZero(&a) + + big := p224AlternativeToBig(&a) + if big.Sign() == 0 && isZero != 1 { + return false + } + if big.Sign() != 0 && isZero != 0 { + return false + } + return true + } + + if err := quick.Check(isZeroMatchesBigInt, quickCheckConfig32); err != nil { + t.Error(err) + } +} + +func TestP224Invert(t *testing.T) { + var out p224FieldElement + + p224Invert(&out, &p224FieldElement{}) + if got := p224IsZero(&out); got != 1 { + t.Errorf("p224Invert(0) = %x, expected 0", out) + } + + p224Invert(&out, (*p224FieldElement)(&p224P)) + if got := p224IsZero(&out); got != 1 { + t.Errorf("p224Invert(p) = %x, expected 0", out) + } + + p224Invert(&out, &p224FieldElement{1}) + p224Contract(&out, &out) + if out != (p224FieldElement{1}) { + t.Errorf("p224Invert(1) = %x, expected 1", out) + } + + var tmp p224LargeFieldElement + a := p224FieldElement{1, 2, 3, 4, 5, 6, 7, 8} + p224Invert(&out, &a) + p224Mul(&out, &out, &a, &tmp) + p224Contract(&out, &out) + if out != (p224FieldElement{1}) { + t.Errorf("p224Invert(a) * a = %x, expected 1", out) + } +} -- cgit v1.2.3-54-g00ecf From e749a96e717718e3ac881a75f805776370302a86 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 11 Jan 2021 09:41:54 -0500 Subject: [release-branch.go1.14-security] cmd/go: pass resolved CC, GCCGO to cgo This makes sure the go command and cgo agree about exactly which compiler is being used. This issue was reported by RyotaK. Fixes CVE-2021-3115. Change-Id: If171c5c8b2523efb5ea2d957e5ad1380a038149c Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/949416 Reviewed-by: Ian Lance Taylor Reviewed-by: Jay Conrod (cherry picked from commit 4cf399ca38587a6e4a3e85b494cd9a9b4cc53378) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955294 Reviewed-by: Katie Hockman --- src/cmd/go/internal/work/action.go | 3 +++ src/cmd/go/internal/work/exec.go | 30 +++++++++++++++++++++++------- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/cmd/go/internal/work/action.go b/src/cmd/go/internal/work/action.go index e3cb87fbb9..8256681fd2 100644 --- a/src/cmd/go/internal/work/action.go +++ b/src/cmd/go/internal/work/action.go @@ -56,6 +56,9 @@ type Builder struct { id sync.Mutex toolIDCache map[string]string // tool name -> tool ID buildIDCache map[string]string // file name -> build ID + + cgoEnvOnce sync.Once + cgoEnvCache []string } // NOTE: Much of Action would not need to be exported if not for test. diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index a87bc802c1..b65e2a0879 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -1075,10 +1075,8 @@ func (b *Builder) vet(a *Action) error { return err } - env := b.cCompilerEnv() - if cfg.BuildToolchainName == "gccgo" { - env = append(env, "GCCGO="+BuildToolchain.compiler()) - } + // TODO(rsc): Why do we pass $GCCGO to go vet? + env := b.cgoEnv() p := a.Package tool := VetTool @@ -1979,6 +1977,24 @@ func (b *Builder) cCompilerEnv() []string { return []string{"TERM=dumb"} } +// cgoEnv returns environment variables to set when running cgo. +// Some of these pass through to cgo running the C compiler, +// so it includes cCompilerEnv. +func (b *Builder) cgoEnv() []string { + b.cgoEnvOnce.Do(func() { + cc, err := exec.LookPath(b.ccExe()[0]) + if err != nil || filepath.Base(cc) == cc { // reject relative path + cc = "/missing-cc" + } + gccgo := GccgoBin + if filepath.Base(gccgo) == gccgo { // reject relative path + gccgo = "/missing-gccgo" + } + b.cgoEnvCache = append(b.cCompilerEnv(), "CC="+cc, "GCCGO="+gccgo) + }) + return b.cgoEnvCache +} + // mkdir makes the named directory. func (b *Builder) Mkdir(dir string) error { // Make Mkdir(a.Objdir) a no-op instead of an error when a.Objdir == "". @@ -2524,13 +2540,13 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo // along to the host linker. At this point in the code, cgoLDFLAGS // consists of the original $CGO_LDFLAGS (unchecked) and all the // flags put together from source code (checked). - cgoenv := b.cCompilerEnv() + cgoenv := b.cgoEnv() if len(cgoLDFLAGS) > 0 { flags := make([]string, len(cgoLDFLAGS)) for i, f := range cgoLDFLAGS { flags[i] = strconv.Quote(f) } - cgoenv = []string{"CGO_LDFLAGS=" + strings.Join(flags, " ")} + cgoenv = append(cgoenv, "CGO_LDFLAGS="+strings.Join(flags, " ")) } if cfg.BuildToolchainName == "gccgo" { @@ -2745,7 +2761,7 @@ func (b *Builder) dynimport(a *Action, p *load.Package, objdir, importGo, cgoExe if p.Standard && p.ImportPath == "runtime/cgo" { cgoflags = []string{"-dynlinker"} // record path to dynamic linker } - return b.run(a, p.Dir, p.ImportPath, b.cCompilerEnv(), cfg.BuildToolexec, cgoExe, "-dynpackage", p.Name, "-dynimport", dynobj, "-dynout", importGo, cgoflags) + return b.run(a, p.Dir, p.ImportPath, b.cgoEnv(), cfg.BuildToolexec, cgoExe, "-dynpackage", p.Name, "-dynimport", dynobj, "-dynout", importGo, cgoflags) } // Run SWIG on all SWIG input files. -- cgit v1.2.3-54-g00ecf From 4bf9990a6bc8d7c44650f7f061639a2507104366 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 11 Jan 2021 10:01:24 -0500 Subject: [release-branch.go1.14-security] cmd/cgo: report exec errors a bit more clearly Change-Id: I0e6bebf0e2e6efdef4be880e0c6c7451b938924b Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/949417 Reviewed-by: Katie Hockman Reviewed-by: Jay Conrod Reviewed-by: Ian Lance Taylor (cherry picked from commit 4c2e5f85dda6ad5cc1d5be863ae62f2050f12be9) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955296 --- src/cmd/cgo/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmd/cgo/util.go b/src/cmd/cgo/util.go index 921306b7aa..779f7be225 100644 --- a/src/cmd/cgo/util.go +++ b/src/cmd/cgo/util.go @@ -63,7 +63,7 @@ func run(stdin []byte, argv []string) (stdout, stderr []byte, ok bool) { p.Env = append(os.Environ(), "TERM=dumb") err := p.Run() if _, ok := err.(*exec.ExitError); err != nil && !ok { - fatalf("%s", err) + fatalf("exec %s: %s", argv[0], err) } ok = p.ProcessState.Success() stdout, stderr = bout.Bytes(), berr.Bytes() @@ -88,7 +88,7 @@ func fatalf(msg string, args ...interface{}) { // If we've already printed other errors, they might have // caused the fatal condition. Assume they're enough. if nerrors == 0 { - fmt.Fprintf(os.Stderr, msg+"\n", args...) + fmt.Fprintf(os.Stderr, "cgo: "+msg+"\n", args...) } os.Exit(2) } -- cgit v1.2.3-54-g00ecf From bd04382057fcfadd78ccc80684ab4c942d1adf9a Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 11 Jan 2021 09:43:08 -0500 Subject: [release-branch.go1.14-security] cmd/go: add test case for cgo CC setting Change-Id: Ied986053a64447c5eac6369f6c9b69ed3d3f94d9 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/949415 Reviewed-by: Ian Lance Taylor (cherry picked from commit e97d4ed8dcc1fed64fe44b56dfdfb0f929aabb65) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955298 Reviewed-by: Katie Hockman --- src/cmd/go/testdata/script/cgo_path.txt | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 src/cmd/go/testdata/script/cgo_path.txt diff --git a/src/cmd/go/testdata/script/cgo_path.txt b/src/cmd/go/testdata/script/cgo_path.txt new file mode 100644 index 0000000000..e4d07dea2c --- /dev/null +++ b/src/cmd/go/testdata/script/cgo_path.txt @@ -0,0 +1,35 @@ +[!cgo] skip + +env GOCACHE=$WORK/gocache # Looking for compile flags, so need a clean cache. +[!windows] env PATH=.:$PATH +[!windows] chmod 0777 p/gcc p/clang +[!windows] exists p/gcc p/clang +[windows] exists p/gcc.bat p/clang.bat +! exists p/bug.txt +go build -x +! exists p/bug.txt + +-- go.mod -- +module m + +-- m.go -- +package m + +import _ "m/p" + +-- p/p.go -- +package p + +// #define X 1 +import "C" + +-- p/gcc -- +#!/bin/sh +echo ran gcc >bug.txt +-- p/clang -- +#!/bin/sh +echo ran clang >bug.txt +-- p/gcc.bat -- +echo ran gcc >bug.txt +-- p/clang.bat -- +echo ran clang >bug.txt -- cgit v1.2.3-54-g00ecf From 94200a92cf4d6dfdab5291f9e29785cad566faa0 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Fri, 15 Jan 2021 12:14:06 -0800 Subject: [release-branch.go1.14-security] all: introduce and use internal/execabs Introduces a wrapper around os/exec, internal/execabs, for use in all commands. This wrapper prevents exec.LookPath and exec.Command from running executables in the current directory. All imports of os/exec in non-test files in cmd/ are replaced with imports of internal/execabs. This issue was reported by RyotaK. Fixes CVE-2021-3115 Change-Id: I0423451a6e27ec1e1d6f3fe929ab1ef69145c08f Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955304 Reviewed-by: Russ Cox Reviewed-by: Katie Hockman (cherry picked from commit 44f09a6990ccf4db601cbf8208c89ac4e888f884) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/955309 --- src/cmd/api/goapi.go | 2 +- src/cmd/api/run.go | 2 +- src/cmd/cgo/out.go | 2 +- src/cmd/cgo/util.go | 2 +- src/cmd/compile/internal/ssa/html.go | 2 +- src/cmd/cover/func.go | 2 +- src/cmd/cover/testdata/toolexec.go | 2 +- src/cmd/dist/buildtool.go | 4 +- src/cmd/doc/dirs.go | 2 +- src/cmd/fix/typecheck.go | 2 +- src/cmd/go/internal/base/base.go | 2 +- src/cmd/go/internal/bug/bug.go | 2 +- src/cmd/go/internal/generate/generate.go | 2 +- src/cmd/go/internal/modfetch/codehost/codehost.go | 2 +- src/cmd/go/internal/modfetch/codehost/git.go | 2 +- src/cmd/go/internal/test/test.go | 2 +- src/cmd/go/internal/tool/tool.go | 2 +- src/cmd/go/internal/vet/vetflag.go | 2 +- src/cmd/go/internal/work/build.go | 2 +- src/cmd/go/internal/work/buildid.go | 2 +- src/cmd/go/internal/work/exec.go | 2 +- src/cmd/go/internal/work/gccgo.go | 2 +- src/cmd/go/testdata/addmod.go | 2 +- src/cmd/internal/browser/browser.go | 2 +- src/cmd/internal/diff/diff.go | 2 +- src/cmd/internal/dwarf/dwarf.go | 2 +- src/cmd/link/internal/ld/execarchive.go | 2 +- src/cmd/link/internal/ld/lib.go | 2 +- src/cmd/test2json/main.go | 2 +- src/cmd/trace/pprof.go | 2 +- src/go/build/build.go | 2 +- src/go/build/deps_test.go | 8 +- src/go/internal/gccgoimporter/gccgoinstallation.go | 2 +- src/internal/execabs/execabs.go | 70 ++++++++++++++ src/internal/execabs/execabs_test.go | 107 +++++++++++++++++++++ src/internal/goroot/gc.go | 2 +- 36 files changed, 217 insertions(+), 36 deletions(-) create mode 100644 src/internal/execabs/execabs.go create mode 100644 src/internal/execabs/execabs_test.go diff --git a/src/cmd/api/goapi.go b/src/cmd/api/goapi.go index b46b310267..26eb32866e 100644 --- a/src/cmd/api/goapi.go +++ b/src/cmd/api/goapi.go @@ -16,11 +16,11 @@ import ( "go/parser" "go/token" "go/types" + exec "internal/execabs" "io" "io/ioutil" "log" "os" - "os/exec" "path/filepath" "regexp" "runtime" diff --git a/src/cmd/api/run.go b/src/cmd/api/run.go index a36f1179c1..ecb1d0f81a 100644 --- a/src/cmd/api/run.go +++ b/src/cmd/api/run.go @@ -10,9 +10,9 @@ package main import ( "fmt" + exec "internal/execabs" "log" "os" - "os/exec" "path/filepath" "runtime" "strings" diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go index d447bcb543..698181ce1b 100644 --- a/src/cmd/cgo/out.go +++ b/src/cmd/cgo/out.go @@ -13,11 +13,11 @@ import ( "go/ast" "go/printer" "go/token" + exec "internal/execabs" "internal/xcoff" "io" "io/ioutil" "os" - "os/exec" "path/filepath" "regexp" "sort" diff --git a/src/cmd/cgo/util.go b/src/cmd/cgo/util.go index 779f7be225..00d931b98a 100644 --- a/src/cmd/cgo/util.go +++ b/src/cmd/cgo/util.go @@ -8,9 +8,9 @@ import ( "bytes" "fmt" "go/token" + exec "internal/execabs" "io/ioutil" "os" - "os/exec" ) // run runs the command argv, feeding in stdin on standard input. diff --git a/src/cmd/compile/internal/ssa/html.go b/src/cmd/compile/internal/ssa/html.go index 1e76a673ef..068c382725 100644 --- a/src/cmd/compile/internal/ssa/html.go +++ b/src/cmd/compile/internal/ssa/html.go @@ -9,9 +9,9 @@ import ( "cmd/internal/src" "fmt" "html" + exec "internal/execabs" "io" "os" - "os/exec" "path/filepath" "strconv" "strings" diff --git a/src/cmd/cover/func.go b/src/cmd/cover/func.go index 988c4caebf..ce7c771ac9 100644 --- a/src/cmd/cover/func.go +++ b/src/cmd/cover/func.go @@ -15,9 +15,9 @@ import ( "go/ast" "go/parser" "go/token" + exec "internal/execabs" "io" "os" - "os/exec" "path" "path/filepath" "runtime" diff --git a/src/cmd/cover/testdata/toolexec.go b/src/cmd/cover/testdata/toolexec.go index 1769efedbe..386de79038 100644 --- a/src/cmd/cover/testdata/toolexec.go +++ b/src/cmd/cover/testdata/toolexec.go @@ -16,7 +16,7 @@ package main import ( "os" - "os/exec" + exec "internal/execabs" "strings" ) diff --git a/src/cmd/dist/buildtool.go b/src/cmd/dist/buildtool.go index a07e64b472..095ebbd076 100644 --- a/src/cmd/dist/buildtool.go +++ b/src/cmd/dist/buildtool.go @@ -298,8 +298,10 @@ func bootstrapFixImports(srcFile string) string { continue } if strings.HasPrefix(line, `import "`) || strings.HasPrefix(line, `import . "`) || - inBlock && (strings.HasPrefix(line, "\t\"") || strings.HasPrefix(line, "\t. \"")) { + inBlock && (strings.HasPrefix(line, "\t\"") || strings.HasPrefix(line, "\t. \"") || strings.HasPrefix(line, "\texec \"")) { line = strings.Replace(line, `"cmd/`, `"bootstrap/cmd/`, -1) + // During bootstrap, must use plain os/exec. + line = strings.Replace(line, `exec "internal/execabs"`, `"os/exec"`, -1) for _, dir := range bootstrapDirs { if strings.HasPrefix(dir, "cmd/") { continue diff --git a/src/cmd/doc/dirs.go b/src/cmd/doc/dirs.go index 38cbe7fa02..661624cfe4 100644 --- a/src/cmd/doc/dirs.go +++ b/src/cmd/doc/dirs.go @@ -7,9 +7,9 @@ package main import ( "bytes" "fmt" + exec "internal/execabs" "log" "os" - "os/exec" "path/filepath" "regexp" "strings" diff --git a/src/cmd/fix/typecheck.go b/src/cmd/fix/typecheck.go index 66e0cdcec0..8390e4446c 100644 --- a/src/cmd/fix/typecheck.go +++ b/src/cmd/fix/typecheck.go @@ -9,9 +9,9 @@ import ( "go/ast" "go/parser" "go/token" + exec "internal/execabs" "io/ioutil" "os" - "os/exec" "path/filepath" "reflect" "runtime" diff --git a/src/cmd/go/internal/base/base.go b/src/cmd/go/internal/base/base.go index 272da55681..15e603552c 100644 --- a/src/cmd/go/internal/base/base.go +++ b/src/cmd/go/internal/base/base.go @@ -12,9 +12,9 @@ import ( "flag" "fmt" "go/scanner" + exec "internal/execabs" "log" "os" - "os/exec" "strings" "sync" diff --git a/src/cmd/go/internal/bug/bug.go b/src/cmd/go/internal/bug/bug.go index fe71281ef0..9434bc2b1c 100644 --- a/src/cmd/go/internal/bug/bug.go +++ b/src/cmd/go/internal/bug/bug.go @@ -8,11 +8,11 @@ package bug import ( "bytes" "fmt" + exec "internal/execabs" "io" "io/ioutil" urlpkg "net/url" "os" - "os/exec" "path/filepath" "regexp" "runtime" diff --git a/src/cmd/go/internal/generate/generate.go b/src/cmd/go/internal/generate/generate.go index 315db69de8..758fa3be58 100644 --- a/src/cmd/go/internal/generate/generate.go +++ b/src/cmd/go/internal/generate/generate.go @@ -9,10 +9,10 @@ import ( "bufio" "bytes" "fmt" + exec "internal/execabs" "io" "log" "os" - "os/exec" "path/filepath" "regexp" "strconv" diff --git a/src/cmd/go/internal/modfetch/codehost/codehost.go b/src/cmd/go/internal/modfetch/codehost/codehost.go index 5867288c96..383981da34 100644 --- a/src/cmd/go/internal/modfetch/codehost/codehost.go +++ b/src/cmd/go/internal/modfetch/codehost/codehost.go @@ -10,10 +10,10 @@ import ( "bytes" "crypto/sha256" "fmt" + exec "internal/execabs" "io" "io/ioutil" "os" - "os/exec" "path/filepath" "strings" "sync" diff --git a/src/cmd/go/internal/modfetch/codehost/git.go b/src/cmd/go/internal/modfetch/codehost/git.go index f08df512f0..aa696ecf8b 100644 --- a/src/cmd/go/internal/modfetch/codehost/git.go +++ b/src/cmd/go/internal/modfetch/codehost/git.go @@ -8,11 +8,11 @@ import ( "bytes" "errors" "fmt" + exec "internal/execabs" "io" "io/ioutil" "net/url" "os" - "os/exec" "path/filepath" "sort" "strconv" diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 4ad142ccd8..a23a1bbf2a 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -10,10 +10,10 @@ import ( "errors" "fmt" "go/build" + exec "internal/execabs" "io" "io/ioutil" "os" - "os/exec" "path" "path/filepath" "regexp" diff --git a/src/cmd/go/internal/tool/tool.go b/src/cmd/go/internal/tool/tool.go index 930eecb63f..f06c9038a0 100644 --- a/src/cmd/go/internal/tool/tool.go +++ b/src/cmd/go/internal/tool/tool.go @@ -7,8 +7,8 @@ package tool import ( "fmt" + exec "internal/execabs" "os" - "os/exec" "sort" "strings" diff --git a/src/cmd/go/internal/vet/vetflag.go b/src/cmd/go/internal/vet/vetflag.go index e3de48bbff..052b8f1bb0 100644 --- a/src/cmd/go/internal/vet/vetflag.go +++ b/src/cmd/go/internal/vet/vetflag.go @@ -9,9 +9,9 @@ import ( "encoding/json" "flag" "fmt" + exec "internal/execabs" "log" "os" - "os/exec" "path/filepath" "strings" diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go index e3b25c937c..c19e6f1a43 100644 --- a/src/cmd/go/internal/work/build.go +++ b/src/cmd/go/internal/work/build.go @@ -8,8 +8,8 @@ import ( "errors" "fmt" "go/build" + exec "internal/execabs" "os" - "os/exec" "path/filepath" "runtime" "strings" diff --git a/src/cmd/go/internal/work/buildid.go b/src/cmd/go/internal/work/buildid.go index 7558a3091a..5fbdbb6a37 100644 --- a/src/cmd/go/internal/work/buildid.go +++ b/src/cmd/go/internal/work/buildid.go @@ -7,9 +7,9 @@ package work import ( "bytes" "fmt" + exec "internal/execabs" "io/ioutil" "os" - "os/exec" "strings" "cmd/go/internal/base" diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index b65e2a0879..d5c7506247 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -16,13 +16,13 @@ import ( "encoding/json" "errors" "fmt" + exec "internal/execabs" "internal/lazyregexp" "io" "io/ioutil" "log" "math/rand" "os" - "os/exec" "path/filepath" "regexp" "runtime" diff --git a/src/cmd/go/internal/work/gccgo.go b/src/cmd/go/internal/work/gccgo.go index 4c1f36dbd6..2f5d5d6283 100644 --- a/src/cmd/go/internal/work/gccgo.go +++ b/src/cmd/go/internal/work/gccgo.go @@ -6,9 +6,9 @@ package work import ( "fmt" + exec "internal/execabs" "io/ioutil" "os" - "os/exec" "path/filepath" "strings" diff --git a/src/cmd/go/testdata/addmod.go b/src/cmd/go/testdata/addmod.go index d9c3aab9c4..9c74cf500b 100644 --- a/src/cmd/go/testdata/addmod.go +++ b/src/cmd/go/testdata/addmod.go @@ -25,7 +25,7 @@ import ( "io/ioutil" "log" "os" - "os/exec" + exec "internal/execabs" "path/filepath" "strings" diff --git a/src/cmd/internal/browser/browser.go b/src/cmd/internal/browser/browser.go index 6867c85d23..577d31789f 100644 --- a/src/cmd/internal/browser/browser.go +++ b/src/cmd/internal/browser/browser.go @@ -6,8 +6,8 @@ package browser import ( + exec "internal/execabs" "os" - "os/exec" "runtime" "time" ) diff --git a/src/cmd/internal/diff/diff.go b/src/cmd/internal/diff/diff.go index e9d2c23780..c0ca2f3106 100644 --- a/src/cmd/internal/diff/diff.go +++ b/src/cmd/internal/diff/diff.go @@ -7,9 +7,9 @@ package diff import ( + exec "internal/execabs" "io/ioutil" "os" - "os/exec" "runtime" ) diff --git a/src/cmd/internal/dwarf/dwarf.go b/src/cmd/internal/dwarf/dwarf.go index 56b44a1ab5..f97a1279fe 100644 --- a/src/cmd/internal/dwarf/dwarf.go +++ b/src/cmd/internal/dwarf/dwarf.go @@ -12,7 +12,7 @@ import ( "cmd/internal/objabi" "errors" "fmt" - "os/exec" + exec "internal/execabs" "sort" "strconv" "strings" diff --git a/src/cmd/link/internal/ld/execarchive.go b/src/cmd/link/internal/ld/execarchive.go index fe5cc40865..4687c624de 100644 --- a/src/cmd/link/internal/ld/execarchive.go +++ b/src/cmd/link/internal/ld/execarchive.go @@ -7,8 +7,8 @@ package ld import ( + exec "internal/execabs" "os" - "os/exec" "path/filepath" "syscall" ) diff --git a/src/cmd/link/internal/ld/lib.go b/src/cmd/link/internal/ld/lib.go index cd63963d7f..be6ffa0224 100644 --- a/src/cmd/link/internal/ld/lib.go +++ b/src/cmd/link/internal/ld/lib.go @@ -51,11 +51,11 @@ import ( "encoding/binary" "encoding/hex" "fmt" + exec "internal/execabs" "io" "io/ioutil" "log" "os" - "os/exec" "path/filepath" "runtime" "sort" diff --git a/src/cmd/test2json/main.go b/src/cmd/test2json/main.go index 0385d8f246..52bad6bec8 100644 --- a/src/cmd/test2json/main.go +++ b/src/cmd/test2json/main.go @@ -82,9 +82,9 @@ package main import ( "flag" "fmt" + exec "internal/execabs" "io" "os" - "os/exec" "cmd/internal/test2json" ) diff --git a/src/cmd/trace/pprof.go b/src/cmd/trace/pprof.go index a31d71b013..e6ea1a4ab6 100644 --- a/src/cmd/trace/pprof.go +++ b/src/cmd/trace/pprof.go @@ -9,12 +9,12 @@ package main import ( "bufio" "fmt" + exec "internal/execabs" "internal/trace" "io" "io/ioutil" "net/http" "os" - "os/exec" "path/filepath" "runtime" "sort" diff --git a/src/go/build/build.go b/src/go/build/build.go index 1a122c615f..9136319ad9 100644 --- a/src/go/build/build.go +++ b/src/go/build/build.go @@ -12,13 +12,13 @@ import ( "go/doc" "go/parser" "go/token" + exec "internal/execabs" "internal/goroot" "internal/goversion" "io" "io/ioutil" "log" "os" - "os/exec" pathpkg "path" "path/filepath" "runtime" diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index 8eed6260a2..6106afaa0c 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -207,6 +207,8 @@ var pkgDeps = map[string][]string{ "internal/lazyregexp": {"L2", "OS", "regexp"}, "internal/lazytemplate": {"L2", "OS", "text/template"}, + "internal/execabs": {"L2", "OS", "fmt", "context", "reflect"}, + // L4 is defined as L3+fmt+log+time, because in general once // you're using L3 packages, use of fmt, log, or time is not a big deal. "L4": { @@ -240,7 +242,7 @@ var pkgDeps = map[string][]string{ "go/constant": {"L4", "go/token", "math/big"}, "go/importer": {"L4", "go/build", "go/internal/gccgoimporter", "go/internal/gcimporter", "go/internal/srcimporter", "go/token", "go/types"}, "go/internal/gcimporter": {"L4", "OS", "go/build", "go/constant", "go/token", "go/types", "text/scanner"}, - "go/internal/gccgoimporter": {"L4", "OS", "debug/elf", "go/constant", "go/token", "go/types", "internal/xcoff", "text/scanner"}, + "go/internal/gccgoimporter": {"L4", "OS", "debug/elf", "go/constant", "go/token", "go/types", "internal/xcoff", "text/scanner", "internal/execabs"}, "go/internal/srcimporter": {"L4", "OS", "fmt", "go/ast", "go/build", "go/parser", "go/token", "go/types", "path/filepath"}, "go/types": {"L4", "GOPARSER", "container/heap", "go/constant"}, @@ -272,7 +274,7 @@ var pkgDeps = map[string][]string{ "encoding/pem": {"L4"}, "encoding/xml": {"L4", "encoding"}, "flag": {"L4", "OS"}, - "go/build": {"L4", "OS", "GOPARSER", "internal/goroot", "internal/goversion"}, + "go/build": {"L4", "OS", "GOPARSER", "internal/goroot", "internal/goversion", "internal/execabs"}, "html": {"L4"}, "image/draw": {"L4", "image/internal/imageutil"}, "image/gif": {"L4", "compress/lzw", "image/color/palette", "image/draw"}, @@ -280,7 +282,7 @@ var pkgDeps = map[string][]string{ "image/jpeg": {"L4", "image/internal/imageutil"}, "image/png": {"L4", "compress/zlib"}, "index/suffixarray": {"L4", "regexp"}, - "internal/goroot": {"L4", "OS"}, + "internal/goroot": {"L4", "OS", "internal/execabs"}, "internal/singleflight": {"sync"}, "internal/trace": {"L4", "OS", "container/heap"}, "internal/xcoff": {"L4", "OS", "debug/dwarf"}, diff --git a/src/go/internal/gccgoimporter/gccgoinstallation.go b/src/go/internal/gccgoimporter/gccgoinstallation.go index 8fc7ce3232..e90a3cc0b0 100644 --- a/src/go/internal/gccgoimporter/gccgoinstallation.go +++ b/src/go/internal/gccgoimporter/gccgoinstallation.go @@ -7,8 +7,8 @@ package gccgoimporter import ( "bufio" "go/types" + exec "internal/execabs" "os" - "os/exec" "path/filepath" "strings" ) diff --git a/src/internal/execabs/execabs.go b/src/internal/execabs/execabs.go new file mode 100644 index 0000000000..547c3a50c8 --- /dev/null +++ b/src/internal/execabs/execabs.go @@ -0,0 +1,70 @@ +// Copyright 2020 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 execabs is a drop-in replacement for os/exec +// that requires PATH lookups to find absolute paths. +// That is, execabs.Command("cmd") runs the same PATH lookup +// as exec.Command("cmd"), but if the result is a path +// which is relative, the Run and Start methods will report +// an error instead of running the executable. +package execabs + +import ( + "context" + "fmt" + "os/exec" + "path/filepath" + "reflect" + "unsafe" +) + +var ErrNotFound = exec.ErrNotFound + +type ( + Cmd = exec.Cmd + Error = exec.Error + ExitError = exec.ExitError +) + +func relError(file, path string) error { + return fmt.Errorf("%s resolves to executable in current directory (.%c%s)", file, filepath.Separator, path) +} + +func LookPath(file string) (string, error) { + path, err := exec.LookPath(file) + if err != nil { + return "", err + } + if filepath.Base(file) == file && !filepath.IsAbs(path) { + return "", relError(file, path) + } + return path, nil +} + +func fixCmd(name string, cmd *exec.Cmd) { + if filepath.Base(name) == name && !filepath.IsAbs(cmd.Path) { + // exec.Command was called with a bare binary name and + // exec.LookPath returned a path which is not absolute. + // Set cmd.lookPathErr and clear cmd.Path so that it + // cannot be run. + lookPathErr := (*error)(unsafe.Pointer(reflect.ValueOf(cmd).Elem().FieldByName("lookPathErr").Addr().Pointer())) + if *lookPathErr == nil { + *lookPathErr = relError(name, cmd.Path) + } + cmd.Path = "" + } +} + +func CommandContext(ctx context.Context, name string, arg ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, name, arg...) + fixCmd(name, cmd) + return cmd + +} + +func Command(name string, arg ...string) *exec.Cmd { + cmd := exec.Command(name, arg...) + fixCmd(name, cmd) + return cmd +} diff --git a/src/internal/execabs/execabs_test.go b/src/internal/execabs/execabs_test.go new file mode 100644 index 0000000000..a0b88dd2a0 --- /dev/null +++ b/src/internal/execabs/execabs_test.go @@ -0,0 +1,107 @@ +// Copyright 2020 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 execabs + +import ( + "context" + "fmt" + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" +) + +func TestFixCmd(t *testing.T) { + cmd := &exec.Cmd{Path: "hello"} + fixCmd("hello", cmd) + if cmd.Path != "" { + t.Errorf("fixCmd didn't clear cmd.Path") + } + expectedErr := fmt.Sprintf("hello resolves to executable in current directory (.%chello)", filepath.Separator) + if err := cmd.Run(); err == nil { + t.Fatal("Command.Run didn't fail") + } else if err.Error() != expectedErr { + t.Fatalf("Command.Run returned unexpected error: want %q, got %q", expectedErr, err.Error()) + } +} + +func TestCommand(t *testing.T) { + for _, cmd := range []func(string) *Cmd{ + func(s string) *Cmd { return Command(s) }, + func(s string) *Cmd { return CommandContext(context.Background(), s) }, + } { + tmpDir, err := ioutil.TempDir("", "execabs-test") + if err != nil { + t.Fatalf("ioutil.TempDir failed: %s", err) + } + defer os.RemoveAll(tmpDir) + executable := "execabs-test" + if runtime.GOOS == "windows" { + executable += ".exe" + } + if err = ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0111); err != nil { + t.Fatalf("ioutil.WriteFile failed: %s", err) + } + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd failed: %s", err) + } + defer os.Chdir(cwd) + if err = os.Chdir(tmpDir); err != nil { + t.Fatalf("os.Chdir failed: %s", err) + } + if runtime.GOOS != "windows" { + // add "." to PATH so that exec.LookPath looks in the current directory on + // non-windows platforms as well + origPath := os.Getenv("PATH") + defer os.Setenv("PATH", origPath) + os.Setenv("PATH", fmt.Sprintf(".:%s", origPath)) + } + expectedErr := fmt.Sprintf("execabs-test resolves to executable in current directory (.%c%s)", filepath.Separator, executable) + if err = cmd("execabs-test").Run(); err == nil { + t.Fatalf("Command.Run didn't fail when exec.LookPath returned a relative path") + } else if err.Error() != expectedErr { + t.Errorf("Command.Run returned unexpected error: want %q, got %q", expectedErr, err.Error()) + } + } +} + +func TestLookPath(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "execabs-test") + if err != nil { + t.Fatalf("ioutil.TempDir failed: %s", err) + } + defer os.RemoveAll(tmpDir) + executable := "execabs-test" + if runtime.GOOS == "windows" { + executable += ".exe" + } + if err = ioutil.WriteFile(filepath.Join(tmpDir, executable), []byte{1, 2, 3}, 0111); err != nil { + t.Fatalf("ioutil.WriteFile failed: %s", err) + } + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("os.Getwd failed: %s", err) + } + defer os.Chdir(cwd) + if err = os.Chdir(tmpDir); err != nil { + t.Fatalf("os.Chdir failed: %s", err) + } + if runtime.GOOS != "windows" { + // add "." to PATH so that exec.LookPath looks in the current directory on + // non-windows platforms as well + origPath := os.Getenv("PATH") + defer os.Setenv("PATH", origPath) + os.Setenv("PATH", fmt.Sprintf(".:%s", origPath)) + } + expectedErr := fmt.Sprintf("execabs-test resolves to executable in current directory (.%c%s)", filepath.Separator, executable) + if _, err := LookPath("execabs-test"); err == nil { + t.Fatalf("LookPath didn't fail when finding a non-relative path") + } else if err.Error() != expectedErr { + t.Errorf("LookPath returned unexpected error: want %q, got %q", expectedErr, err.Error()) + } +} diff --git a/src/internal/goroot/gc.go b/src/internal/goroot/gc.go index 0f541d734b..ce72bc3896 100644 --- a/src/internal/goroot/gc.go +++ b/src/internal/goroot/gc.go @@ -7,8 +7,8 @@ package goroot import ( + exec "internal/execabs" "os" - "os/exec" "path/filepath" "strings" "sync" -- cgit v1.2.3-54-g00ecf From 05822ab49228c3351af1973a7d6345e5cd01083a Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 19 Jan 2021 09:59:24 -0800 Subject: [release-branch.go1.14-security] cmd/go: overwrite program name with full path If the program path is resolved, replace the first argument of the exec.Cmd, which is the bare program name with the resolved path. Change-Id: I92cf5e6f4bb7c8fef9b59f5eab963f4e75b90d07 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/957908 Reviewed-by: Katie Hockman Reviewed-by: Russ Cox Reviewed-by: Jay Conrod (cherry picked from commit a863cb56b33a24aad88f23f1d48629dc4b4b9539) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/958253 Reviewed-by: Dmitri Shuralyov --- src/cmd/go/internal/work/exec.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index d5c7506247..8230d85938 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -1918,6 +1918,9 @@ func (b *Builder) runOut(a *Action, dir string, env []string, cmdargs ...interfa var buf bytes.Buffer cmd := exec.Command(cmdline[0], cmdline[1:]...) + if cmd.Path != "" { + cmd.Args[0] = cmd.Path + } cmd.Stdout = &buf cmd.Stderr = &buf cleanup := passLongArgsInResponseFiles(cmd) -- cgit v1.2.3-54-g00ecf From ccb4f250bd7e382e50824c36ec5a3e1a57dcf11a Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Tue, 19 Jan 2021 13:37:47 -0500 Subject: [release-branch.go1.14-security] go1.14.14 Change-Id: Id4260bbb5aa55b7e93c0c4686f174ea7916c14db Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/957919 Reviewed-by: Roland Shoemaker --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 03e2b577a6..31a26de05c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.14.13 \ No newline at end of file +go1.14.14 \ No newline at end of file -- cgit v1.2.3-54-g00ecf