Skip to content

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.

env: CC="gcc-5" CXX="g++-5" OS="linux"
env: CC="gcc-5" CXX="g++-5" OS="linux" CGO_LDFLAGS_ALLOW="-z|noexecstack"
- 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?

/*
#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"?

#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