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: options missing from cgo whitelists #23749

Closed
ianlancetaylor opened this Issue Feb 8, 2018 · 138 comments

Comments

Projects
None yet
@ianlancetaylor
Contributor

ianlancetaylor commented Feb 8, 2018

The recent security patches for cmd/go (#23672) added a whitelist of options permitted for use with cgo. Several different people have reported packages that fail to build by default because they use options that are not on the whitelists. This issue is intended to collect a list of all the missing options, so that we can add them in a single CL.

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Feb 8, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

#23737 reports that flags generated by pkg-config -libs are being checked against the compiler whitelist, and CGO_CFLAGS_ALLOW, when they should instead by checked against the linker whitelist and CGO_LDFLAGS_ALLOW. There is a CL to fix this: https://golang.org/cl/92755.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

The linker whitelist should accept .a files as it already accepts .o, .so, etc., files. There is a CL to fix this: https://golang.org/cl/92855. This is #23739.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

Comments on #23672 list these compiler options:

-fno-exceptions
-fno-rtti
-fpermissive

and these linker options:

-Wl,-framework
-Wl,--no-as-needed
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

#23742 suggests adding the compiler option -fmodules. The clang compiler supports a number of -fmodules options, but it's not clear that they are all safe. In particular -fmodules-cache-path and -fmodules-user-build-path would seem to permit specifying a path that clang will use to read modules, which could perhaps change the compilation mode in various ways.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

#23743 suggests adding the linker option -Wl,--no-as-needed. There is a CL for this: https://golang.org/cl/92795.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

#23744 suggests adding these compiler options:

-finput-charset=UTF-8
--std=c99
-pedantic-errors
@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

There are many compiler and linker options that may be used with either a single dash or a double dash. We should be similarly lax in the whitelists.

@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 8, 2018

For orthogonality: I forget if the option to add a directory to -framework's search path is already covered or not. I also forget what option that is. (The typical use case that I can think of is /Library/Frameworks, which is where Apple puts app-specific frameworks, and is not searched by default.)

@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 8, 2018

Also is -as-needed safe to use with cgo in the first place? This blog post (which is the first result I can find for "gcc as-needed") says that it's a positional argument, but I'm not sure if cgo guarantees anything about where your flags go on the resultant command lines.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

@andlabs It's fine to write

#cgo LDFLAGS: -Wl,--as-needed -loptlib -WL,--no-as-needed

In any case, the topic for this issue should be whether options are safe to use from go get.

@rgburke

This comment has been minimized.

rgburke commented Feb 8, 2018

When using:

#cgo !windows pkg-config: --static ${SRCDIR}/vendor/libgit2/build/libgit2.pc

compilation fails with the following message:

invalid pkg-config package name: --static

Looking over the code (for go 1.9.4) it appears there aren't any enviroment variables that can be used to whiteslist pkg-config arguments.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 8, 2018

@rgburke pkg-config output goes through the same FLAGS_ALLOW variables as other outputs.

However, it appears that pkg-config -libs checks CGO_CFLAGS_ALLOW when it ought to check CGO_LDFLAGS_ALLOW.

@mirtchovski

This comment has been minimized.

Contributor

mirtchovski commented Feb 8, 2018

We link a large number of C libraries statically in a closed-source project. Up until now we have been doing the following:

#cgo LDFLAGS:/path/to/one/liblibrary1.a
#cgo LDFLAGS:/path/to/two/liblibrary2.a
etc.

This is of course disallowed now. A workaround:

#cgo LDFLAGS:-L/path/to/one -llibrary1
#cgo LDFLAGS:-L/path/to/two -llibrary2

However some of these directories contain dynamic libraries too, which confuses the linker. Another option is to add '/' to the list of allowed "names" in https://go-review.googlesource.com/c/go/+/92855 In particular change on line 91 is:

re(`[a-zA-Z0-9_\/].*\.(o|obj|dll|dylib|so|a)`), // direct linker inputs: x.o or libfoo.so (but not -foo.o or @foo.o)

the latter option fixes our issue but I can't speak to its impact on security.

@andlabs

This comment has been minimized.

Contributor

andlabs commented Feb 8, 2018

@mirtchovski there's a patch for that (the issue is that .a was not whitelisted, but other object file formats were)

@mirtchovski

This comment has been minimized.

Contributor

mirtchovski commented Feb 8, 2018

.a is now whitelisted (after that patch) so 'libsomething.a' will work, but '/path/to/libsomething.a' will not work.

@jeddenlea

This comment has been minimized.

Contributor

jeddenlea commented Feb 8, 2018

@ianlancetaylor @rgburke I actually ran into the very same problem with --static which lead me down the hole to #23737. --static is rejected because it doesn't appear to be a valid package name before attempting to execute pkg-config, here

if !load.SafeArg(pkg) {
return nil, nil, fmt.Errorf("invalid pkg-config package name: %s", pkg)
.

Our quick fix internally was to point PKG_CONFIG to a script which simply executed pkg-config --static "$@".

@rgburke

This comment has been minimized.

rgburke commented Feb 8, 2018

@jeddenlea - Thank you very much! I was trying to find a workaround but didn't think of that.

@bobrik

This comment has been minimized.

bobrik commented Feb 9, 2018

We hit this with -msse and -msse4.2.

@minux

This comment has been minimized.

Member

minux commented Feb 9, 2018

@peterebden

This comment has been minimized.

peterebden commented Feb 9, 2018

We've hit this with --std=c99

@cbgo

This comment has been minimized.

cbgo commented Apr 27, 2018

The official SAP HANA client driver (the cgo based shared library) comes with #cgo LDFLAGS: -Wl,-rpath -Wl,\$ORIGIN which results in go build SAP/go-hdb/driver: invalid flag in #cgo LDFLAGS: -Wl,-rpath. Also see SAPs documentation at https://help.sap.com/viewer/0eec0d68141541d1b07893a39944924e/2.0.03/en-US/fba20e31f75c4f7ca5629083869069e5.html?q=golang%20driver for details.

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Apr 27, 2018

@cbgo see #23672

Code that passes a flag and argument pair to the linker must now use one -Wl flag instead of two: use #cgo LDFLAGS: -Wl,-rpath,$ORIGIN, not #cgo LDFLAGS: -Wl,-rpath -Wl,$ORIGIN.

@cbgo

This comment has been minimized.

cbgo commented Apr 29, 2018

@AlexRouSg thanks a lot. I should have read that more carefully :-)

@zcm211

This comment has been minimized.

zcm211 commented May 7, 2018

@PassKit Did you solve this problem?

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented May 7, 2018

@zcm211 If you're talking about https://github.com/miekg/pkcs11 they removed the -I... linker flag in feb. If you're using a package that uses it then you have to set the environmental variable CGO_LDFLAGS_ALLOW="-I.*"

@glycerine

This comment has been minimized.

glycerine commented May 8, 2018

darwin 64-bit, go1.10.2, I thought .o files were whitelisted for LDFLAGs, but I'm getting:

jaten@jatens-MacBook-Pro ~/go/src/github.com/go-interpreter/chezgo (master) $ make run                           
cd chez_scheme_9.5.1/c; make                                                                                     
make[1]: Nothing to be done for `doit'.                                                                          
go build && ./chezgo                                                                                             
go build github.com/go-interpreter/chezgo: invalid flag in #cgo LDFLAGS: ./chez_scheme_9.5.1/boot/a6osx/kernel.o 
make: *** [run] Error 1                                                                                          
jaten@jatens-MacBook-Pro ~/go/src/github.com/go-interpreter/chezgo (master) $ go version                         
go version go1.10.2 darwin/amd64                                                                                 
jaten@jatens-MacBook-Pro ~/go/src/github.com/go-interpreter/chezgo (master) $  

due to the cgo preamble in https://github.com/go-interpreter/chezgo/blob/master/embed.go#L10

#cgo LDFLAGS:  -liconv -lm -lncurses -L/usr/local/lib ./chez_scheme_9.5.1/boot/a6osx/kernel.o

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented May 8, 2018

@glycerine According to https://go-review.googlesource.com/c/go/+/94676 they are. Maybe try a full path instead of a relative one?

@glycerine

This comment has been minimized.

glycerine commented May 8, 2018

Good catch @AlexRouSg, the accept regex is buggy as you point out;

re(`[a-zA-Z0-9_/].*\.(a|o|obj|dll|dylib|so)`), // direct linker inputs: x.o or libfoo.so (but not -foo.o or @foo.o)

src/cmd/go/internal/work/security.go#121 should allow .o files to start with a . to support relative paths.

After all, I can't predict the users' GOPATH, or how nested the location of that file will become, so setting an absolute path isn't practical.

@andlabs

This comment has been minimized.

Contributor

andlabs commented May 8, 2018

Is the .o file in the source directory? If so, referring to the source directory is what ${SRCDIR} was provided for. (I forget specifically why this was introduced, but it was not because of this issue.)

@glycerine glycerine referenced this issue May 8, 2018

Open

build issue #1

@glycerine

This comment has been minimized.

glycerine commented May 8, 2018

@andlabs Even if there are klunky workarounds, relative paths should be linkable, and that is clearly a bug.

@andlabs

This comment has been minimized.

Contributor

andlabs commented May 8, 2018

It is, except IIRC there is no guarantee that the link would be relative TO the source directory (it could very well be in $WORK, and then your relative path breaks again)... Again, I've forgotten the history; someone else will need to explain.

@reusee

This comment has been minimized.

reusee commented May 11, 2018

gtk4 uses -mfpmath=sse

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented May 22, 2018

@ianlancetaylor Instead of having separate whitelists for cflags and ldflags, should there just be one list? gcc/llvm doesn't seem to care that you mix ldflags into cflags and vice versa. And it seems sometimes C devs do that causing issues like #25493 or #23749 (comment)

@jmhodges

This comment has been minimized.

Contributor

jmhodges commented May 22, 2018

Ah, I see we have a ticket already, so let me mention "-D_THREAD_SAFE" from protobuf as documented in #25493

@gopherbot

This comment has been minimized.

gopherbot commented May 30, 2018

Change https://golang.org/cl/115415 mentions this issue: cmd/go: accept more safe CFLAGS/LDFLAGS

@gopherbot gopherbot closed this in cc6e568 May 30, 2018

@gopherbot

This comment has been minimized.

gopherbot commented May 30, 2018

Change https://golang.org/cl/115435 mentions this issue: [release-branch.go1.10] cmd/go: accept more safe CFLAGS/LDFLAGS

@gopherbot

This comment has been minimized.

gopherbot commented May 30, 2018

Change https://golang.org/cl/115436 mentions this issue: [release-branch.go1.9] cmd/go: accept more safe CFLAGS/LDFLAGS

gopherbot pushed a commit that referenced this issue May 31, 2018

[release-branch.go1.10] cmd/go: accept more safe CFLAGS/LDFLAGS
Fixes #23749
Fixes #24703
Fixes #24858

Change-Id: Ib32d8efee294004c70fdd602087df2da0867f099
Reviewed-on: https://go-review.googlesource.com/115415
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 886d5601a63bef0271e705cfa6b6ac6f5134ee60)
Reviewed-on: https://go-review.googlesource.com/115435
Reviewed-by: Andrew Bonventre <andybons@golang.org>

gopherbot pushed a commit that referenced this issue May 31, 2018

[release-branch.go1.9] cmd/go: accept more safe CFLAGS/LDFLAGS
Fixes #23749
Fixes #24703
Fixes #24858

Change-Id: Ib32d8efee294004c70fdd602087df2da0867f099
Reviewed-on: https://go-review.googlesource.com/115415
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit cc6e568)
Reviewed-on: https://go-review.googlesource.com/115436
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@tssajo

This comment has been minimized.

tssajo commented Sep 4, 2018

Hey Guys,
Why is this issue closed when we still cannot build bimg with the latest version of Go as of 9/4/2018 ?
Please see issue: h2non/bimg#230
We cannot build anything that imports bimg since Go 1.10 and the problem still persist in Go 1.11
The error message we get is this:

# go build
go build gopkg.in/h2non/bimg.v1: invalid flag in pkg-config --libs: -Wl,--export-dynamic

Any advise?

EDIT:
I created a new issue: #27496

@alexflint

This comment has been minimized.

alexflint commented Sep 23, 2018

I believe this whitelist is responsible for the following: alexflint/gallium#63

@AlexRouSg

This comment has been minimized.

Contributor

AlexRouSg commented Sep 23, 2018

@alexflint This issue is closed, please create a new one

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