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

Make stack non-executable #105

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make stack non-executable #105

wants to merge 4 commits into from

Conversation

cohosh
Copy link

@cohosh cohosh commented May 13, 2019

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.

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.
@cohosh
Copy link
Author

cohosh commented May 13, 2019

@cohosh
Copy link
Author

cohosh commented May 14, 2019

We're going to have to set CGO_LDFLAGS_ALLOW="-z|noexecstack" for CI to pass.

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.
@cohosh
Copy link
Author

cohosh commented May 14, 2019

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.

@cohosh
Copy link
Author

cohosh commented May 21, 2019

Okay, updated the CGO directives to be platform specific, and CI passes now.
I talked to GeKo and they said they'd prefer this to be fixed upstream rather than handled with a patch in tor-browser-build.

- os: osx
env: CGO_LDFLAGS_ALLOW="-z|noexecstack"
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Author

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.

@uumaro
Copy link
Collaborator

uumaro commented May 28, 2019

Maybe the way to handle this is to get CGO_LDFLAGS_ALLOW and CGO_LDFLAGS in build.sh, and rebuild the precompiled libraries? I imagine that's what most downstream users other than Tor Browser are using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants