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.
This commit is contained in:
Eelco Dolstra 2013-02-26 02:30:19 +01:00
parent dadf7a5b46
commit 5526a282b5
5 changed files with 17 additions and 20 deletions

View file

@ -2287,7 +2287,7 @@ void DerivationGoal::computeClosure()
} }
/* Get rid of all weird permissions. */ /* 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 /* For this output path, find the references to other paths
contained in it. Compute the SHA-256 NAR hash at the same contained in it. Compute the SHA-256 NAR hash at the same
@ -2839,7 +2839,7 @@ void SubstitutionGoal::finished()
return; return;
} }
canonicalisePathMetaData(destPath); canonicalisePathMetaData(destPath, -1);
worker.store.optimisePath(destPath); // FIXME: combine with hashPath() worker.store.optimisePath(destPath); // FIXME: combine with hashPath()

View file

@ -465,7 +465,7 @@ void LocalStore::makeStoreWritable()
const time_t mtimeStore = 1; /* 1 second into the epoch */ 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(); checkInterrupt();
@ -477,6 +477,9 @@ void canonicalisePathMetaData(const Path & path, bool recurse)
has already been checked in dumpPath(). */ has already been checked in dumpPath(). */
assert(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode)); 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 /* Change ownership to the current uid. If it's a symlink, use
lchown if available, otherwise don't bother. Wrong ownership lchown if available, otherwise don't bother. Wrong ownership
of a symlink doesn't matter, since the owning user can't change 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)) { if (recurse && S_ISDIR(st.st_mode)) {
Strings names = readDirectory(path); Strings names = readDirectory(path);
foreach (Strings::iterator, i, names) 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 /* On platforms that don't have lchown(), the top-level path can't
be a symlink, since we can't change its ownership. */ be a symlink, since we can't change its ownership. */
@ -1198,7 +1201,7 @@ Path LocalStore::addToStoreFromDump(const string & dump, const string & name,
} else } else
writeFile(dstPath, dump); writeFile(dstPath, dump);
canonicalisePathMetaData(dstPath); canonicalisePathMetaData(dstPath, -1);
/* Register the SHA-256 hash of the NAR serialisation of /* Register the SHA-256 hash of the NAR serialisation of
the path in the database. We may just have computed it 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); writeFile(dstPath, s);
canonicalisePathMetaData(dstPath); canonicalisePathMetaData(dstPath, -1);
HashResult hash = hashPath(htSHA256, dstPath); HashResult hash = hashPath(htSHA256, dstPath);
@ -1498,7 +1501,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source)
throw SysError(format("cannot move `%1%' to `%2%'") throw SysError(format("cannot move `%1%' to `%2%'")
% unpacked % dstPath); % unpacked % dstPath);
canonicalisePathMetaData(dstPath); canonicalisePathMetaData(dstPath, -1);
/* !!! if we were clever, we could prevent the hashPath() /* !!! if we were clever, we could prevent the hashPath()
here. */ here. */

View file

@ -307,9 +307,9 @@ private:
without execute permission; setuid bits etc. are cleared) without execute permission; setuid bits etc. are cleared)
- the owner and group are set to the Nix user and group, if we're - the owner and group are set to the Nix user and group, if we're
in a setuid Nix installation. */ 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); MakeError(PathInUse, Error);

View file

@ -32,7 +32,7 @@ struct MakeReadOnly
{ {
try { try {
/* This will make the path read-only. */ /* This will make the path read-only. */
if (path != "") canonicalisePathMetaData(path, false); if (path != "") canonicalisePathMetaData(path, false, -1);
} catch (...) { } catch (...) {
ignoreException(); ignoreException();
} }

View file

@ -238,12 +238,6 @@ static void printTree(const Path & path,
PathSet references; PathSet references;
store->queryReferences(path, 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 /* Topologically sort under the relation A < B iff A \in
closure(B). That is, if derivation A is an (possibly indirect) closure(B). That is, if derivation A is an (possibly indirect)
input of B, then A is printed first. This has the effect of 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); Paths sorted = topoSortPaths(*store, references);
reverse(sorted.begin(), sorted.end()); 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; Paths::iterator j = i; ++j;
printTree(*i, tailPad + treeConn, printTree(*i, tailPad + treeConn,
j == sorted.end() ? tailPad + treeNull : tailPad + treeLine, 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) { if (!store->isValidPath(info.path) || reregister) {
/* !!! races */ /* !!! races */
if (canonicalise) if (canonicalise)
canonicalisePathMetaData(info.path); canonicalisePathMetaData(info.path, -1);
if (!hashGiven) { if (!hashGiven) {
HashResult hash = hashPath(htSHA256, info.path); HashResult hash = hashPath(htSHA256, info.path);
info.hash = hash.first; info.hash = hash.first;