From 8bfcdb47c5c9f6f5cdbdbaf379b09db5f161ac49 Mon Sep 17 00:00:00 2001 From: Nicolas Pierron Date: Mon, 25 Aug 2014 23:04:07 +0200 Subject: [PATCH] Enforce definition of a SecretMode around canonicalise functions. --- src/libstore/build.cc | 12 +++++++++--- src/libstore/local-store.cc | 31 +++++++++++++++++-------------- src/libstore/local-store.hh | 8 ++++---- src/libstore/optimise-store.cc | 24 +++++++++++++++++------- src/nix-store/nix-store.cc | 7 +++++-- tests/sec-secretDerivation.nix | 4 ++++ tests/sec-secretDerivation.sh | 6 ++++++ 7 files changed, 62 insertions(+), 30 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index c547a5cbfecf..e75215d2c4d1 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2200,6 +2200,9 @@ void DerivationGoal::registerOutputs() Path path = i->second.path; if (missingPaths.find(path) == missingPaths.end()) continue; + // :TODO: Transfer the user name down to here. + SecretMode smode(publicUserName()); + Path actualPath = path; if (useChroot) { actualPath = chrootRootDir + path; @@ -2249,7 +2252,7 @@ void DerivationGoal::registerOutputs() /* Canonicalise first. This ensures that the path we're rewriting doesn't contain a hard link to /etc/shadow or something like that. */ - canonicalisePathMetaData(actualPath, buildUser.enabled() ? buildUser.getUID() : -1, inodesSeen); + canonicalisePathMetaData(actualPath, buildUser.enabled() ? buildUser.getUID() : -1, inodesSeen, smode); /* FIXME: this is in-memory. */ StringSink sink; @@ -2292,7 +2295,7 @@ void DerivationGoal::registerOutputs() /* Get rid of all weird permissions. This also checks that all files are owned by the build user, if applicable. */ canonicalisePathMetaData(actualPath, - buildUser.enabled() && !rewritten ? buildUser.getUID() : -1, inodesSeen); + buildUser.enabled() && !rewritten ? buildUser.getUID() : -1, inodesSeen, smode); /* For this output path, find the references to other paths contained in it. Compute the SHA-256 NAR hash at the same @@ -2841,9 +2844,12 @@ void SubstitutionGoal::finished() return; } + // :TODO: Transfer the user name down to here. + SecretMode smode(publicUserName()); + if (repair) replaceValidPath(storePath, destPath); - canonicalisePathMetaData(storePath, -1); + canonicalisePathMetaData(storePath, -1, smode); worker.store.optimisePath(storePath); // FIXME: combine with hashPath() diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 32ab51bef2a7..5c6e5ce9d335 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -499,7 +499,7 @@ void LocalStore::makeStoreWritable() const time_t mtimeStore = 1; /* 1 second into the epoch */ -static void canonicaliseTimestampAndPermissions(const Path & path, const struct stat & st, const SecretMode *secret) +static void canonicaliseTimestampAndPermissions(const Path & path, const struct stat & st, const SecretMode & smode) { if (!S_ISLNK(st.st_mode)) { @@ -510,8 +510,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct mode = (st.st_mode & S_IFMT) | 0444 | (st.st_mode & S_IXUSR ? 0111 : 0); - if (secret) - mode = secret->filterMode(mode); + mode = smode.filterMode(mode); if (chmod(path.c_str(), mode) == -1) throw SysError(format("changing mode of ‘%1%’ to %2$o") % path % mode); } @@ -536,7 +535,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct } -void canonicaliseTimestampAndPermissions(const Path & path, const SecretMode *secret) +void canonicaliseTimestampAndPermissions(const Path & path, const SecretMode & secret) { struct stat st; if (lstat(path.c_str(), &st)) @@ -545,7 +544,7 @@ void canonicaliseTimestampAndPermissions(const Path & path, const SecretMode *se } -static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode *secret = nullptr) +static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode & smode) { checkInterrupt(); @@ -574,7 +573,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe inodesSeen.insert(Inode(st.st_dev, st.st_ino)); - canonicaliseTimestampAndPermissions(path, st, secret); + canonicaliseTimestampAndPermissions(path, st, smode); /* Change ownership to the current uid. If it's a symlink, use lchown if available, otherwise don't bother. Wrong ownership @@ -597,14 +596,14 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe if (S_ISDIR(st.st_mode)) { DirEntries entries = readDirectory(path); for (auto & i : entries) - canonicalisePathMetaData_(path + "/" + i.name, fromUid, inodesSeen); + canonicalisePathMetaData_(path + "/" + i.name, fromUid, inodesSeen, smode); } } -void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode *secret) +void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode & smode) { - canonicalisePathMetaData_(path, fromUid, inodesSeen, secret); + canonicalisePathMetaData_(path, fromUid, inodesSeen, smode); /* On platforms that don't have lchown(), the top-level path can't be a symlink, since we can't change its ownership. */ @@ -619,10 +618,10 @@ void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & ino } -void canonicalisePathMetaData(const Path & path, uid_t fromUid, const SecretMode *secret) +void canonicalisePathMetaData(const Path & path, uid_t fromUid, const SecretMode & smode) { InodesSeen inodesSeen; - canonicalisePathMetaData(path, fromUid, inodesSeen, secret); + canonicalisePathMetaData(path, fromUid, inodesSeen, smode); } @@ -1349,6 +1348,8 @@ Path LocalStore::addToStoreFromDump(const string & dump, const string & name, addTempRoot(dstPath); if (repair || !isValidPath(dstPath)) { + // :TODO: Transfer the user name down to here. + SecretMode smode(publicUserName()); /* The first check above is an optimisation to prevent unnecessary lock acquisition. */ @@ -1365,7 +1366,7 @@ Path LocalStore::addToStoreFromDump(const string & dump, const string & name, } else writeFile(dstPath, dump); - canonicalisePathMetaData(dstPath, -1); + canonicalisePathMetaData(dstPath, -1, smode); /* Register the SHA-256 hash of the NAR serialisation of the path in the database. We may just have computed it @@ -1431,7 +1432,7 @@ Path LocalStore::addTextToStore(const string & name, const string & s, writeFile(dstPath, s); - canonicalisePathMetaData(dstPath, -1, &smode); + canonicalisePathMetaData(dstPath, -1, smode); HashResult hash = hashPath(htSHA256, dstPath); @@ -1647,6 +1648,8 @@ Path LocalStore::importPath(bool requireSignature, Source & source) addTempRoot(dstPath); if (!isValidPath(dstPath)) { + // :TODO: Infer the security of the path based on the ACL. + SecretMode smode(publicUserName()); PathLocks outputLock; @@ -1665,7 +1668,7 @@ Path LocalStore::importPath(bool requireSignature, Source & source) throw SysError(format("cannot move ‘%1%’ to ‘%2%’") % unpacked % dstPath); - canonicalisePathMetaData(dstPath, -1); + canonicalisePathMetaData(dstPath, -1, smode); /* !!! 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 6db3c2cf724b..d5d3ded2ef8a 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -308,7 +308,7 @@ private: InodeHash loadInodeHash(); Strings readDirectoryIgnoringInodes(const Path & path, const InodeHash & inodeHash); - void optimisePath_(OptimiseStats & stats, const Path & path, InodeHash & inodeHash); + void optimisePath_(OptimiseStats & stats, const Path & path, InodeHash & inodeHash, const SecretMode & smode); // Internal versions that are not wrapped in retry_sqlite. bool isValidPath_(const Path & path); @@ -328,10 +328,10 @@ typedef set InodesSeen; without execute permission; setuid bits etc. are cleared) - the owner and group are set to the Nix user and group, if we're running as root. */ -void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode *secret = nullptr); -void canonicalisePathMetaData(const Path & path, uid_t fromUid, const SecretMode *secret = nullptr); +void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & inodesSeen, const SecretMode & secret); +void canonicalisePathMetaData(const Path & path, uid_t fromUid, const SecretMode & secret); -void canonicaliseTimestampAndPermissions(const Path & path, const SecretMode *secret = nullptr); +void canonicaliseTimestampAndPermissions(const Path & path, const SecretMode & secret); MakeError(PathInUse, Error); diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 208d9688ed98..56bcb754dc71 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -27,12 +27,17 @@ static void makeWritable(const Path & path) struct MakeReadOnly { Path path; - MakeReadOnly(const Path & path) : path(path) { } + const SecretMode & smode; + + MakeReadOnly(const Path & path, const SecretMode & smode) + : path(path), smode(smode) + { } + ~MakeReadOnly() { try { /* This will make the path read-only. */ - if (path != "") canonicaliseTimestampAndPermissions(path); + if (path != "") canonicaliseTimestampAndPermissions(path, smode); } catch (...) { ignoreException(); } @@ -88,7 +93,8 @@ Strings LocalStore::readDirectoryIgnoringInodes(const Path & path, const InodeHa } -void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path, InodeHash & inodeHash) +void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path, + InodeHash & inodeHash, const SecretMode & smode) { checkInterrupt(); @@ -99,7 +105,7 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path, InodeHa if (S_ISDIR(st.st_mode)) { Strings names = readDirectoryIgnoringInodes(path, inodeHash); foreach (Strings::iterator, i, names) - optimisePath_(stats, path + "/" + *i, inodeHash); + optimisePath_(stats, path + "/" + *i, inodeHash, smode); return; } @@ -173,7 +179,7 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path, InodeHa /* When we're done, make the directory read-only again and reset its timestamp back to 0. */ - MakeReadOnly makeReadOnly(mustToggle ? dirOf(path) : ""); + MakeReadOnly makeReadOnly(mustToggle ? dirOf(path) : "", smode); Path tempLink = (format("%1%/.tmp-link-%2%-%3%") % settings.nixStore % getpid() % rand()).str(); @@ -221,7 +227,9 @@ void LocalStore::optimiseStore(OptimiseStats & stats) addTempRoot(*i); if (!isValidPath(*i)) continue; /* path was GC'ed, probably */ startNest(nest, lvlChatty, format("hashing files in ‘%1%’") % *i); - optimisePath_(stats, *i, inodeHash); + // :TODO: Infer the security of the path based on the ACL. + SecretMode smode(publicUserName()); + optimisePath_(stats, *i, inodeHash, smode); } } @@ -231,7 +239,9 @@ void LocalStore::optimisePath(const Path & path) OptimiseStats stats; InodeHash inodeHash; - if (settings.autoOptimiseStore) optimisePath_(stats, path, inodeHash); + // :TODO: Infer the security of the path based on the ACL. + SecretMode smode(publicUserName()); + if (settings.autoOptimiseStore) optimisePath_(stats, path, inodeHash, smode); } diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 8c3744824ce6..7e80fbfb4158 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -549,8 +549,11 @@ static void registerValidity(bool reregister, bool hashGiven, bool canonicalise) if (info.path == "") break; if (!store->isValidPath(info.path) || reregister) { /* !!! races */ - if (canonicalise) - canonicalisePathMetaData(info.path, -1); + if (canonicalise) { + // :TODO: Infer the security of the path based on the ACL. + SecretMode smode(publicUserName()); + canonicalisePathMetaData(info.path, -1, smode); + } if (!hashGiven) { HashResult hash = hashPath(htSHA256, info.path); info.hash = hash.first; diff --git a/tests/sec-secretDerivation.nix b/tests/sec-secretDerivation.nix index a272c0a83eb4..6fac0041d592 100644 --- a/tests/sec-secretDerivation.nix +++ b/tests/sec-secretDerivation.nix @@ -3,7 +3,11 @@ with import ./config.nix; let builder = builtins.toFile "builder.sh" '' + umask 077 + mkdir $out + chmod 700 $out echo 42 > $out/file + umask 077 ''; in diff --git a/tests/sec-secretDerivation.sh b/tests/sec-secretDerivation.sh index d53a92ea200b..05acfa07a803 100644 --- a/tests/sec-secretDerivation.sh +++ b/tests/sec-secretDerivation.sh @@ -15,10 +15,16 @@ startDaemon # Get the public derivation rights. publicDrv=$(nix-instantiate ./sec-secretDerivation.nix -A public) publicDrvStat=$(stat --format=%A $publicDrv) +publicOut=$(nix-store -rvv $publicDrv) +publicOutStat=$(stat --format=%A $publicOut) +publicOutFileStat=$(stat --format=%A $publicOut/file) # Get the secret derivation rights. secretDrv=$(nix-instantiate ./sec-secretDerivation.nix -A secret) secretDrvStat=$(stat --format=%A $secretDrv) +secretOut=$(nix-store -rvv $secretDrv) +secretOutStat=$(stat --format=%A $secretOut) +secretOutFileStat=$(stat --format=%A $secretOut/file) # Check file ownership, and verify that only the owner of the store is the # only one capable of reading the secret file.