Security: Don't allow builders to change permissions on files they don't own

It turns out that in multi-user Nix, a builder may be able to do

  ln /etc/shadow $out/foo

Afterwards, canonicalisePathMetaData() will be applied to $out/foo,
causing /etc/shadow's mode to be set to 444 (readable by everybody but
writable by nobody).  That's obviously Very Bad.

Fortunately, this fails in NixOS's default configuration because
/nix/store is a bind mount, so "ln" will fail with "Invalid
cross-device link".  It also fails if hard-link restrictions are
enabled, so a workaround is:

  echo 1 > /proc/sys/fs/protected_hardlinks

The solution is to check that all files in $out are owned by the build
user.  This means that innocuous operations like "ln
${pkgs.foo}/some-file $out/" are now rejected, but that already failed
in chroot builds anyway.

5 files changed, 17 insertions, 20 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index f583d6857e..73223bc1ad 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -2287,7 +2287,7 @@ void DerivationGoal::computeClosure()
         }
 
         /* Get rid of all weird permissions. */
-        canonicalisePathMetaData(path);
+        canonicalisePathMetaData(path, buildUser.enabled() ? buildUser.getUID() : -1);
 
         /* For this output path, find the references to other paths
            contained in it.  Compute the SHA-256 NAR hash at the same
@@ -2839,7 +2839,7 @@ void SubstitutionGoal::finished()
         return;
     }
 
-    canonicalisePathMetaData(destPath);
+    canonicalisePathMetaData(destPath, -1);
 
     worker.store.optimisePath(destPath); // FIXME: combine with hashPath()
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 84050f0e49..d5ee9361e0 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -465,7 +465,7 @@ void LocalStore::makeStoreWritable()
 const time_t mtimeStore = 1; /* 1 second into the epoch */
 
 
-void canonicalisePathMetaData(const Path & path, bool recurse)
+void canonicalisePathMetaData(const Path & path, bool recurse, uid_t fromUid)
 {
     checkInterrupt();
 
@@ -477,6 +477,9 @@ void canonicalisePathMetaData(const Path & path, bool recurse)
        has already been checked in dumpPath(). */
     assert(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode));
 
+    if (fromUid != (uid_t) -1 && st.st_uid != fromUid)
+        throw BuildError(format("invalid ownership on file `%1%'") % path);
+
     /* Change ownership to the current uid.  If it's a symlink, use
        lchown if available, otherwise don't bother.  Wrong ownership
        of a symlink doesn't matter, since the owning user can't change
@@ -529,14 +532,14 @@ void canonicalisePathMetaData(const Path & path, bool recurse)
     if (recurse && S_ISDIR(st.st_mode)) {
         Strings names = readDirectory(path);
         foreach (Strings::iterator, i, names)
-            canonicalisePathMetaData(path + "/" + *i, true);
+            canonicalisePathMetaData(path + "/" + *i, true, fromUid);
     }
 }
 
 
-void canonicalisePathMetaData(const Path & path)
+void canonicalisePathMetaData(const Path & path, uid_t fromUid)
 {
-    canonicalisePathMetaData(path, true);
+    canonicalisePathMetaData(path, true, fromUid);
 
     /* On platforms that don't have lchown(), the top-level path can't
        be a symlink, since we can't change its ownership. */
@@ -1198,7 +1201,7 @@ Path LocalStore::addToStoreFromDump(const string & dump, const string & name,
             } else
                 writeFile(dstPath, dump);
 
-            canonicalisePathMetaData(dstPath);
+            canonicalisePathMetaData(dstPath, -1);
 
             /* Register the SHA-256 hash of the NAR serialisation of
                the path in the database.  We may just have computed it
@@ -1263,7 +1266,7 @@ Path LocalStore::addTextToStore(const string & name, const string & s,
 
             writeFile(dstPath, s);
 
-            canonicalisePathMetaData(dstPath);
+            canonicalisePathMetaData(dstPath, -1);
 
             HashResult hash = hashPath(htSHA256, dstPath);
 
@@ -1498,7 +1501,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source)
                 throw SysError(format("cannot move `%1%' to `%2%'")
                     % unpacked % dstPath);
 
-            canonicalisePathMetaData(dstPath);
+            canonicalisePathMetaData(dstPath, -1);
 
             /* !!! if we were clever, we could prevent the hashPath()
                here. */
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index 2b0d713809..d3e1ca2929 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -307,9 +307,9 @@ private:
      without execute permission; setuid bits etc. are cleared)
    - the owner and group are set to the Nix user and group, if we're
      in a setuid Nix installation. */
-void canonicalisePathMetaData(const Path & path);
+void canonicalisePathMetaData(const Path & path, uid_t fromUid);
 
-void canonicalisePathMetaData(const Path & path, bool recurse);
+void canonicalisePathMetaData(const Path & path, bool recurse, uid_t fromUid);
 
 MakeError(PathInUse, Error);
 
diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc
index e91c2b1ce5..75f2d7d6cc 100644
--- a/src/libstore/optimise-store.cc
+++ b/src/libstore/optimise-store.cc
@@ -32,7 +32,7 @@ struct MakeReadOnly
     {
         try {
             /* This will make the path read-only. */
-            if (path != "") canonicalisePathMetaData(path, false);
+            if (path != "") canonicalisePathMetaData(path, false, -1);
         } catch (...) {
             ignoreException();
         }
diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc
index e419223040..151ed97e4b 100644
--- a/src/nix-store/nix-store.cc
+++ b/src/nix-store/nix-store.cc
@@ -238,12 +238,6 @@ static void printTree(const Path & path,
     PathSet references;
     store->queryReferences(path, references);
 
-#if 0
-    for (PathSet::iterator i = drv.inputSrcs.begin();
-         i != drv.inputSrcs.end(); ++i)
-        cout << format("%1%%2%\n") % (tailPad + treeConn) % *i;
-#endif
-
     /* Topologically sort under the relation A < B iff A \in
        closure(B).  That is, if derivation A is an (possibly indirect)
        input of B, then A is printed first.  This has the effect of
@@ -251,7 +245,7 @@ static void printTree(const Path & path,
     Paths sorted = topoSortPaths(*store, references);
     reverse(sorted.begin(), sorted.end());
 
-    for (Paths::iterator i = sorted.begin(); i != sorted.end(); ++i) {
+    foreach (Paths::iterator, i, sorted) {
         Paths::iterator j = i; ++j;
         printTree(*i, tailPad + treeConn,
             j == sorted.end() ? tailPad + treeNull : tailPad + treeLine,
@@ -521,7 +515,7 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise)
         if (!store->isValidPath(info.path) || reregister) {
             /* !!! races */
             if (canonicalise)
-                canonicalisePathMetaData(info.path);
+                canonicalisePathMetaData(info.path, -1);
             if (!hashGiven) {
                 HashResult hash = hashPath(htSHA256, info.path);
                 info.hash = hash.first;