engine: Build with non-executable stacks#37939
Conversation
|
This change is |
petermattis
left a comment
There was a problem hiding this 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) { | ||
|
|
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
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
.sfiles used ingoidare only used on older go versions. But there are other.sfiles that I'm pretty sure we do use that aren't having this effect. Mysterious.Fun fact:
pkg/sql/exechas its own empty assembly file, which evidently does not cause the executable to get the execstack bit. There is noimport "C"in the package, so the.sfile 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 usego: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.
|
Ah, the empty assembly files are needed not to enable Putting the empty files back for now. |
cb6922d to
a7442c1
Compare
|
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
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/engine/slice.s, line 1 at r2 (raw file):
// This empty assembly file has non-obvious side effects.
No copyright header?
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.
Whoops :) |
petermattis
left a comment
There was a problem hiding this comment.
No copyright header?
Whoops :)
Heh, your left hand didn't know what your right hand was doing.
Reviewable status:
|
bors r=petermattis |
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>
Build succeeded |
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.