Skip to content
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: enable ASLR on Windows -buildmode=c-shared #41421

Closed
qmuntal opened this issue Sep 16, 2020 · 16 comments
Closed

cmd/go: enable ASLR on Windows -buildmode=c-shared #41421

qmuntal opened this issue Sep 16, 2020 · 16 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Sep 16, 2020

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

go version go1.15.1 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\muntal\AppData\Local\go-build
set GOENV=C:\Users\muntal\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\muntal\go\pkg\mod
set GONOPROXY=*.hp.com
set GONOSUMDB=*.hp.com
set GOOS=windows
set GOPATH=C:\Users\muntal\go
set GOPRIVATE=*.hp.com
set GOPROXY=direct
set GOROOT=c:\go
set GOSUMDB=off
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\muntal\AppData\Local\Temp\go-build392009438=/tmp/go-build -gno-record-gcc-switches

What did you do?

go build -buildmode=c-shared -o libfoo.dll .
dumpbin /headers libfoo.dll

What did you expect to see?

...
OPTIONAL HEADER VALUES
...
            8160 DLL characteristics
                   High Entropy Virtual Addresses
                   Dynamic base
                   NX compatible
                   Terminal Server Aware
...

I expected the dll to have IMAGE_DLLCHARACTERISTICS_DYNAMIC_BASE and IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA flags set just as if I would be building a binary with -buildmode=pie (implemented in #35192).

What did you see instead?

...
OPTIONAL HEADER VALUES
...
            8100 DLL characteristics
                   NX compatible
                   Terminal Server Aware
...

Current implementation doesn't use ASLR nor High Entropy which causes go dll's to by flagged by cyber-security as insecure.

This is the follow up of golang-nuts/hVK37aucmco.
@alexbrainman

@jayconrod jayconrod added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 16, 2020
@jayconrod jayconrod added this to the Backlog milestone Sep 16, 2020
@gopherbot
Copy link

Change https://golang.org/cl/255259 mentions this issue: cmd/link: enable ASLR on windows binaries built with -buildmode=c-shared

@alexbrainman
Copy link
Member

@qmuntal thank you for your report and CL.

But CL 255259 makes every DLL use ASLR. What about, if some DLLs needs to be compiled the old way without ASLR? How would the developers build DLL without ASLR?

We already had reports that some packages are broken when building executables with Go 1.15. These developers just use -buildmode=exe to force no ASLR build. So this problem is real. We need something similar for DLLs before we can proceed with any fix here.

Alex

/cc @zx2c4

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 17, 2020

What about, if some DLLs needs to be compiled the old way without ASLR? How would the developers build DLL without ASLR?

Good point @alexbrainman , enabling ASLR on pie binaries could be justified because there is the exe workaround, but mapping optional linker flags to -buildmode does not scale nor make much sense.

One option could be to define an env var, p.e. GOLINK, that allow developers to opt-in or opt-out linker features, in a similar manner we already do on GOARM and GOWASM. The GOLINK values would be a predefined list of well known features, such as aslr or dep, that each GOOS/GOARCH, or even linker if #20982 finally lands, would implement as needed. In case of opt-out features one could just negate the feature name: GOLINK=aslr,-dep.

@alexbrainman
Copy link
Member

One option could be to define an env var, p.e. GOLINK, that allow developers to opt-in or opt-out linker features

I cannot make that decision. You need to take this proposal with Go Team.

Personally I think there are too many GO* variables already.

Alex

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 19, 2020

@alexbrainman I´ve updated the CL and removed the --export-all-symbols flag from -buildmode=c-shared as for DLLs relocs are not stripped thus there is no need for this hack.

With this change the only new flags added to -buildmode=c-shared are --dynamicbase and --high-entropy-va, which have been enabled by default on msvc since 2008, therefore I think it is safe to use them without having an opt-out mechanism, just as --nxcompat.

Also the issues I´ve seen related to go1.15 defaulting to -pie and ASLR seem to be related to being a PIE binary (#41329) or --export-all-symbols (#40795). Yet obviously the risk is there, so instead of defining a new GOLINK flag we could just gate aslr behind GODEBUG=aslroff=1.

@alexbrainman
Copy link
Member

With this change the only new flags added to -buildmode=c-shared are --dynamicbase and --high-entropy-va, which have been enabled by default on msvc since 2008, therefore I think it is safe to use them without having an opt-out mechanism, just as --nxcompat.

What happened to msvc in 2008 is not relevant here, because Go users don't use msvc, they use gcc. And most of these are very very out of date.

Yet obviously the risk is there, so instead of defining a new GOLINK flag we could just gate aslr behind GODEBUG=aslroff=1.

Not my call, sorry. Go team need to decide that is what they want to do. Cherry can, probably, make that call.

Alex

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 20, 2020

Thanks for the feedback and the review Alex. Lets see if @rsc or @cherrymui can add something to this issue.

@ianlancetaylor
Copy link
Contributor

Seems like we can already get a DSL with ASLR on Windows by running something like -buildmode=c-shared -extldflags=-Wl,--dynamicbase,--high-entropy-va. Or, if we make that the default, perhaps there is -extldflags option we can use to disable ALSR. Perhaps we just need to document that, rather than introduce a new build mode or a new environment variable.

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 21, 2020

Seems like we can already get a DSL with ASLR on Windows by running something like -buildmode=c-shared -extldflags=-Wl,--dynamicbase,--high-entropy-va

Good shoot, it does add ASLR and the required dllcharacteristics to the output DLL.

if we make that the default, perhaps there is -extldflags option we can use to disable ALSR. Perhaps we just need to document that, rather than introduce a new build mode or a new environment variable

Unfortunately mingw-w64 does not have an option to disable dynamicbase nor high-entropy-va. See Patch 1 of https://sourceware.org/bugzilla/show_bug.cgi?id=19011, it adds the necessary flags to disable them but it is still not merged.

@alexbrainman
Copy link
Member

Good shoot, it does add ASLR and the required dllcharacteristics to the output DLL.

Option 1: We could just leave things as they are and update the documentation on how to build ASLR DLL. It is only that the flags are bit complicated. And we also want ASLR to be the default.

Unfortunately mingw-w64 does not have an option to disable dynamicbase nor high-entropy-va. See Patch 1 of https://sourceware.org/bugzilla/show_bug.cgi?id=19011, it adds the necessary flags to disable them but it is still not merged.

That is unfortunate indeed. It would be nice, if we could make ASLR the default, and document how to disable the ASLR.

rather than introduce a new build mode or a new environment variable.

I don't want to introduce new build mode or a new environment variable too. But that only leaves us with Option 1. But maybe option 1 is good enough.

Alex

@ianlancetaylor
Copy link
Contributor

We could add a Go linker option that disables ASLR, by telling the linker to not add the options that are passed to the system linker. This would then be available using something like -buildmode=c-shared -ldflags=-no-aslr.

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 22, 2020

We could add a Go linker option that disables ASLR, by telling the linker to not add the options that are passed to the system linker. This would then be available using something like -buildmode=c-shared -ldflags=-no-aslr

Should -no-aslr flag also disable ASLR on -buildmode=pie? I see no reason why it shouldn't, but just double checking with you all.

@ianlancetaylor
Copy link
Contributor

I don't know what -buildmode=pie means if we disable ASLR. Is that exactly the same as -buildmode=exe? If so I would be inclined to make it an error. For a tool it seems confusing to have two different ways to do exactly the same thing.

@alexbrainman
Copy link
Member

I don't know what -buildmode=pie means if we disable ASLR. Is that exactly the same as -buildmode=exe? If so I would be inclined to make it an error. For a tool it seems confusing to have two different ways to do exactly the same thing.

I agree with Ian. Lets only allow -no-aslr flag together with -buildmode=c-shared. And make it error with any other buildmode value. Hopefully no one needs to use -no-aslr flag - -no-aslr flag should be an exception.

Alex

@qmuntal
Copy link
Contributor Author

qmuntal commented Sep 23, 2020

I'll use the positive flag -aslr instead of -no-aslr as per @cherrymui comment in Gerrit if this is also ok to you.

@alexbrainman
Copy link
Member

I'll use the positive flag -aslr instead of -no-aslr as per @cherrymui comment in Gerrit if this is also ok to you.

-aslr is fine with me too. As long as you set its default value correctly.

Thank you.

Alex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants