libstore: add intermediate directory to build-dirs

this makes the actual build directories used by builders invisible and
inaccessible to other processes on the system, avoiding another vector
for outside processes to interfere with builds or pass credentials the
build sandbox should not have access to into the build sandbox anyway.

fixes #919

Change-Id: Ifaa4d8e3940cfde1406e925f75c1375d2e86d81a

lix/libstore/build/local-derivation-goal.cc

@@ -359,10 +359,11 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull()
         if (statvfs(localStore.config().realStoreDir.get().c_str(), &st) == 0 &&
             (uint64_t) st.f_bavail * st.f_bsize < required)
             diskFull = true;
-        if (statvfs(tmpDir.c_str(), &st) == 0 &&
-            (uint64_t) st.f_bavail * st.f_bsize < required)
+        if (statvfs(tmpDirRoot.c_str(), &st) == 0 && (uint64_t) st.f_bavail * st.f_bsize < required)
+        {
             diskFull = true;
         }
+    }
 #endif
 
     deleteTmpDir(false);
@@ -439,7 +440,7 @@ try {
 
         /* Create a temporary directory where the build will take
            place. */
-        tmpDir =
+        tmpDirRoot =
             createTempDir(buildDir, "nix-build-" + std::string(drvPath.name()), false, false, 0700);
     } catch (SysError & e) {
         /*
@@ -470,17 +471,46 @@ try {
             nixBuildsTmp
         );
         worker.buildDirOverride = nixBuildsTmp;
-        tmpDir = createTempDir(
+        tmpDirRoot = createTempDir(
             nixBuildsTmp, "nix-build-" + std::string(drvPath.name()), false, false, 0700
         );
     }
     /* The TOCTOU between the previous mkdir call and this open call is unavoidable due to
      * POSIX semantics.*/
-    tmpDirFd = AutoCloseFD{open(tmpDir.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
+    tmpDirRootFd = AutoCloseFD{open(tmpDirRoot.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
+    if (!tmpDirRootFd) {
+        throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDirRoot);
+    }
+
+    // place the actual build directory in a subdirectory of tmpDirRoot. if
+    // we do not do this a build can `chown 777` its build directory and so
+    // make it accessible to everyone in the system, breaking isolation. we
+    // also need the intermediate level to be inaccessible to others. build
+    // processes must be able to at least traverse to the directory though,
+    // without being able to chmod. this means either mode 0750 or 0710. we
+    // use 0710 just to be extra safe; if we ever add more directories they
+    // will not be enumerable to other processes in the builder user group.
+    //
+    // use a short name to not increase the path length too much on darwin.
+    // darwin has a severe sockaddr_un path length limitation, so this does
+    // make a difference over more evocative names. we use `b` for `build`.
+    tmpDir = tmpDirRoot + "/b";
+    if (mkdirat(tmpDirRootFd.get(), "b", 0700)) {
+        throw SysError("failed to create the build temporary directory '%1%'", tmpDir);
+    }
+    tmpDirFd = AutoCloseFD{openat(tmpDirRootFd.get(), "b", O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
     if (!tmpDirFd)
         throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir);
 
     chownToBuilder(tmpDirFd);
+    if (buildUser) {
+        if (fchown(tmpDirRootFd.get(), -1, buildUser->getGID()) == -1) {
+            throw SysError("cannot change ownership of '%1%'", tmpDirRoot);
+        }
+        if (fchmod(tmpDirRootFd.get(), 0710) == -1) {
+            throw SysError("cannot change mode of '%1%'", tmpDirRoot);
+        }
+    }
 
     for (auto & [outputName, status] : initialOutputs) {
         /* Set scratch path we'll actually use during the build.
@@ -2357,18 +2387,18 @@ try {
 
 void LocalDerivationGoal::deleteTmpDir(bool force, bool duringDestruction)
 {
-    if (tmpDir != "") {
+    if (tmpDirRoot != "") {
         /* Don't keep temporary directories for builtins because they
            might have privileged stuff (like a copy of netrc). */
         if (settings.keepFailed && !force && !drv->isBuiltin()) {
-            printError("note: keeping build directory '%s'", tmpDir);
-            chmod(tmpDir.c_str(), 0755);
+            printError("note: keeping build directory '%s'", tmpDirRoot);
+            chmod(tmpDirRoot.c_str(), 0755);
         }
         else if (duringDestruction)
-            deletePathUninterruptible(tmpDir);
+            deletePathUninterruptible(tmpDirRoot);
         else
-            deletePath(tmpDir);
-        tmpDir = "";
+            deletePath(tmpDirRoot);
+        tmpDirRoot = "";
     }
 }
 

lix/libstore/build/local-derivation-goal.hh

@@ -40,12 +40,12 @@ struct LocalDerivationGoal : public DerivationGoal
     /**
      * The temporary directory.
      */
-    Path tmpDir;
+    Path tmpDirRoot, tmpDir;
 
     /**
      * The temporary directory file descriptor
      */
-    AutoCloseFD tmpDirFd;
+    AutoCloseFD tmpDirRootFd, tmpDirFd;
 
     /**
      * The path of the temporary directory in the sandbox.

tests/functional/build-remote.sh

@@ -77,4 +77,4 @@ out="$(nix-build 2>&1 failing.nix \
 [[ "$out" =~ .*"note: keeping build directory".* ]]
 
 build_dir="$(grep "note: keeping build" <<< "$out" | sed -E "s/^(.*)note: keeping build directory '(.*)'(.*)$/\2/")"
-[[ "foo" = $(<"$build_dir"/bar) ]]
+[[ "foo" = $(<"$build_dir"/b/bar) ]]

tests/functional/build.sh

@@ -179,3 +179,23 @@ test "$(<<<"$out" grep -E '^error:' | wc -l)" = 3
 BUILD_DIR=$(mktemp -d)
 chmod 0000 "$BUILD_DIR"
 nix --build-dir "$BUILD_DIR" build -E 'with import ./config.nix; mkDerivation { name = "test"; buildCommand = "echo rawr > $out"; }' --impure --no-link
+
+# ensure that the build directory parent is not world-accessible
+chmod 0755 "$BUILD_DIR"
+FIFO="$BUILD_DIR/fifo"
+mkfifo "$FIFO"
+(
+  echo > "$FIFO"
+  trap 'echo > "$FIFO"' EXIT
+  mode=$(stat -c %a $BUILD_DIR/b/*)
+  [ "$mode" = "700" -o "$mode" = "710" ]
+) &
+nix build --build-dir "$BUILD_DIR/b" -E '
+  with import ./config.nix; mkDerivation {
+    name = "test";
+    buildCommand = "cat '"$FIFO"'; cat '"$FIFO"' > $out";
+  }' \
+  --extra-sandbox-paths "$FIFO" \
+  --impure \
+  --no-link
+wait

tests/functional/check.sh

@@ -45,7 +45,7 @@ test_custom_build_dir() {
   [ "$status" = "100" ]
   [[ 1 == "$(count "$customBuildDir/nix-build-"*)" ]]
   local buildDir="$customBuildDir/nix-build-"*
-  grep $checkBuildId $buildDir/checkBuildId
+  grep $checkBuildId $buildDir/b/checkBuildId
 }
 test_custom_build_dir