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

security: Build with non-executable stacks #37885

Closed
bdarnell opened this issue May 28, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@bdarnell
Copy link
Member

commented May 28, 2019

Cockroach currently fails on SELinux (with default configurations) because it attempts to mmap a region of memory with PROT_WRITE and PROT_EXEC. Preliminary investigation shows that this is memory used for the stack.

According to this page (from 2011, don't know if it's still accurate), the executable bit for the stack is controlled by a link-time option. Somewhere in our build toolchain, that's not getting set correctly.

@bdarnell

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

This was a fun/frustrating one to chase down.

History: When the ability to mark stack memory as non-executable was introduced, GCC adopted it conservatively. Each object file must be marked as OK for use with a non-executable stack, or else the entire binary would be marked as requiring an executable stack. (This is a case where the default should really be flipped, but there's a lot of inertia for these things).

Most compilers now set the "needs executable stack" bit on their object files automatically (usually to false, but certain dynamic features might cause it to be set to true). The major exception is assembly: any .s file that compiles to a separate object file needs the right magic incantation to set this flag, or else it will default to true. (Or a link-time option can be used to override this. The link-time option is somewhat unsafe since it will cause crashes if your program really does need an executable stack)

How was cockroach getting marked as needing an executable stack? The gold linker supports a --warn-execstack option, but I'm not sure how to make that happen with our build system. Instead, I ran scanelf -qeR ~/go/native ~/.ccache, which found a single object file with the execstack bit. This file looked empty as far as I could tell. Where was it coming from? By setting the CCACHE_LOGFILE environment variable and rebuilding, it told me that it accessed this file while building slice.s.

The culprit turns out to be the empty file introduced in #22244 (cc @petermattis). The presence of this assembly file, in addition to allowing us to perform unseemly hacks with the go:linkname directive, had the distant side effect of setting the execstack bit on our executable, a potential security vulnerability.

There's a simple fix, to add the .section .note.GNU-stack,"",@progbits magic to this "empty" assembly file. I'm not sure whether this has any downsides for portability to non-gnu systems.

bdarnell added a commit to bdarnell/cockroach that referenced this issue May 30, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.
@jordanlewis

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Ouch. This must have been a pain to understand and track down.

bdarnell added a commit to bdarnell/cockroach that referenced this issue May 30, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.

bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 3, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.

bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 3, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.

bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 3, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.

craig bot pushed a commit that referenced this issue Jun 3, 2019

Merge #37939
37939: engine: Build with non-executable stacks r=petermattis a=bdarnell

Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes #37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.

Co-authored-by: Ben Darnell <ben@bendarnell.com>

@craig craig bot closed this in #37939 Jun 3, 2019

bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 4, 2019

engine: Build with non-executable stacks
Three lines of inscrutable magic (and plenty of comments) to avoid
tripping a complex series of heuristics resulting in a security risk.

Fixes cockroachdb#37885

Release note (security improvement): Stack memory used by CockroachDB
is now marked as non-executable, improving security and compatibility
with SELinux.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.