From 5526a282b5b44e9296e61e07d7d2626a79141ac4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 26 Feb 2013 02:30:19 +0100 Subject: 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. --- src/libstore/local-store.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'src/libstore/local-store.cc') diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 84050f0e49b6..d5ee9361e044 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. */ -- cgit 1.4.1