New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: add more options to security whitelist #23937

Closed
olt opened this Issue Feb 20, 2018 · 36 comments

Comments

Projects
None yet
10 participants
@olt
Contributor

olt commented Feb 20, 2018

What version of Go are you using (go version)?

Go 1.10

What operating system and processor architecture are you using (go env)?

MacOS 10.13.2

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/olt/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/olt/dev/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/np/2fhk15fn5qq6fm4j9s3vt_6r0000gn/T/go-build391399925=/tmp/go-build -gno-record-gcc-switches -fno-common"

Debian Jessie 3.16.0-4-amd64

GOARCH="amd64"
GOBIN=""
GOCHAR="6"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/olt/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"

What did you do?

Build code cgo code which includes the evaluated output of

// #cgo CXXFLAGS: $(mapnik-config --cxxflags --includes --dep-includes | tr '\n' ' ')

The output of mapnik-config --cxxflags is as follows (for two different versions):

-g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g0 -ansi -Wall -pthread -O2 -fno-strict-aliasing -finline-functions -Wno-inline -Wno-parentheses -Wno-char-subscripts

and

-std=c++11 -stdlib=libc++ -DBOOST_EXCEPTION_DISABLE -fvisibility=hidden -fvisibility-inlines-hidden -Wall -ftemplate-depth-300 -Wsign-compare -Wshadow -O3

What did you see instead?

For the first version:

go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -ansi
go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -finline-functions

It works with the following env: CGO_CXXFLAGS_ALLOW='-ansi|-finline-functions'

For the second version:

go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -stdlib=libc++
go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -fvisibility=hidden
go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -fvisibility-inlines-hidden
go build: xx/xx/xx: invalid flag in #cgo CXXFLAGS: -ftemplate-depth-300

CGO_CXXFLAGS_ALLOW='-stdlib=.*|-fvisibility=hidden|-fvisibility-inlines-hidden|-ftemplate-depth[=-]\d+

-stdlib is already included in master, but the other options are missing. Note that -ftemplate-depth-300 uses - instead of =.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 20, 2018

More options to add to the whitelists:

From #23904:

LDFLAGS: -Wl,-rpath=$ORIGIN/lib

From #23909:

LDFLAGS: -mwindows

From #23923:

CXXFLAGS: -ffat-lto-objects
CXXFLAGS: -fuse-linker-plugin

(Note that -fuse-linker-plugin is safe as long as -B is not used, and -B is never safe anyhow.)

From #23749:

LDFLAGS: -mmacosx-version-min=10.8

(We currently accept -mmacos-x for the compiler, but not the linker.)

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 20, 2018

From #23749:

LDFLAGS: -O3
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 20, 2018

From #23938:

CFLAGS: -w
@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 21, 2018

(You meant to say 23904 instead of 23094 there; in case that bug accidentally gets marked as fixed by this fix.)

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 21, 2018

Thanks; I updated the comment.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 21, 2018

Reported on #23749:

LDFLAGS: -shared
@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 21, 2018

(Oops, missed it: likewise for 23090. Not sure what the right issue number there is.)

Seconding -mmacosx in the linker here; was just about to report that myself. I will point out that the macOS linker will complain if the values of this flag conflict for any object file; what does Go itself do?

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Feb 21, 2018

-mwindows is from #23909

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 22, 2018

Corrected issue reference in comment above. Thanks.

Go itself does not even know about the -maccosx option, and as such will not complain about it.

@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 22, 2018

No; I meant to ask what does Go set in the Mach-O headers for the minimum version. I can make that a separate discussion, though.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 22, 2018

@andlabs Currently when using internal linking the program requests OS X version 10.7.0. When using external linking it's set by the system linker. Let's take any followup on this to golang-nuts.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 23, 2018

From #23749:

LDFLAGS: -Wl,-static
@mattn

This comment has been minimized.

Member

mattn commented Feb 23, 2018

go build github.com/golang-ui/nuklear/nk: invalid flag in #cgo LDFLAGS: -Wl,--allow-multiple-definition
@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 25, 2018

I am starting to see a number of people on my own project assume that the consequences of the security fix are bugs in my project specifically — people are updating Go without realizing the security patch exists in in the first place. I'll be editing up my README, but I have to wonder what other projects also have this problem — and worse, what missing options have gone unnoticed by us as a result.

To further address the lack of information-spreading, I wonder if the error message from cgo should point to the security fix announcement as well.

Furthermore, at least two people are resorting to setting the _ALLOW env vars to .* as a workaround. Should we be disallowing this entirely, since it unintentionally circumvents the security fix? I'm probably going to find out who originated the idea; hopefully it's on github and thus can be fine-tuned...

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 25, 2018

@andlabs In 1.10 the error message points to https://golang.org/s/invalidflag, and in 1.9.5 it will as well.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 25, 2018

From #24113:

CFLAGS: -arch
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 25, 2018

From #23749:

CFLAGS: -m32
LDFLAGS: -O3
LDFLAGS: -static-libgcc
LDFLAGS: -static-libstdc++
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 26, 2018

From #23749:

LDFLAGS: -Wl,-z,relro

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Feb 26, 2018

