diff --git a/misc/cgo/errors/src/err1.go b/misc/cgo/errors/src/err1.go index 61bbcd2957..2c232cf58a 100644 --- a/misc/cgo/errors/src/err1.go +++ b/misc/cgo/errors/src/err1.go @@ -5,7 +5,7 @@ package main /* -#cgo LDFLAGS: -c +#cgo LDFLAGS: -L/nonexist void test() { xxx; // ERROR HERE diff --git a/src/cmd/cgo/doc.go b/src/cmd/cgo/doc.go index 8e4cd88b37..c16b63a313 100644 --- a/src/cmd/cgo/doc.go +++ b/src/cmd/cgo/doc.go @@ -45,8 +45,8 @@ For example: // #include import "C" -Alternatively, CPPFLAGS and LDFLAGS may be obtained via the pkg-config -tool using a '#cgo pkg-config:' directive followed by the package names. +Alternatively, CPPFLAGS and LDFLAGS may be obtained via the pkg-config tool +using a '#cgo pkg-config:' directive followed by the package names. For example: // #cgo pkg-config: png cairo @@ -55,11 +55,21 @@ For example: The default pkg-config tool may be changed by setting the PKG_CONFIG environment variable. +For security reasons, only a limited set of flags are allowed, notably -D, -I, and -l. +To allow additional flags, set CGO_CFLAGS_ALLOW to a regular expression +matching the new flags. To disallow flags that would otherwise be allowed, +set CGO_CFLAGS_DISALLOW to a regular expression matching arguments +that must be disallowed. In both cases the regular expression must match +a full argument: to allow -mfoo=bar, use CGO_CFLAGS_ALLOW='-mfoo.*', +not just CGO_CFLAGS_ALLOW='-mfoo'. Similarly named variables control +the allowed CPPFLAGS, CXXFLAGS, FFLAGS, and LDFLAGS. + When building, the CGO_CFLAGS, CGO_CPPFLAGS, CGO_CXXFLAGS, CGO_FFLAGS and CGO_LDFLAGS environment variables are added to the flags derived from these directives. Package-specific flags should be set using the directives, not the environment variables, so that builds work in -unmodified environments. +unmodified environments. Flags obtained from environment variables +are not subject to the security limitations described above. All the cgo CPPFLAGS and CFLAGS directives in a package are concatenated and used to compile C files in that package. All the CPPFLAGS and CXXFLAGS diff --git a/src/cmd/compile/internal/gc/noder.go b/src/cmd/compile/internal/gc/noder.go index dcd5f20dfd..fff04bcbef 100644 --- a/src/cmd/compile/internal/gc/noder.go +++ b/src/cmd/compile/internal/gc/noder.go @@ -7,6 +7,7 @@ package gc import ( "fmt" "os" + "path/filepath" "runtime" "strconv" "strings" @@ -1346,6 +1347,11 @@ func (p *noder) pragma(pos src.Pos, text string) syntax.Pragma { p.linknames = append(p.linknames, linkname{pos, f[1], f[2]}) case strings.HasPrefix(text, "go:cgo_"): + // For security, we disallow //go:cgo_* directives outside cgo-generated files. + // Exception: they are allowed in the standard library, for runtime and syscall. + if !isCgoGeneratedFile(pos) && !compiling_std { + p.error(syntax.Error{Pos: pos, Msg: fmt.Sprintf("//%s only allowed in cgo-generated code", text)}) + } p.pragcgobuf += p.pragcgo(pos, text) fallthrough // because of //go:cgo_unsafe_args default: @@ -1367,6 +1373,16 @@ func (p *noder) pragma(pos src.Pos, text string) syntax.Pragma { return 0 } +// isCgoGeneratedFile reports whether pos is in a file +// generated by cgo, which is to say a file with name +// beginning with "_cgo_". Such files are allowed to +// contain cgo directives, and for security reasons +// (primarily misuse of linker flags), other files are not. +// See golang.org/issue/23672. +func isCgoGeneratedFile(pos src.Pos) bool { + return strings.HasPrefix(filepath.Base(filepath.Clean(pos.AbsFilename())), "_cgo_") +} + func mkname(sym *types.Sym) *Node { n := oldname(sym) if n.Name != nil && n.Name.Pack != nil { diff --git a/src/cmd/dist/build.go b/src/cmd/dist/build.go index e80d466d35..49ed80033e 100644 --- a/src/cmd/dist/build.go +++ b/src/cmd/dist/build.go @@ -784,7 +784,7 @@ func runInstall(dir string, ch chan struct{}) { } else { archive = b } - compile := []string{pathf("%s/compile", tooldir), "-pack", "-o", b, "-p", pkg} + compile := []string{pathf("%s/compile", tooldir), "-std", "-pack", "-o", b, "-p", pkg} if gogcflags != "" { compile = append(compile, strings.Fields(gogcflags)...) } diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index 91657eb74c..7557647ea6 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -1227,17 +1227,26 @@ // CGO_CFLAGS // Flags that cgo will pass to the compiler when compiling // C code. -// CGO_CPPFLAGS -// Flags that cgo will pass to the compiler when compiling -// C or C++ code. -// CGO_CXXFLAGS -// Flags that cgo will pass to the compiler when compiling -// C++ code. -// CGO_FFLAGS -// Flags that cgo will pass to the compiler when compiling -// Fortran code. -// CGO_LDFLAGS -// Flags that cgo will pass to the compiler when linking. +// CGO_CFLAGS_ALLOW +// A regular expression specifying additional flags to allow +// to appear in #cgo CFLAGS source code directives. +// Does not apply to the CGO_CFLAGS environment variable. +// CGO_CFLAGS_DISALLOW +// A regular expression specifying flags that must be disallowed +// from appearing in #cgo CFLAGS source code directives. +// Does not apply to the CGO_CFLAGS environment variable. +// CGO_CPPFLAGS, CGO_CPPFLAGS_ALLOW, CGO_CPPFLAGS_DISALLOW +// Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, +// but for the C preprocessor. +// CGO_CXXFLAGS, CGO_CXXFLAGS_ALLOW, CGO_CXXFLAGS_DISALLOW +// Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, +// but for the C++ compiler. +// CGO_FFLAGS, CGO_FFLAGS_ALLOW, CGO_FFLAGS_DISALLOW +// Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, +// but for the Fortran compiler. +// CGO_LDFLAGS, CGO_LDFLAGS_ALLOW, CGO_LDFLAGS_DISALLOW +// Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, +// but for the linker. // CXX // The command to use to compile C++ code. // PKG_CONFIG diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 92600b6238..3972d79e3f 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -2789,7 +2789,7 @@ func TestCgoHandlesWlORIGIN(t *testing.T) { defer tg.cleanup() tg.parallel() tg.tempFile("src/origin/origin.go", `package origin - // #cgo !darwin LDFLAGS: -Wl,-rpath -Wl,$ORIGIN + // #cgo !darwin LDFLAGS: -Wl,-rpath,$ORIGIN // void f(void) {} import "C" func f() { C.f() }`) @@ -5739,3 +5739,152 @@ func TestAtomicCoverpkgAll(t *testing.T) { tg.run("test", "-coverpkg=all", "-race", "x") } } + +func TestBadCommandLines(t *testing.T) { + tg := testgo(t) + defer tg.cleanup() + + tg.tempFile("src/x/x.go", "package x\n") + tg.setenv("GOPATH", tg.path(".")) + + tg.run("build", "x") + + tg.tempFile("src/x/@y.go", "package x\n") + tg.runFail("build", "x") + tg.grepStderr("invalid input file name \"@y.go\"", "did not reject @y.go") + tg.must(os.Remove(tg.path("src/x/@y.go"))) + + tg.tempFile("src/x/-y.go", "package x\n") + tg.runFail("build", "x") + tg.grepStderr("invalid input file name \"-y.go\"", "did not reject -y.go") + tg.must(os.Remove(tg.path("src/x/-y.go"))) + + tg.runFail("build", "-gcflags=all=@x", "x") + tg.grepStderr("invalid command-line argument @x in command", "did not reject @x during exec") + + tg.tempFile("src/@x/x.go", "package x\n") + tg.setenv("GOPATH", tg.path(".")) + tg.runFail("build", "@x") + tg.grepStderr("invalid input directory name \"@x\"", "did not reject @x directory") + + tg.tempFile("src/@x/y/y.go", "package y\n") + tg.setenv("GOPATH", tg.path(".")) + tg.runFail("build", "@x/y") + tg.grepStderr("invalid import path \"@x/y\"", "did not reject @x/y import path") + + tg.tempFile("src/-x/x.go", "package x\n") + tg.setenv("GOPATH", tg.path(".")) + tg.runFail("build", "--", "-x") + tg.grepStderr("invalid input directory name \"-x\"", "did not reject -x directory") + + tg.tempFile("src/-x/y/y.go", "package y\n") + tg.setenv("GOPATH", tg.path(".")) + tg.runFail("build", "--", "-x/y") + tg.grepStderr("invalid import path \"-x/y\"", "did not reject -x/y import path") +} + +func TestBadCgoDirectives(t *testing.T) { + if !canCgo { + t.Skip("no cgo") + } + tg := testgo(t) + defer tg.cleanup() + + tg.tempFile("src/x/x.go", "package x\n") + tg.setenv("GOPATH", tg.path(".")) + + tg.tempFile("src/x/x.go", `package x + + //go:cgo_ldflag "-fplugin=foo.so" + + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("//go:cgo_ldflag .* only allowed in cgo-generated code", "did not reject //go:cgo_ldflag directive") + + tg.must(os.Remove(tg.path("src/x/x.go"))) + tg.runFail("build", "x") + tg.grepStderr("no Go files", "did not report missing source code") + tg.tempFile("src/x/_cgo_yy.go", `package x + + //go:cgo_ldflag "-fplugin=foo.so" + + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("no Go files", "did not report missing source code") // _* files are ignored... + + tg.runFail("build", tg.path("src/x/_cgo_yy.go")) // ... but if forced, the comment is rejected + // Actually, today there is a separate issue that _ files named + // on the command-line are ignored. Once that is fixed, + // we want to see the cgo_ldflag error. + tg.grepStderr("//go:cgo_ldflag only allowed in cgo-generated code|no Go files", "did not reject //go:cgo_ldflag directive") + tg.must(os.Remove(tg.path("src/x/_cgo_yy.go"))) + + tg.tempFile("src/x/x.go", "package x\n") + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: -fplugin=foo.so + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: -fplugin=foo.so", "did not reject -fplugin") + + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: -Ibar -fplugin=foo.so + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: -fplugin=foo.so", "did not reject -fplugin") + + tg.tempFile("src/x/y.go", `package x + // #cgo pkg-config: -foo + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid pkg-config package name: -foo", "did not reject pkg-config: -foo") + + tg.tempFile("src/x/y.go", `package x + // #cgo pkg-config: @foo + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid pkg-config package name: @foo", "did not reject pkg-config: -foo") + + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: @foo + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: @foo", "did not reject @foo flag") + + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: -D + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: -D without argument", "did not reject trailing -I flag") + + // Note that -I @foo is allowed because we rewrite it into -I /path/to/src/@foo + // before the check is applied. There's no such rewrite for -D. + + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: -D @foo + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: -D @foo", "did not reject -D @foo flag") + + tg.tempFile("src/x/y.go", `package x + // #cgo CFLAGS: -D@foo + import "C" + `) + tg.runFail("build", "x") + tg.grepStderr("invalid flag in #cgo CFLAGS: -D@foo", "did not reject -D@foo flag") + + tg.setenv("CGO_CFLAGS", "-D@foo") + tg.tempFile("src/x/y.go", `package x + import "C" + `) + tg.run("build", "-n", "x") + tg.grepStderr("-D@foo", "did not find -D@foo in commands") +} diff --git a/src/cmd/go/internal/envcmd/env.go b/src/cmd/go/internal/envcmd/env.go index fa19bebe21..603f7b5060 100644 --- a/src/cmd/go/internal/envcmd/env.go +++ b/src/cmd/go/internal/envcmd/env.go @@ -113,7 +113,12 @@ func findEnv(env []cfg.EnvVar, name string) string { func ExtraEnvVars() []cfg.EnvVar { var b work.Builder b.Init() - cppflags, cflags, cxxflags, fflags, ldflags := b.CFlags(&load.Package{}) + cppflags, cflags, cxxflags, fflags, ldflags, err := b.CFlags(&load.Package{}) + if err != nil { + // Should not happen - b.CFlags was given an empty package. + fmt.Fprintf(os.Stderr, "go: invalid cflags: %v\n", err) + return nil + } cmd := b.GccCmd(".", "") return []cfg.EnvVar{ // Note: Update the switch in runEnv below when adding to this list. diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index 4ebf206078..c39af79604 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/src/cmd/go/internal/help/helpdoc.go @@ -487,17 +487,26 @@ Environment variables for use with cgo: CGO_CFLAGS Flags that cgo will pass to the compiler when compiling C code. - CGO_CPPFLAGS - Flags that cgo will pass to the compiler when compiling - C or C++ code. - CGO_CXXFLAGS - Flags that cgo will pass to the compiler when compiling - C++ code. - CGO_FFLAGS - Flags that cgo will pass to the compiler when compiling - Fortran code. - CGO_LDFLAGS - Flags that cgo will pass to the compiler when linking. + CGO_CFLAGS_ALLOW + A regular expression specifying additional flags to allow + to appear in #cgo CFLAGS source code directives. + Does not apply to the CGO_CFLAGS environment variable. + CGO_CFLAGS_DISALLOW + A regular expression specifying flags that must be disallowed + from appearing in #cgo CFLAGS source code directives. + Does not apply to the CGO_CFLAGS environment variable. + CGO_CPPFLAGS, CGO_CPPFLAGS_ALLOW, CGO_CPPFLAGS_DISALLOW + Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, + but for the C preprocessor. + CGO_CXXFLAGS, CGO_CXXFLAGS_ALLOW, CGO_CXXFLAGS_DISALLOW + Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, + but for the C++ compiler. + CGO_FFLAGS, CGO_FFLAGS_ALLOW, CGO_FFLAGS_DISALLOW + Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, + but for the Fortran compiler. + CGO_LDFLAGS, CGO_LDFLAGS_ALLOW, CGO_LDFLAGS_DISALLOW + Like CGO_CFLAGS, CGO_CFLAGS_ALLOW, and CGO_CFLAGS_DISALLOW, + but for the linker. CXX The command to use to compile C++ code. PKG_CONFIG diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go index eaff7177b6..bbd75bc9b6 100644 --- a/src/cmd/go/internal/load/pkg.go +++ b/src/cmd/go/internal/load/pkg.go @@ -16,6 +16,7 @@ import ( "sort" "strings" "unicode" + "unicode/utf8" "cmd/go/internal/base" "cmd/go/internal/cfg" @@ -54,6 +55,8 @@ type PackagePublic struct { StaleReason string `json:",omitempty"` // why is Stale true? // Source files + // If you add to this list you MUST add to p.AllFiles (below) too. + // Otherwise file name security lists will not apply to any new additions. GoFiles []string `json:",omitempty"` // .go source files (excluding CgoFiles, TestGoFiles, XTestGoFiles) CgoFiles []string `json:",omitempty"` // .go sources files that import "C" IgnoredGoFiles []string `json:",omitempty"` // .go sources ignored due to build constraints @@ -85,12 +88,38 @@ type PackagePublic struct { DepsErrors []*PackageError `json:",omitempty"` // errors loading dependencies // Test information + // If you add to this list you MUST add to p.AllFiles (below) too. + // Otherwise file name security lists will not apply to any new additions. TestGoFiles []string `json:",omitempty"` // _test.go files in package TestImports []string `json:",omitempty"` // imports from TestGoFiles XTestGoFiles []string `json:",omitempty"` // _test.go files outside package XTestImports []string `json:",omitempty"` // imports from XTestGoFiles } +// AllFiles returns the names of all the files considered for the package. +// This is used for sanity and security checks, so we include all files, +// even IgnoredGoFiles, because some subcommands consider them. +// The go/build package filtered others out (like foo_wrongGOARCH.s) +// and that's OK. +func (p *Package) AllFiles() []string { + return str.StringList( + p.GoFiles, + p.CgoFiles, + p.IgnoredGoFiles, + p.CFiles, + p.CXXFiles, + p.MFiles, + p.HFiles, + p.FFiles, + p.SFiles, + p.SwigFiles, + p.SwigCXXFiles, + p.SysoFiles, + p.TestGoFiles, + p.XTestGoFiles, + ) +} + type PackageInternal struct { // Unexported fields are not part of the public API. Build *build.Package @@ -1005,22 +1034,8 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) { // To avoid problems on case-insensitive files, we reject any package // where two different input files have equal names under a case-insensitive // comparison. - f1, f2 := str.FoldDup(str.StringList( - p.GoFiles, - p.CgoFiles, - p.IgnoredGoFiles, - p.CFiles, - p.CXXFiles, - p.MFiles, - p.HFiles, - p.FFiles, - p.SFiles, - p.SysoFiles, - p.SwigFiles, - p.SwigCXXFiles, - p.TestGoFiles, - p.XTestGoFiles, - )) + inputs := p.AllFiles() + f1, f2 := str.FoldDup(inputs) if f1 != "" { p.Error = &PackageError{ ImportStack: stk.Copy(), @@ -1029,6 +1044,37 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) { return } + // If first letter of input file is ASCII, it must be alphanumeric. + // This avoids files turning into flags when invoking commands, + // and other problems we haven't thought of yet. + // Also, _cgo_ files must be generated by us, not supplied. + // They are allowed to have //go:cgo_ldflag directives. + // The directory scan ignores files beginning with _, + // so we shouldn't see any _cgo_ files anyway, but just be safe. + for _, file := range inputs { + if !SafeArg(file) || strings.HasPrefix(file, "_cgo_") { + p.Error = &PackageError{ + ImportStack: stk.Copy(), + Err: fmt.Sprintf("invalid input file name %q", file), + } + return + } + } + if name := pathpkg.Base(p.ImportPath); !SafeArg(name) { + p.Error = &PackageError{ + ImportStack: stk.Copy(), + Err: fmt.Sprintf("invalid input directory name %q", name), + } + return + } + if !SafeArg(p.ImportPath) { + p.Error = &PackageError{ + ImportStack: stk.Copy(), + Err: fmt.Sprintf("invalid import path %q", p.ImportPath), + } + return + } + // Build list of imported packages and full dependency list. imports := make([]*Package, 0, len(p.Imports)) for i, path := range importPaths { @@ -1152,6 +1198,22 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) { } } +// SafeArg reports whether arg is a "safe" command-line argument, +// meaning that when it appears in a command-line, it probably +// doesn't have some special meaning other than its own name. +// Obviously args beginning with - are not safe (they look like flags). +// Less obviously, args beginning with @ are not safe (they look like +// GNU binutils flagfile specifiers, sometimes called "response files"). +// To be conservative, we reject almost any arg beginning with non-alphanumeric ASCII. +// We accept leading . _ and / as likely in file system paths. +func SafeArg(name string) bool { + if name == "" { + return false + } + c := name[0] + return '0' <= c && c <= '9' || 'A' <= c && c <= 'Z' || 'a' <= c && c <= 'z' || c == '.' || c == '_' || c == '/' || c >= utf8.RuneSelf +} + // LinkerDeps returns the list of linker-induced dependencies for main package p. func LinkerDeps(p *Package) []string { // Everything links runtime. diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index 58bab5cb2f..a5ab75f6a8 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -197,7 +197,7 @@ func (b *Builder) buildActionID(a *Action) cache.ActionID { fmt.Fprintf(h, "omitdebug %v standard %v local %v prefix %q\n", p.Internal.OmitDebug, p.Standard, p.Internal.Local, p.Internal.LocalPrefix) if len(p.CgoFiles)+len(p.SwigFiles) > 0 { fmt.Fprintf(h, "cgo %q\n", b.toolID("cgo")) - cppflags, cflags, cxxflags, fflags, _ := b.CFlags(p) + cppflags, cflags, cxxflags, fflags, _, _ := b.CFlags(p) fmt.Fprintf(h, "CC=%q %q %q\n", b.ccExe(), cppflags, cflags) if len(p.CXXFiles)+len(p.SwigFiles) > 0 { fmt.Fprintf(h, "CXX=%q %q\n", b.cxxExe(), cxxflags) @@ -521,7 +521,14 @@ func (b *Builder) build(a *Action) (err error) { } // Prepare Go import config. + // We start it off with a comment so it can't be empty, so icfg.Bytes() below is never nil. + // It should never be empty anyway, but there have been bugs in the past that resulted + // in empty configs, which then unfortunately turn into "no config passed to compiler", + // and the compiler falls back to looking in pkg itself, which mostly works, + // except when it doesn't. var icfg bytes.Buffer + fmt.Fprintf(&icfg, "# import config\n") + for i, raw := range a.Package.Internal.RawImports { final := a.Package.Imports[i] if final != raw { @@ -928,28 +935,38 @@ func splitPkgConfigOutput(out []byte) []string { // Calls pkg-config if needed and returns the cflags/ldflags needed to build the package. func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string, err error) { if pkgs := p.CgoPkgConfig; len(pkgs) > 0 { + for _, pkg := range pkgs { + if !load.SafeArg(pkg) { + return nil, nil, fmt.Errorf("invalid pkg-config package name: %s", pkg) + } + } var out []byte - out, err = b.runOut(p.Dir, p.ImportPath, nil, b.PkgconfigCmd(), "--cflags", pkgs) + out, err = b.runOut(p.Dir, p.ImportPath, nil, b.PkgconfigCmd(), "--cflags", "--", pkgs) if err != nil { b.showOutput(nil, p.Dir, b.PkgconfigCmd()+" --cflags "+strings.Join(pkgs, " "), string(out)) b.Print(err.Error() + "\n") - err = errPrintedOutput - return + return nil, nil, errPrintedOutput } if len(out) > 0 { cflags = splitPkgConfigOutput(out) + if err := checkCompilerFlags("CFLAGS", "pkg-config --cflags", cflags); err != nil { + return nil, nil, err + } } - out, err = b.runOut(p.Dir, p.ImportPath, nil, b.PkgconfigCmd(), "--libs", pkgs) + out, err = b.runOut(p.Dir, p.ImportPath, nil, b.PkgconfigCmd(), "--libs", "--", pkgs) if err != nil { b.showOutput(nil, p.Dir, b.PkgconfigCmd()+" --libs "+strings.Join(pkgs, " "), string(out)) b.Print(err.Error() + "\n") - err = errPrintedOutput - return + return nil, nil, errPrintedOutput } if len(out) > 0 { ldflags = strings.Fields(string(out)) + if err := checkLinkerFlags("CFLAGS", "pkg-config --cflags", ldflags); err != nil { + return nil, nil, err + } } } + return } @@ -1447,6 +1464,17 @@ func (b *Builder) processOutput(out []byte) string { // It returns the command output and any errors that occurred. func (b *Builder) runOut(dir string, desc string, env []string, cmdargs ...interface{}) ([]byte, error) { cmdline := str.StringList(cmdargs...) + + for _, arg := range cmdline { + // GNU binutils commands, including gcc and gccgo, interpret an argument + // @foo anywhere in the command line (even following --) as meaning + // "read and insert arguments from the file named foo." + // Don't say anything that might be misinterpreted that way. + if strings.HasPrefix(arg, "@") { + return nil, fmt.Errorf("invalid command-line argument %s in command: %s", arg, joinUnambiguously(cmdline)) + } + } + if cfg.BuildN || cfg.BuildX { var envcmdline string for _, e := range env { @@ -1888,22 +1916,44 @@ func envList(key, def string) []string { } // CFlags returns the flags to use when invoking the C, C++ or Fortran compilers, or cgo. -func (b *Builder) CFlags(p *load.Package) (cppflags, cflags, cxxflags, fflags, ldflags []string) { +func (b *Builder) CFlags(p *load.Package) (cppflags, cflags, cxxflags, fflags, ldflags []string, err error) { defaults := "-g -O2" - cppflags = str.StringList(envList("CGO_CPPFLAGS", ""), p.CgoCPPFLAGS) - cflags = str.StringList(envList("CGO_CFLAGS", defaults), p.CgoCFLAGS) - cxxflags = str.StringList(envList("CGO_CXXFLAGS", defaults), p.CgoCXXFLAGS) - fflags = str.StringList(envList("CGO_FFLAGS", defaults), p.CgoFFLAGS) - ldflags = str.StringList(envList("CGO_LDFLAGS", defaults), p.CgoLDFLAGS) + if cppflags, err = buildFlags("CPPFLAGS", "", p.CgoCPPFLAGS, checkCompilerFlags); err != nil { + return + } + if cflags, err = buildFlags("CFLAGS", defaults, p.CgoCFLAGS, checkCompilerFlags); err != nil { + return + } + if cxxflags, err = buildFlags("CXXFLAGS", defaults, p.CgoCXXFLAGS, checkCompilerFlags); err != nil { + return + } + if fflags, err = buildFlags("FFLAGS", defaults, p.CgoFFLAGS, checkCompilerFlags); err != nil { + return + } + if ldflags, err = buildFlags("LDFLAGS", defaults, p.CgoLDFLAGS, checkLinkerFlags); err != nil { + return + } + return } +func buildFlags(name, defaults string, fromPackage []string, check func(string, string, []string) error) ([]string, error) { + if err := check(name, "#cgo "+name, fromPackage); err != nil { + return nil, err + } + return str.StringList(envList("CGO_"+name, defaults), fromPackage), nil +} + var cgoRe = regexp.MustCompile(`[/\\:]`) func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgofiles, gccfiles, gxxfiles, mfiles, ffiles []string) (outGo, outObj []string, err error) { p := a.Package - cgoCPPFLAGS, cgoCFLAGS, cgoCXXFLAGS, cgoFFLAGS, cgoLDFLAGS := b.CFlags(p) + cgoCPPFLAGS, cgoCFLAGS, cgoCXXFLAGS, cgoFFLAGS, cgoLDFLAGS, err := b.CFlags(p) + if err != nil { + return nil, nil, err + } + cgoCPPFLAGS = append(cgoCPPFLAGS, pcCFLAGS...) cgoLDFLAGS = append(cgoLDFLAGS, pcLDFLAGS...) // If we are compiling Objective-C code, then we need to link against libobjc @@ -1953,6 +2003,12 @@ func (b *Builder) cgo(a *Action, cgoExe, objdir string, pcCFLAGS, pcLDFLAGS, cgo } // Update $CGO_LDFLAGS with p.CgoLDFLAGS. + // These flags are recorded in the generated _cgo_gotypes.go file + // using //go:cgo_ldflag directives, the compiler records them in the + // object file for the package, and then the Go linker passes them + // 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). var cgoenv []string if len(cgoLDFLAGS) > 0 { flags := make([]string, len(cgoLDFLAGS)) @@ -2245,7 +2301,11 @@ func (b *Builder) swigIntSize(objdir string) (intsize string, err error) { // Run SWIG on one SWIG input file. func (b *Builder) swigOne(a *Action, p *load.Package, file, objdir string, pcCFLAGS []string, cxx bool, intgosize string) (outGo, outC string, err error) { - cgoCPPFLAGS, cgoCFLAGS, cgoCXXFLAGS, _, _ := b.CFlags(p) + cgoCPPFLAGS, cgoCFLAGS, cgoCXXFLAGS, _, _, err := b.CFlags(p) + if err != nil { + return "", "", err + } + var cflags []string if cxx { cflags = str.StringList(cgoCPPFLAGS, pcCFLAGS, cgoCXXFLAGS) diff --git a/src/cmd/go/internal/work/security.go b/src/cmd/go/internal/work/security.go new file mode 100644 index 0000000000..fee5beeb15 --- /dev/null +++ b/src/cmd/go/internal/work/security.go @@ -0,0 +1,160 @@ +// 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. + +// Checking of compiler and linker flags. +// We must avoid flags like -fplugin=, which can allow +// arbitrary code execution during the build. +// Do not make changes here without carefully +// considering the implications. +// (That's why the code is isolated in a file named security.go.) +// +// Note that -Wl,foo means split foo on commas and pass to +// the linker, so that -Wl,-foo,bar means pass -foo bar to +// the linker. Similarly -Wa,foo for the assembler and so on. +// If any of these are permitted, the wildcard portion must +// disallow commas. +// +// Note also that GNU binutils accept any argument @foo +// as meaning "read more flags from the file foo", so we must +// guard against any command-line argument beginning with @, +// even things like "-I @foo". +// We use load.SafeArg (which is even more conservative) +// to reject these. +// +// Even worse, gcc -I@foo (one arg) turns into cc1 -I @foo (two args), +// so although gcc doesn't expand the @foo, cc1 will. +// So out of paranoia, we reject @ at the beginning of every +// flag argument that might be split into its own argument. + +package work + +import ( + "cmd/go/internal/load" + "fmt" + "os" + "regexp" +) + +var re = regexp.MustCompile + +var validCompilerFlags = []*regexp.Regexp{ + re(`-D([A-Za-z_].*)`), + re(`-I([^@\-].*)`), + re(`-O`), + re(`-O([^@\-].*)`), + re(`-W`), + re(`-W([^@,]+)`), // -Wall but not -Wa,-foo. + re(`-f(no-)?objc-arc`), + re(`-f(no-)?omit-frame-pointer`), + re(`-f(no-)?(pic|PIC|pie|PIE)`), + re(`-f(no-)?split-stack`), + re(`-f(no-)?stack-(.+)`), + re(`-f(no-)?strict-aliasing`), + re(`-fsanitize=(.+)`), + re(`-g([^@\-].*)?`), + re(`-m(arch|cpu|fpu|tune)=([^@\-].*)`), + re(`-m(no-)?stack-(.+)`), + re(`-mmacosx-(.+)`), + re(`-mnop-fun-dllimport`), + re(`-pthread`), + re(`-std=([^@\-].*)`), + re(`-x([^@\-].*)`), +} + +var validCompilerFlagsWithNextArg = []string{ + "-D", + "-I", + "-framework", + "-x", +} + +var validLinkerFlags = []*regexp.Regexp{ + re(`-F([^@\-].*)`), + re(`-l([^@\-].*)`), + re(`-L([^@\-].*)`), + re(`-f(no-)?(pic|PIC|pie|PIE)`), + re(`-fsanitize=([^@\-].*)`), + re(`-g([^@\-].*)?`), + re(`-m(arch|cpu|fpu|tune)=([^@\-].*)`), + re(`-(pic|PIC|pie|PIE)`), + re(`-pthread`), + + // Note that any wildcards in -Wl need to exclude comma, + // since -Wl splits its argument at commas and passes + // them all to the linker uninterpreted. Allowing comma + // in a wildcard would allow tunnelling arbitrary additional + // linker arguments through one of these. + re(`-Wl,-rpath,([^,@\-][^,]+)`), + re(`-Wl,--(no-)?warn-([^,]+)`), + + re(`[a-zA-Z0-9_].*\.(o|obj|dll|dylib|so)`), // direct linker inputs: x.o or libfoo.so (but not -foo.o or @foo.o) +} + +var validLinkerFlagsWithNextArg = []string{ + "-F", + "-l", + "-L", + "-framework", +} + +func checkCompilerFlags(name, source string, list []string) error { + return checkFlags(name, source, list, validCompilerFlags, validCompilerFlagsWithNextArg) +} + +func checkLinkerFlags(name, source string, list []string) error { + return checkFlags(name, source, list, validLinkerFlags, validLinkerFlagsWithNextArg) +} + +func checkFlags(name, source string, list []string, valid []*regexp.Regexp, validNext []string) error { + // Let users override rules with $CGO_CFLAGS_ALLOW, $CGO_CFLAGS_DISALLOW, etc. + var ( + allow *regexp.Regexp + disallow *regexp.Regexp + ) + if env := os.Getenv("CGO_" + name + "_ALLOW"); env != "" { + r, err := regexp.Compile(env) + if err != nil { + return fmt.Errorf("parsing $CGO_%s_ALLOW: %v", name, err) + } + allow = r + } + if env := os.Getenv("CGO_" + name + "_DISALLOW"); env != "" { + r, err := regexp.Compile(env) + if err != nil { + return fmt.Errorf("parsing $CGO_%s_DISALLOW: %v", name, err) + } + disallow = r + } + +Args: + for i := 0; i < len(list); i++ { + arg := list[i] + if disallow != nil && disallow.FindString(arg) == arg { + goto Bad + } + if allow != nil && allow.FindString(arg) == arg { + continue Args + } + for _, re := range valid { + if re.FindString(arg) == arg { // must be complete match + continue Args + } + } + for _, x := range validNext { + if arg == x { + if i+1 < len(list) && load.SafeArg(list[i+1]) { + i++ + continue Args + } + if i+1 < len(list) { + return fmt.Errorf("invalid flag in %s: %s %s", source, arg, list[i+1]) + } + return fmt.Errorf("invalid flag in %s: %s without argument", source, arg) + } + } + Bad: + return fmt.Errorf("invalid flag in %s: %s", source, arg) + } + return nil +} diff --git a/src/cmd/go/internal/work/security_test.go b/src/cmd/go/internal/work/security_test.go new file mode 100644 index 0000000000..739ab5a6ee --- /dev/null +++ b/src/cmd/go/internal/work/security_test.go @@ -0,0 +1,240 @@ +// 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 work + +import ( + "os" + "testing" +) + +var goodCompilerFlags = [][]string{ + {"-DFOO"}, + {"-Dfoo=bar"}, + {"-I/"}, + {"-I/etc/passwd"}, + {"-I."}, + {"-O"}, + {"-O2"}, + {"-Osmall"}, + {"-W"}, + {"-Wall"}, + {"-fobjc-arc"}, + {"-fno-objc-arc"}, + {"-fomit-frame-pointer"}, + {"-fno-omit-frame-pointer"}, + {"-fpic"}, + {"-fno-pic"}, + {"-fPIC"}, + {"-fno-PIC"}, + {"-fpie"}, + {"-fno-pie"}, + {"-fPIE"}, + {"-fno-PIE"}, + {"-fsplit-stack"}, + {"-fno-split-stack"}, + {"-fstack-xxx"}, + {"-fno-stack-xxx"}, + {"-fsanitize=hands"}, + {"-g"}, + {"-ggdb"}, + {"-march=souza"}, + {"-mcpu=123"}, + {"-mfpu=123"}, + {"-mtune=happybirthday"}, + {"-mstack-overflow"}, + {"-mno-stack-overflow"}, + {"-mmacosx-version"}, + {"-mnop-fun-dllimport"}, + {"-pthread"}, + {"-std=c99"}, + {"-xc"}, + {"-D", "FOO"}, + {"-D", "foo=bar"}, + {"-I", "."}, + {"-I", "/etc/passwd"}, + {"-I", "世界"}, + {"-framework", "Chocolate"}, + {"-x", "c"}, +} + +var badCompilerFlags = [][]string{ + {"-D@X"}, + {"-D-X"}, + {"-I@dir"}, + {"-I-dir"}, + {"-O@1"}, + {"-Wa,-foo"}, + {"-W@foo"}, + {"-g@gdb"}, + {"-g-gdb"}, + {"-march=@dawn"}, + {"-march=-dawn"}, + {"-std=@c99"}, + {"-std=-c99"}, + {"-x@c"}, + {"-x-c"}, + {"-D", "@foo"}, + {"-D", "-foo"}, + {"-I", "@foo"}, + {"-I", "-foo"}, + {"-framework", "-Caffeine"}, + {"-framework", "@Home"}, + {"-x", "--c"}, + {"-x", "@obj"}, +} + +func TestCheckCompilerFlags(t *testing.T) { + for _, f := range goodCompilerFlags { + if err := checkCompilerFlags("test", "test", f); err != nil { + t.Errorf("unexpected error for %q: %v", f, err) + } + } + for _, f := range badCompilerFlags { + if err := checkCompilerFlags("test", "test", f); err == nil { + t.Errorf("missing error for %q", f) + } + } +} + +var goodLinkerFlags = [][]string{ + {"-Fbar"}, + {"-lbar"}, + {"-Lbar"}, + {"-fpic"}, + {"-fno-pic"}, + {"-fPIC"}, + {"-fno-PIC"}, + {"-fpie"}, + {"-fno-pie"}, + {"-fPIE"}, + {"-fno-PIE"}, + {"-fsanitize=hands"}, + {"-g"}, + {"-ggdb"}, + {"-march=souza"}, + {"-mcpu=123"}, + {"-mfpu=123"}, + {"-mtune=happybirthday"}, + {"-pic"}, + {"-pthread"}, + {"-Wl,-rpath,foo"}, + {"-Wl,-rpath,$ORIGIN/foo"}, + {"-Wl,--warn-error"}, + {"-Wl,--no-warn-error"}, + {"foo.so"}, + {"_世界.dll"}, + {"libcgosotest.dylib"}, + {"-F", "framework"}, + {"-l", "."}, + {"-l", "/etc/passwd"}, + {"-l", "世界"}, + {"-L", "framework"}, + {"-framework", "Chocolate"}, +} + +var badLinkerFlags = [][]string{ + {"-DFOO"}, + {"-Dfoo=bar"}, + {"-O"}, + {"-O2"}, + {"-Osmall"}, + {"-W"}, + {"-Wall"}, + {"-fobjc-arc"}, + {"-fno-objc-arc"}, + {"-fomit-frame-pointer"}, + {"-fno-omit-frame-pointer"}, + {"-fsplit-stack"}, + {"-fno-split-stack"}, + {"-fstack-xxx"}, + {"-fno-stack-xxx"}, + {"-mstack-overflow"}, + {"-mno-stack-overflow"}, + {"-mmacosx-version"}, + {"-mnop-fun-dllimport"}, + {"-std=c99"}, + {"-xc"}, + {"-D", "FOO"}, + {"-D", "foo=bar"}, + {"-I", "FOO"}, + {"-L", "@foo"}, + {"-L", "-foo"}, + {"-x", "c"}, + {"-D@X"}, + {"-D-X"}, + {"-I@dir"}, + {"-I-dir"}, + {"-O@1"}, + {"-Wa,-foo"}, + {"-W@foo"}, + {"-g@gdb"}, + {"-g-gdb"}, + {"-march=@dawn"}, + {"-march=-dawn"}, + {"-std=@c99"}, + {"-std=-c99"}, + {"-x@c"}, + {"-x-c"}, + {"-D", "@foo"}, + {"-D", "-foo"}, + {"-I", "@foo"}, + {"-I", "-foo"}, + {"-l", "@foo"}, + {"-l", "-foo"}, + {"-framework", "-Caffeine"}, + {"-framework", "@Home"}, + {"-x", "--c"}, + {"-x", "@obj"}, + {"-Wl,-rpath,@foo"}, +} + +func TestCheckLinkerFlags(t *testing.T) { + for _, f := range goodLinkerFlags { + if err := checkLinkerFlags("test", "test", f); err != nil { + t.Errorf("unexpected error for %q: %v", f, err) + } + } + for _, f := range badLinkerFlags { + if err := checkLinkerFlags("test", "test", f); err == nil { + t.Errorf("missing error for %q", f) + } + } +} + +func TestCheckFlagAllowDisallow(t *testing.T) { + if err := checkCompilerFlags("TEST", "test", []string{"-disallow"}); err == nil { + t.Fatalf("missing error for -disallow") + } + os.Setenv("CGO_TEST_ALLOW", "-disallo") + if err := checkCompilerFlags("TEST", "test", []string{"-disallow"}); err == nil { + t.Fatalf("missing error for -disallow with CGO_TEST_ALLOW=-disallo") + } + os.Setenv("CGO_TEST_ALLOW", "-disallow") + if err := checkCompilerFlags("TEST", "test", []string{"-disallow"}); err != nil { + t.Fatalf("unexpected error for -disallow with CGO_TEST_ALLOW=-disallow: %v", err) + } + os.Unsetenv("CGO_TEST_ALLOW") + + if err := checkCompilerFlags("TEST", "test", []string{"-Wall"}); err != nil { + t.Fatalf("unexpected error for -Wall: %v", err) + } + os.Setenv("CGO_TEST_DISALLOW", "-Wall") + if err := checkCompilerFlags("TEST", "test", []string{"-Wall"}); err == nil { + t.Fatalf("missing error for -Wall with CGO_TEST_DISALLOW=-Wall") + } + os.Setenv("CGO_TEST_ALLOW", "-Wall") // disallow wins + if err := checkCompilerFlags("TEST", "test", []string{"-Wall"}); err == nil { + t.Fatalf("missing error for -Wall with CGO_TEST_DISALLOW=-Wall and CGO_TEST_ALLOW=-Wall") + } + + os.Setenv("CGO_TEST_ALLOW", "-fplugin.*") + os.Setenv("CGO_TEST_DISALLOW", "-fplugin=lint.so") + if err := checkCompilerFlags("TEST", "test", []string{"-fplugin=faster.so"}); err != nil { + t.Fatalf("unexpected error for -fplugin=faster.so: %v", err) + } + if err := checkCompilerFlags("TEST", "test", []string{"-fplugin=lint.so"}); err == nil { + t.Fatalf("missing error for -fplugin=lint.so: %v", err) + } +}