Merge pull request #38011 from bdarnell/backport19.1-37939

release-19.1: engine: Build with non-executable stacks

pkg/cmd/roachtest/acceptance.go

@@ -40,6 +40,7 @@ func registerAcceptance(r *registry) {
 		},
 		// {"bank/zerosum-restart", runBankZeroSumRestart},
 		{name: "build-info", fn: runBuildInfo},
+		{name: "build-analyze", fn: runBuildAnalyze},
 		{name: "cli/node-status", fn: runCLINodeStatus},
 		{name: "decommission", fn: runDecommissionAcceptance},
 		{name: "cluster-init", fn: runClusterInit},

pkg/cmd/roachtest/build_info.go

@@ -18,6 +18,7 @@ package main
 import (
 	"context"
 	"net/http"
+	"os/exec"
 
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/util/httputil"
@@ -47,3 +48,50 @@ func runBuildInfo(ctx context.Context, t *test, c *cluster) {
 		}
 	}
 }
+
+// runBuildAnalyze performs static analysis on the built binary to
+// ensure it's built as expected.
+func runBuildAnalyze(ctx context.Context, t *test, c *cluster) {
+
+	if c.isLocal() {
+		// This test is linux-specific and needs to be able to install apt
+		// packages, so only run it on dedicated remote VMs.
+		t.spec.Skip = "local execution not supported"
+		return
+	}
+
+	c.Put(ctx, cockroach, "./cockroach")
+
+	// 1. Check for executable stack.
+	//
+	// Executable stack memory is a security risk (not a vulnerability
+	// in itself, but makes it easier to exploit other vulnerabilities).
+	// Whether or not the stack is executable is a property of the built
+	// executable, subject to some subtle heuristics. This test ensures
+	// that we're not hitting anything that causes our stacks to become
+	// executable.
+	//
+	// References:
+	// https://www.airs.com/blog/archives/518
+	// https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
+	// https://github.com/cockroachdb/cockroach/issues/37885
+
+	// There are several ways to do this analysis: `readelf -lW`,
+	// `scanelf -qe`, and `execstack -q`. `readelf` is part of binutils,
+	// so it's relatively ubiquitous, but we don't have it in the
+	// roachtest environment. Since we don't have anything preinstalled
+	// we can use, choose `scanelf` for being the simplest to use (empty
+	// output indicates everything's fine, non-empty means something
+	// bad).
+	c.Run(ctx, c.Node(1), "sudo apt-get update")
+	c.Run(ctx, c.Node(1), "sudo apt-get -qqy install pax-utils")
+
+	cmd := exec.CommandContext(ctx, roachprod, "run", c.makeNodes(c.Node(1)), "scanelf -qe cockroach")
+	output, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("scanelf failed: %s", err)
+	}
+	if len(output) > 0 {
+		t.Fatalf("scanelf returned non-empty output (executable stack): %s", string(output))
+	}
+}

pkg/cmd/roachtest/jepsen.go

@@ -80,10 +80,12 @@ func initJepsen(ctx context.Context, t *test, c *cluster) {
 	// depending on whether the test passed or not.
 	c.Run(ctx, c.All(), "mkdir", "-p", "logs")
 
-	// TODO(bdarnell): Does this blanket apt update matter? I just
-	// copied it from the old jepsen scripts. It's slow, so we should
-	// probably either remove it or use a new base image with more of
-	// these preinstalled.
+	// `apt-get update` is slow but necessary: the base image has
+	// outdated information and refers to package versions that are no
+	// longer retrievable.
+	//
+	// TODO(bdarnell): Create a new base image with the packages we need
+	// instead of installing them on every run.
 	c.Run(ctx, c.All(), "sh", "-c", `"sudo apt-get -y update > logs/apt-upgrade.log 2>&1"`)
 	c.Run(ctx, c.All(), "sh", "-c", `"sudo apt-get -y upgrade -o Dpkg::Options::='--force-confold' > logs/apt-upgrade.log 2>&1"`)
 

pkg/sql/exec/noescape.s

@@ -1 +1,41 @@
-// Empty assembly file to allow go:linkname to work.
+// Copyright 2018 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License. See the AUTHORS file
+// for names of contributors.
+
+// This empty assembly file has non-obvious side effects.
+//
+// 1. In go 1.11, the existence of an assembly file makes it
+// possible to declare a function with no body. This in turn is
+// needed to use the go:linkname directive to refer to functions
+// from another package. This is the reason this file exists and
+// it should go away when we require go 1.12.
+//
+// 2. Assembly files may cause GCC to mark the binary
+// as requiring an executable stack. This is a security risk. The
+// magic below instructs GCC to keep the stack non-executable.
+//
+// For reasons that are not understood, point 2 only applies in
+// some packages (I think it's related to whether cgo is also used
+// in the package). In packages where this is not true, the
+// .s file is not run through the preprocessor, so we can't
+// use ifdef guards. Since it doesn't appear to matter, we
+// don't use the magic at all in those cases.
+//
+// References:
+// https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
+// https://github.com/cockroachdb/cockroach/issues/37885
+
+// #if defined(__linux__) && defined(__ELF__)
+// .section        .note.GNU-stack, "", %progbits
+// #endif

pkg/storage/engine/slice_magic.S

@@ -0,0 +1,41 @@
+// Copyright 2018 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License. See the AUTHORS file
+// for names of contributors.
+
+// This empty assembly file has non-obvious side effects.
+//
+// 1. In go 1.11, the existence of an assembly file makes it
+// possible to declare a function with no body. This in turn is
+// needed to use the go:linkname directive to refer to functions
+// from another package. This is the reason this file exists and
+// it should go away when we require go 1.12.
+//
+// 2. Assembly files may cause GCC to mark the binary
+// as requiring an executable stack. This is a security risk. The
+// magic below instructs GCC to keep the stack non-executable.
+//
+// For reasons that are not understood, point 2 only applies in
+// some packages (I think it's related to whether cgo is also used
+// in the package). In packages where this is not true, the
+// .s file is not run through the preprocessor, so we can't
+// use ifdef guards. Since it doesn't appear to matter, we
+// don't use the magic at all in those cases.
+//
+// References:
+// https://wiki.ubuntu.com/SecurityTeam/Roadmap/ExecutableStacks
+// https://github.com/cockroachdb/cockroach/issues/37885
+
+#if defined(__linux__) && defined(__ELF__)
+.section        .note.GNU-stack, "", %progbits
+#endif