LDFLAGS: -Wl,-z,relro
Is from #24124 and I see a bunch more flags in the sample code.

LDFLAGS: -Wl,-undefined 
LDFLAGS: -Wl,dynamic_lookup 
LDFLAGS: -Wl,-unresolved-symbols=ignore-all 
LDFLAGS: -Wl,-z,noexecstack 
LDFLAGS: -Wl,-dn 
LDFLAGS: -Wl,-dy

Might be a good idea to ask the user to use CGO_CFLAGS_ALLOW and friends to find out if there are more flags not in the sample code.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 6, 2018

From #23749:

LDFLAGS: -Wl,--subsystem,windows
CFLAGS: -mwindows
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 6, 2018

From #23749:

-isysroot (ios)
-mios-simulator-version-min= (ios)
-miphoneos-version-min= (ios)

-target (for android)
-sysroot (for android)

@eliasnaur Are these compiler options or linker options?

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented Mar 6, 2018

-target and --sysroot are put in CFLAGS, CPPFLAGS and LDFLAGS by gomobile on android. Note that I spelled --sysroot with one dash in #23749. Sorry.

-mios-simulator-version-min=, -miphoneos-version-min=, and -isysroot are passed to CFLAGS and LDFLAGS by gomobile on ios.

@andlabs

This comment has been minimized.

Contributor

andlabs commented Mar 6, 2018

(You should probably put those iOS CFLAGS into CXXFLAGS as well, lest someone using Objective-C++ be blocked.)

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented Mar 7, 2018

I found another one: "-arch ". It's added to all of CFLAG, CPPFLAG, and LDFLAG and isn't the same as -march.

@mattn

This comment has been minimized.

Member

mattn commented Mar 10, 2018

-Wa,-mbig-obj to link big objects.

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Mar 11, 2018

From #23749 (comment)
LDFLAGS: -Wl,-undefined,dynamic_lookup

@coolgeek6667

This comment has been minimized.

coolgeek6667 commented Mar 23, 2018

A couple more CFLAGS that I ran into in my current project:
-fsigned-char
-funsigned-char

@JakeDOD

This comment has been minimized.

JakeDOD commented Mar 23, 2018

LDFLAGS: -Wl,-sectcreate,segname,sectname,filename to add sections to the binary.

From a security standpoint, on Darwin this is also an important flag for adding the __RESTRICT,__restrict segment to prevent DYLD environment variable injection.

@andlabs

This comment has been minimized.

Contributor

andlabs commented Mar 23, 2018

@JakeDOD what is this __RESTRICT,__restrict injection thing, out of curiosity?

@JakeDOD

This comment has been minimized.

JakeDOD commented Mar 23, 2018

@andlabs More generally, it allows a macOS binary to opt-in as a restricted binary and tell DYLD to prune dangerous environment variables like DYLD_INSERT_LIBRARIES, override the use of @rpaths, and limit Framework and dylib loading to the SIP protected locations /System/Library/Frameworks and /usr/lib respectively. See dyld.cpp source

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Mar 27, 2018

In case they're missed:

From #23749 (comment)

CFLAGS: -fmessage-length=152
CFLAGS: -fdiagnostics-show-note-include-stack
CFLAGS: -fmacro-backtrace-limit=0

From #23749 (comment)
(Assuming CFLAGS): -stdlib=libstdc++

@gopherbot

This comment has been minimized.

gopherbot commented Mar 28, 2018

Change https://golang.org/cl/102818 mentions this issue: cmd/go: add more C compiler/linker options to whitelist

@gopherbot gopherbot closed this in 7e34ac1 Mar 28, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Mar 28, 2018

Reopening for 1.10.1 and 1.9.5 cherry picks.

@andybons

This comment has been minimized.

Member

andybons commented Mar 28, 2018

CL 102818 OK for Go 1.10.1

@gopherbot

This comment has been minimized.

gopherbot commented Mar 28, 2018

Change https://golang.org/cl/103015 mentions this issue: [release-branch.go1.10] cmd/go: add more C compiler/linker options to whitelist

@gopherbot

This comment has been minimized.

gopherbot commented Mar 28, 2018

Change https://golang.org/cl/103156 mentions this issue: [release-branch.go1.9] cmd/go: add more C compiler/linker options to whitelist

gopherbot pushed a commit that referenced this issue Mar 29, 2018

[release-branch.go1.9] cmd/go: add more C compiler/linker options to …
…whitelist

Fixes #23937

Change-Id: Ie63d91355d1a724d0012d99d457d939deeeb8d3e
Reviewed-on: https://go-review.googlesource.com/102818
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Reviewed-on: https://go-review.googlesource.com/103156
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot pushed a commit that referenced this issue Mar 29, 2018

[release-branch.go1.10] cmd/go: add more C compiler/linker options to…
… whitelist

Fixes #23937

Change-Id: Ie63d91355d1a724d0012d99d457d939deeeb8d3e
Reviewed-on: https://go-review.googlesource.com/102818
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Reviewed-on: https://go-review.googlesource.com/103015
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@andybons andybons closed this Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment