sandbox:

- add flag --sandbox_add_path, which takes a list of additional paths as argument and mount these paths to sandbox. Fixes #884.
- mount target of /etc/resolv.conf if it is a symlink. Fixes #738.

RELNOTES:
- add flag --sandbox_add_path, which takes a list of additional paths as argument and mount these paths to sandbox.
- mount target of /etc/resolv.conf if it is a symlink.

--
MOS_MIGRATED_REVID=117364211

6 files changed, 164 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
index 76e18e7e09..69ed5666a7 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.sandbox;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicates;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.Files;
@@ -74,6 +75,7 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
   private final boolean verboseFailures;
   private final boolean sandboxDebug;
   private final StandaloneSpawnStrategy standaloneStrategy;
+  private final List<String> sandboxAddPath;
   private final UUID uuid = UUID.randomUUID();
   private final AtomicInteger execCounter = new AtomicInteger();
 
@@ -82,13 +84,15 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
       BlazeDirectories blazeDirs,
       ExecutorService backgroundWorkers,
       boolean verboseFailures,
-      boolean sandboxDebug) {
+      boolean sandboxDebug,
+      List<String> sandboxAddPath) {
     this.clientEnv = ImmutableMap.copyOf(clientEnv);
     this.blazeDirs = blazeDirs;
     this.execRoot = blazeDirs.getExecRoot();
     this.backgroundWorkers = Preconditions.checkNotNull(backgroundWorkers);
     this.verboseFailures = verboseFailures;
     this.sandboxDebug = sandboxDebug;
+    this.sandboxAddPath = sandboxAddPath;
     this.standaloneStrategy = new StandaloneSpawnStrategy(blazeDirs.getExecRoot(), verboseFailures);
   }
 
@@ -221,6 +225,8 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
       throws IOException, ExecException {
     ImmutableMap.Builder<Path, Path> result = new ImmutableMap.Builder<>();
     result.putAll(mountUsualUnixDirs());
+    result.putAll(mountUserDefinedPath());
+
     MountMap mounts = new MountMap();
     mounts.putAll(setupBlazeUtils());
     mounts.putAll(mountRunfilesFromManifests(spawn));
@@ -303,7 +309,19 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
     FileSystem fs = blazeDirs.getFileSystem();
     mounts.put(fs.getPath("/bin"), fs.getPath("/bin"));
     mounts.put(fs.getPath("/sbin"), fs.getPath("/sbin"));
-    mounts.put(fs.getPath("/etc"), fs.getPath("/etc"));
+
+    // Check if /etc/resolv.conf is a symlink and mount its target
+    // Fix #738
+    Path resolv = fs.getPath("/etc/resolv.conf");
+    if (resolv.exists() && resolv.isSymbolicLink()) {
+      mounts.put(resolv, resolv.resolveSymbolicLinks());
+
+      List<Path> resolvList = ImmutableList.of(resolv);
+      mounts.putAll(mountDirExclude(fs.getPath("/etc"), resolvList));
+    } else {
+      mounts.put(fs.getPath("/etc"), fs.getPath("/etc"));
+    }
+
     for (String entry : NativePosixFiles.readdir("/")) {
       if (entry.startsWith("lib")) {
         Path libDir = fs.getRootDirectory().getRelative(entry);
@@ -498,6 +516,86 @@ public class LinuxSandboxedStrategy implements SpawnActionContext {
     return mounts;
   }
 
+  /**
+   * Mount all user defined path in --sandbox_add_path.
+   */
+  private MountMap mountUserDefinedPath() throws IOException {
+    MountMap mounts = new MountMap();
+    FileSystem fs = blazeDirs.getFileSystem();
+
+    ImmutableList<Path> exclude =
+        ImmutableList.of(blazeDirs.getWorkspace(), blazeDirs.getOutputBase());
+
+    for (String pathStr : sandboxAddPath) {
+      Path path = fs.getPath(pathStr);
+
+      // Check if path is in {workspace, outputBase}
+      for (Path exc : exclude) {
+        if (path.startsWith(exc)) {
+          throw new IllegalArgumentException(
+              "Mounting subdirectory of WORKSPACE or OUTPUTBASE to sandbox is not allowed.");
+        }
+      }
+
+      // Check if path is ancestor of {workspace, outputBase}
+      // Mount subdirectory of path except {workspace, outputBase}
+      mounts.putAll(mountChildDirExclude(path, exclude));
+    }
+
+    return mounts;
+  }
+
+  /**
+   * Mount all subdirectories recursively except some paths
+   */
+  private MountMap mountDirExclude(Path path, List<Path> exclude) throws IOException {
+    MountMap mounts = new MountMap();
+
+    if (!path.isDirectory(Symlinks.NOFOLLOW)) {
+      if (!exclude.contains(path)) {
+        mounts.put(path, path);
+      }
+      return mounts;
+    }
+
+    try {
+      for (Path child : path.getDirectoryEntries()) {
+        // Ignore broken symlink
+        if (!child.exists()) {
+          continue;
+        }
+
+        mounts.putAll(mountChildDirExclude(child, exclude));
+      }
+    } catch (IOException e) {
+      throw new IOException("Illegal additional path for mount", e);
+    }
+
+    return mounts;
+  }
+
+  /**
+   * Helper function of mountDirExclude and mountUserDefinedPath
+   */
+  private MountMap mountChildDirExclude(Path child, List<Path> exclude) throws IOException {
+    MountMap mounts = new MountMap();
+
+    boolean startsWithFlag = false;
+    for (Path exc : exclude) {
+      if (exc.startsWith(child)) {
+        startsWithFlag = true;
+        break;
+      }
+    }
+    if (!startsWithFlag) {
+      mounts.put(child, child);
+    } else if (!exclude.contains(child)) {
+      mounts.putAll(mountDirExclude(child, exclude));
+    }
+
+    return mounts;
+  }
+
   @Override
   public boolean willExecuteRemotely(boolean remotable) {
     return false;
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
index 03476912a1..914b0c56e4 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxActionContextProvider.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.util.OS;
 
+import java.util.List;
 import java.util.concurrent.ExecutorService;
 
 /**
@@ -36,6 +37,7 @@ public class SandboxActionContextProvider extends ActionContextProvider {
       CommandEnvironment env, BuildRequest buildRequest, ExecutorService backgroundWorkers) {
     boolean verboseFailures = buildRequest.getOptions(ExecutionOptions.class).verboseFailures;
     boolean sandboxDebug = buildRequest.getOptions(SandboxOptions.class).sandboxDebug;
+    List<String> sandboxAddPath = buildRequest.getOptions(SandboxOptions.class).sandboxAddPath;
     Builder<ActionContext> strategies = ImmutableList.builder();
 
     if (OS.getCurrent() == OS.LINUX) {
@@ -45,7 +47,8 @@ public class SandboxActionContextProvider extends ActionContextProvider {
               env.getDirectories(),
               backgroundWorkers,
               verboseFailures,
-              sandboxDebug));
+              sandboxDebug,
+              sandboxAddPath));
     }
 
     this.strategies = strategies.build();
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
index 32b715f229..f1291d2cbb 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.lib.sandbox;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsBase;
 
+import java.util.List;
+
 /**
  * Options for sandboxed execution.
  */
@@ -38,4 +40,13 @@ public class SandboxOptions extends OptionsBase {
             + "Bazel or Skylark rules with debugging failures due to missing input files, etc."
   )
   public boolean sandboxDebug;
+
+  @Option(
+    name = "sandbox_add_path",
+    allowMultiple = true,
+    defaultValue = "",
+    category = "config",
+    help = "Add additional path to mount to sandbox. Path including workspace is not allowed."
+  )
+  public List<String> sandboxAddPath;
 }
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTestCase.java b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTestCase.java
index 8f7b9a7ef9..d64758494c 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTestCase.java
@@ -105,7 +105,8 @@ public class LinuxSandboxedStrategyTestCase {
                     blazeDirs,
                     MoreExecutors.newDirectExecutorService(),
                     true,
-                    false)),
+                    false,
+                    ImmutableList.<String>of())),
             ImmutableList.<ActionContextProvider>of());
   }
 
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 03ea162397..bb19c0778c 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -274,6 +274,7 @@ sh_test(
     size = "large",
     srcs = ["bazel_sandboxing_test.sh"],
     data = [":test-deps"],
+    tags = ["local"],
 )
 
 sh_test(
diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh
index e2227dc142..d0f9089764 100755
--- a/src/test/shell/bazel/bazel_sandboxing_test.sh
+++ b/src/test/shell/bazel/bazel_sandboxing_test.sh
@@ -149,6 +149,13 @@ genrule(
   outs = [ "breaks3.txt" ],
   cmd = "wc $(location :cyclic1) > $@",
 )
+
+genrule(
+  name = "check_sandbox_contain_WORKSPACE",
+  outs = [ "check_sandbox_contain_WORKSPACE.txt" ],
+  cmd = "ls -l $$(dirname \"$$(pwd)\") &> $@",
+)
+
 EOF
   cat << 'EOF' >> examples/genrule/datafile
 this is a datafile
@@ -372,6 +379,45 @@ EOF
   kill_nc
 }
 
