From 3cab7b9276042c142f266dfa38d90772709f63b7 Mon Sep 17 00:00:00 2001
From: Oliver Newman <olivernewman@google.com>
Date: Tue, 3 Oct 2023 21:25:11 +0000
Subject: [PATCH] [engine] Don't try to mount invalid PATH elements

Skip trying to mount any elements of $PATH that are not valid absolute
paths. This makes shac resilient to weirdly/improperly configured
environments.

Change-Id: Ib6ba47798433c68c1b19e07e2afc49e172f4ee98
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/926132
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Reviewed-by: Anthony Fandrianto <atyfto@google.com>
---
 internal/engine/run_test.go       | 30 ++++++++++++++++++++++++++++++
 internal/engine/runtime_ctx_os.go |  6 ++++++
 2 files changed, 36 insertions(+)

diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go
index a543d53..4f20284 100644
--- a/internal/engine/run_test.go
+++ b/internal/engine/run_test.go
@@ -635,6 +635,36 @@ func TestRun_PassthroughEnv(t *testing.T) {
 	}
 }
 
+func TestRun_Exec_InvalidPATHElements(t *testing.T) {
+	root := t.TempDir()
+
+	writeFile(t, root, "file.txt", "")
+
+	invalidPathElements := []string{
+		filepath.Join(t.TempDir(), "does-not-exist"),
+		"relative/path",
+		"./dot/relative/path",
+		filepath.Join(root, "file.txt"),
+	}
+
+	t.Setenv("PATH", strings.Join(
+		append(invalidPathElements, os.Getenv("PATH")),
+		string(os.PathListSeparator)))
+
+	// No need for a complicated test case, just make sure that the subprocess
+	// launching succeeds. If shac doesn't handle invalid PATH elements
+	// correctly, launching any subprocess should fail, at least on platforms
+	// where filesystem sandboxing is supported.
+	writeFile(t, root, "shac.star", ""+
+		"def cb(ctx):\n"+
+		"    ctx.os.exec([\"true\"]).wait()\n"+
+		"    print(\"success!\")\n"+
+		"shac.register_check(cb)\n")
+
+	want := "[//shac.star:3] success!\n"
+	testStarlarkPrint(t, root, "shac.star", false, false, want)
+}
+
 func TestRun_SCM_Raw(t *testing.T) {
 	t.Parallel()
 	root := t.TempDir()
diff --git a/internal/engine/runtime_ctx_os.go b/internal/engine/runtime_ctx_os.go
index 977cf85..12e0818 100644
--- a/internal/engine/runtime_ctx_os.go
+++ b/internal/engine/runtime_ctx_os.go
@@ -371,8 +371,14 @@ func ctxOsExec(ctx context.Context, s *shacState, name string, args starlark.Tup
 		// Mount all directories listed in $PATH.
 		for _, p := range strings.Split(env["PATH"], string(os.PathListSeparator)) {
 			// $PATH may contain invalid elements. Filter them out.
+			if !filepath.IsAbs(p) {
+				// Relative paths in $PATH are not allowed.
+				continue
+			}
 			var fi os.FileInfo
 			if fi, err = os.Stat(p); err != nil || !fi.IsDir() {
+				// Skip $PATH elements that don't exist or point to
+				// non-directories.
 				continue
 			}
 			config.Mounts = append(config.Mounts, sandbox.Mount{Path: p})