-
Notifications
You must be signed in to change notification settings - Fork 76
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
Make stack non-executable #105
base: master
Are you sure you want to change the base?
Conversation
Fixes a bug where go programs that rely on this library have executable stacks. In order to build this library with these ld flags, the environment variable CGO_LDFLAGS_ALLOW must be set to a regex that will accept the -z flag. The value "-z|noexecstack" is sufficient. Otherwise the build will fail with the message "invalid flag in #cgo LDFLAGS". This is due to the whitelisting of allowed flags for security purposes.
We're going to have to set |
In order to build go-webrtc with a nonexecutable stack, we need the the LD flag `-z noexecstack`. However, LD flags are whitelisted in Go for security reasons. We need to supply an environment variable with a regex that allows this flag.
Hrm, okay this actually just seems like a linux problem. I'm going to make a go-webrtc patch for linux for the rbm Tor Browser builds that patches this issue. However, I think that the executable stack problem in linux is more generally worrying. Any go program that uses this library will have an executable stack, which is something we want to fix. |
Okay, updated the CGO directives to be platform specific, and CI passes now. |
- os: osx | ||
env: CGO_LDFLAGS_ALLOW="-z|noexecstack" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this doing anything now?
@@ -8,7 +8,8 @@ package webrtc | |||
|
|||
/* | |||
#cgo CXXFLAGS: -std=c++0x | |||
#cgo LDFLAGS: -L${SRCDIR}/lib | |||
#cgo linux LDFLAGS: -L${SRCDIR}/lib -z noexecstack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "android"?
@@ -31,7 +31,8 @@ package webrtc | |||
|
|||
/* | |||
#cgo CXXFLAGS: -std=c++0x | |||
#cgo LDFLAGS: -L${SRCDIR}/lib | |||
#cgo linux LDFLAGS: -L${SRCDIR}/lib -z noexecstack | |||
#cgo darwin LDFLAGS: -L${SRCDIR}/lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not necessarily opposed to these changes but do these flags need to be set at the source level?
Would something like this,
CGO_LDFLAGS_ALLOW="-z|noexecstack" go build -ldflags '-z noexecstack'
work here?
https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/snowflake/build#n46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, even simpler, you can set
export CGO_LDFLAGS="-z noexecstack"
I think we should do this for the Tor Browser build.
It worries me that the stack is executable by default, but this is probably something that should be fixed with the go linker rather than here. The advantage to doing everything in environment variables or with -ldflags
as you suggested is you don't need to remember to set things in more than one place.
I'm definitely good if the decision is to not merge this pull request.
Maybe the way to handle this is to get |
Fixes a bug where go programs that rely on this library have executable
stacks.
In order to build this library with these ld flags, the environment
variable CGO_LDFLAGS_ALLOW must be set to a regex that will accept the
-z flag. The value "-z|noexecstack" is sufficient. Otherwise the build
will fail with the message "invalid flag in #cgo LDFLAGS". This is due
to the whitelisting of allowed flags for security purposes.