From 63ea1f966e4969ead5fe6b90f2fd18ad48ddc9d5 Mon Sep 17 00:00:00 2001
From: Cole Faust <colefaust@google.com>
Date: Tue, 27 Aug 2024 11:42:26 -0700
Subject: [PATCH] Sandbox environment variables

So that the build can't access extra information unintentionally.
Particuarly ANDROID_BUILD_TOP is dangerous.

In the future PATH should be locked down as well.

Bug: 307824623
Test: Added a all_genrules target and built that
Change-Id: I88bb0efb0a82529a1c85875a53cf20c8384d07fe
---
 android/rule_builder.go | 50 ++++++++++++++++++++++++++++++++---------
 cmd/sbox/sbox.go        |  5 ++++-
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/android/rule_builder.go b/android/rule_builder.go
index 95e2b92f6..18bbcab5c 100644
--- a/android/rule_builder.go
+++ b/android/rule_builder.go
@@ -463,6 +463,8 @@ func (r *RuleBuilder) Build(name string, desc string) {
 	r.build(name, desc, true)
 }
 
+var sandboxEnvOnceKey = NewOnceKey("sandbox_environment_variables")
+
 func (r *RuleBuilder) build(name string, desc string, ninjaEscapeCommandString bool) {
 	name = ninjaNameEscape(name)
 
@@ -580,16 +582,44 @@ func (r *RuleBuilder) build(name string, desc string, ninjaEscapeCommandString b
 				})
 			}
 
-			// Set OUT_DIR to the relative path of the sandboxed out directory.
-			// Otherwise, OUT_DIR will be inherited from the rest of the build,
-			// which will allow scripts to escape the sandbox if OUT_DIR is an
-			// absolute path.
-			command.Env = append(command.Env, &sbox_proto.EnvironmentVariable{
-				Name: proto.String("OUT_DIR"),
-				State: &sbox_proto.EnvironmentVariable_Value{
-					Value: sboxOutSubDir,
-				},
-			})
+			// Only allow the build to access certain environment variables
+			command.DontInheritEnv = proto.Bool(true)
+			command.Env = r.ctx.Config().Once(sandboxEnvOnceKey, func() interface{} {
+				// The list of allowed variables was found by running builds of all
+				// genrules and seeing what failed
+				var result []*sbox_proto.EnvironmentVariable
+				inheritedVars := []string{
+					"PATH",
+					"JAVA_HOME",
+					"TMPDIR",
+					// Allow RBE variables because the art tests invoke RBE manually
+					"RBE_log_dir",
+					"RBE_platform",
+					"RBE_server_address",
+					// TODO: RBE_exec_root is set to the absolute path to the root of the source
+					// tree, which we don't want sandboxed actions to find. Remap it to ".".
+					"RBE_exec_root",
+				}
+				for _, v := range inheritedVars {
+					result = append(result, &sbox_proto.EnvironmentVariable{
+						Name: proto.String(v),
+						State: &sbox_proto.EnvironmentVariable_Inherit{
+							Inherit: true,
+						},
+					})
+				}
+				// Set OUT_DIR to the relative path of the sandboxed out directory.
+				// Otherwise, OUT_DIR will be inherited from the rest of the build,
+				// which will allow scripts to escape the sandbox if OUT_DIR is an
+				// absolute path.
+				result = append(result, &sbox_proto.EnvironmentVariable{
+					Name: proto.String("OUT_DIR"),
+					State: &sbox_proto.EnvironmentVariable_Value{
+						Value: sboxOutSubDir,
+					},
+				})
+				return result
+			}).([]*sbox_proto.EnvironmentVariable)
 			command.Chdir = proto.Bool(true)
 		}
 
diff --git a/cmd/sbox/sbox.go b/cmd/sbox/sbox.go
index 6459ea175..f3931a45e 100644
--- a/cmd/sbox/sbox.go
+++ b/cmd/sbox/sbox.go
@@ -275,7 +275,10 @@ func createEnv(command *sbox_proto.Command) ([]string, error) {
 			if !state.Inherit {
 				return nil, fmt.Errorf("Can't have inherit set to false")
 			}
-			env = append(env, *envVar.Name+"="+os.Getenv(*envVar.Name))
+			val, ok := os.LookupEnv(*envVar.Name)
+			if ok {
+				env = append(env, *envVar.Name+"="+val)
+			}
 		default:
 			return nil, fmt.Errorf("Unhandled state type")
 		}