* Use lchown() instead of chown() in canonicalisePathMetaData(). This

  matters when running as root, since then we don't use the setuid
  helper (which already used lchown()).
  
* Also check for an obscure security problem on platforms that don't
  have lchown.  Then we can't change the ownership of symlinks, which
  doesn't matter *except* when the containing directory is writable by
  the owner (which is the case with the top-level Nix store directory).

1 files changed, 38 insertions, 8 deletions

diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 4144f0831..01d1e398c 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -6,6 +6,7 @@
 #include "pathlocks.hh"
 #include "aterm.hh"
 #include "derivations-ast.hh"
+#include "config.h"
     
 #include <iostream>
 #include <algorithm>
@@ -213,7 +214,7 @@ void copyPath(const Path & src, const Path & dst)
 }
 
 
-void canonicalisePathMetaData(const Path & path)
+static void _canonicalisePathMetaData(const Path & path)
 {
     checkInterrupt();
 
@@ -221,14 +222,26 @@ void canonicalisePathMetaData(const Path & path)
     if (lstat(path.c_str(), &st))
 	throw SysError(format("getting attributes of path `%1%'") % path);
 
+    /* Change ownership to the current uid.  If its 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
+       the symlink and can't delete it because the directory is not
+       writable.  The only exception is top-level paths in the Nix
+       store (since that directory is group-writable for the Nix build
+       users group); we check for this case below. */
+    if (st.st_uid != geteuid()) {
+#if HAVE_LCHOWN
+        if (lchown(path.c_str(), geteuid(), -1) == -1)
+#else
+        if (!S_ISLNK(st.st_mode) &&
+            chown(path.c_str(), geteuid(), -1) == -1)
+#endif
+            throw SysError(format("changing owner of `%1%' to %2%")
+                % path % geteuid());
+    }
+    
     if (!S_ISLNK(st.st_mode)) {
 
-        if (st.st_uid != geteuid()) {
-            if (chown(path.c_str(), geteuid(), -1) == -1)
-                throw SysError(format("changing owner of `%1%' to %2%")
-                    % path % geteuid());
-        }
-
         /* Mask out all type related bits. */
         mode_t mode = st.st_mode & ~S_IFMT;
         
@@ -253,7 +266,24 @@ void canonicalisePathMetaData(const Path & path)
     if (S_ISDIR(st.st_mode)) {
         Strings names = readDirectory(path);
 	for (Strings::iterator i = names.begin(); i != names.end(); ++i)
-	    canonicalisePathMetaData(path + "/" + *i);
+	    _canonicalisePathMetaData(path + "/" + *i);
+    }
+}
+
+
+void canonicalisePathMetaData(const Path & path)
+{
+    _canonicalisePathMetaData(path);
+
+    /* On platforms that don't have lchown(), the top-level path can't
+       be a symlink, since we can't change its ownership. */
+    struct stat st;
+    if (lstat(path.c_str(), &st))
+	throw SysError(format("getting attributes of path `%1%'") % path);
+
+    if (st.st_uid != geteuid()) {
+        assert(S_ISLNK(st.st_mode));
+        throw Error(format("wrong ownership of top-level store path `%1%'") % path);
     }
 }