Skip to content

Commit

Permalink
Enforce definition of a SecretMode around canonicalise functions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@nbp
nbp committed Aug 25, 2014
1 parent cea9f58 commit 8bfcdb4
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 30 deletions.
12 changes: 9 additions & 3 deletions src/libstore/build.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand Down
31 changes: 17 additions & 14 deletions src/libstore/local-store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {

Expand All @@ -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);
}
Expand All @@ -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))
Expand All @@ -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();

Expand Down Expand Up @@ -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
Expand All @@ -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. */
Expand All @@ -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);
}


Expand Down Expand Up @@ -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. */
Expand All @@ -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
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;

Expand All @@ -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. */
Expand Down
8 changes: 4 additions & 4 deletions src/libstore/local-store.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -328,10 +328,10 @@ typedef set<Inode> 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);

Expand Down
24 changes: 17 additions & 7 deletions src/libstore/optimise-store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();

Expand All @@ -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;
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
}


Expand Down
7 changes: 5 additions & 2 deletions src/nix-store/nix-store.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions tests/sec-secretDerivation.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 6 additions & 0 deletions tests/sec-secretDerivation.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 8bfcdb4

Please sign in to comment.