+function test_sandbox_add_path_valid_path() {
+  output_file="${BAZEL_GENFILES_DIR}/examples/genrule/breaks2.txt"
+
+  bazel build --sandbox_add_path=/var/log examples/genrule:breaks2 &> $TEST_log \
+    || fail "Non-hermetic genrule failed: examples/genrule:breaks2 (with additional path)"
+
+  [ -f "$output_file" ] ||
+    fail "Action did not produce output: $output_file"
+
+  if [ $(wc -l < $output_file) -le 1 ]; then
+    fail "Output contained less than or equal to one line: $output_file"
+  fi
+}
+
+function test_sandbox_add_path_workspace_parent() {
+  output_file="${BAZEL_GENFILES_DIR}/examples/genrule/check_sandbox_contain_WORKSPACE.txt"
+  parent_path="$(dirname "$(pwd)")"
+
+  bazel build --sandbox_add_path=$parent_path examples/genrule:check_sandbox_contain_WORKSPACE &> $TEST_log \
+    || fail "Non-hermetic genrule succeeded: examples/genrule:works (with additional path)"
+  [ -f "$output_file" ] \
+    || fail "Genrule did not produce output: examples/genrule:check_sandbox_contain_WORKSPACE (with additional path: WORKSPACE/..)"
+  cat $output_file &> $TEST_log
+
+  # file and directory inside workspace (except project) should not be mounted
+  egrep "\bWORKSPACE\b" $output_file \
+    && fail "WORKSPACE file should not be mounted." || true
+}
+
+function test_sandbox_add_path_workspace_child() {
+  child_path="$(pwd)/examples"
+  output_file="${BAZEL_GENFILES_DIR}/examples/genrule/works.txt"
+
+  bazel build --sandbox_add_path=$child_path examples/genrule:works &> $TEST_log \
+    && fail "Non-hermetic genrule succeeded: examples/genrule:works (with additional path: WORKSPACE:/examples)" || true
+
+  expect_log "Mounting subdirectory of WORKSPACE or OUTPUTBASE to sandbox is not allowed"
+}
+
 # The test shouldn't fail if the environment doesn't support running it.
 check_supported_platform || exit 0
 check_sandbox_allowed || exit 0