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

engine: Build with non-executable stacks #37939

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
3 participants
@bdarnell
Copy link
Member

commented May 30, 2019

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.

@bdarnell bdarnell requested a review from petermattis May 30, 2019

@bdarnell bdarnell requested a review from cockroachdb/core-prs as a code owner May 30, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 30, 2019

This change is Reviewable

@bdarnell bdarnell force-pushed the bdarnell:exec-stack branch from 86759cb to a07ce84 May 30, 2019

@petermattis
Copy link
Contributor

left a comment

The use of go:linkname can go away when we upgrade to go 1.12. The associated perf hack is no longer needed as the problem was fixed upstream.

// runBuildAnalyze performs static analysis on the built binary to
// ensure it's built as expected.
func runBuildAnalyze(ctx context.Context, t *test, c *cluster) {

This comment has been minimized.

Copy link
@petermattis

petermattis May 30, 2019

Contributor

Spurious blank line.

// contain assembly files; this is the reason this file exists.
//
// 2. Completely empty assembly files cause GCC to mark the binary
// as requiring an executable stack. This is a security risk. The

This comment has been minimized.

Copy link
@petermattis

petermattis May 30, 2019

Contributor

Why?!?

This comment has been minimized.

Copy link
@bdarnell

bdarnell May 31, 2019

Author Member

I'll rephrase this: it's nothing special about empty files, it's that any assembly file that doesn't have this marker is assumed to contain code that might rely on an executable stack. Empty files are a degenerate case that could be understood to not need executable stack, but that's not something that anyone really considered since there's not normally a reason for an empty file to exist.

This comment has been minimized.

Copy link
@petermattis

petermattis May 31, 2019

Contributor

Ok, though I imagine empty assembly files are special in some regard since both the Go runtime and some third-party libraries we use such as petermattis/goid have non-empty assembly files which don't seem to cause a problem. My "Why?!?" comment was more about why GCC had such a subtle security behavior.

This comment has been minimized.

Copy link
@bdarnell

bdarnell May 31, 2019

Author Member

The .s files used in goid are only used on older go versions. But there are other .s files that I'm pretty sure we do use that aren't having this effect. Mysterious.

Fun fact: pkg/sql/exec has its own empty assembly file, which evidently does not cause the executable to get the execstack bit. There is no import "C" in the package, so the .s file isn't even getting compiled. In current versions of go, it's not required to have a magic empty assembly file, it's required to import unsafe to use go:linkname. The "empty assembly file" pattern appears to be meaningless (I looked through blame as far as I could easily go and the logic about importing unsafe has been there since at least 2016). So I'm just going to delete both of these assembly files and keep the new test.

@bdarnell bdarnell requested a review from cockroachdb/sql-rest-prs as a code owner May 31, 2019

@petermattis
Copy link
Contributor

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)


pkg/storage/engine/slice.s, line 7 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The .s files used in goid are only used on older go versions. But there are other .s files that I'm pretty sure we do use that aren't having this effect. Mysterious.

Fun fact: pkg/sql/exec has its own empty assembly file, which evidently does not cause the executable to get the execstack bit. There is no import "C" in the package, so the .s file isn't even getting compiled. In current versions of go, it's not required to have a magic empty assembly file, it's required to import unsafe to use go:linkname. The "empty assembly file" pattern appears to be meaningless (I looked through blame as far as I could easily go and the logic about importing unsafe has been there since at least 2016). So I'm just going to delete both of these assembly files and keep the new test.

Huh, I could have sworn the empty .s file did something. Well, I trust your research and everything compiles and links, this is certainly the best approach.

@bdarnell

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Ah, the empty assembly files are needed not to enable go:linkname per se, but to allow you to declare a function with no body. And this changed in go 1.12 (which I have locally), so that's why I though they weren't needed. See golang/go#23311

Putting the empty files back for now.

@bdarnell bdarnell force-pushed the bdarnell:exec-stack branch 2 times, most recently from cb6922d to a7442c1 Jun 3, 2019

@bdarnell

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Hmm, the two empty assembly files are getting built differently. The one in storage/engine gets run through the preprocessor and the three-line magic works (without it, the binary gets an executable stack). The one in sql/exec does not go through the preprocessor so the three-line magic fails to compile (I didn't try the one-line form without the ifdef). But without the magic line, the binary still doesn't get an executable stack.

I'll stop shaving this yak now (assuming CI finally passes), and leave it with a comment-only assembly file in sql/exec and one with the magic lines in storage/engine. Both of them should be removed when we require go 1.12.

@petermattis
Copy link
Contributor

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)


pkg/storage/engine/slice.s, line 1 at r2 (raw file):

// This empty assembly file has non-obvious side effects.

No copyright header?

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 #37885

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

@bdarnell bdarnell force-pushed the bdarnell:exec-stack branch from a7442c1 to 52d048d Jun 3, 2019

@bdarnell

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

No copyright header?

Whoops :)

@petermattis
Copy link
Contributor

left a comment

No copyright header?

Whoops :)

Heh, your left hand didn't know what your right hand was doing.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)

@bdarnell

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

bors r=petermattis

craig bot pushed a commit that referenced this pull request 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

This comment has been minimized.

Copy link

commented Jun 3, 2019

Build succeeded

@craig craig bot merged commit 52d048d into cockroachdb:master Jun 3, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bdarnell bdarnell deleted the bdarnell:exec-stack branch Jun 3, 2019

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.