* Fix a huuuuge security hole in the Nix daemon. It didn't check that

  derivations added to the store by clients have "correct" output
  paths (meaning that the output paths are computed by hashing the
  derivation according to a certain algorithm).  This means that a
  malicious user could craft a special .drv file to build *any*
  desired path in the store with any desired contents (so long as the
  path doesn't already exist).  Then the attacker just needs to wait
  for a victim to come along and install the compromised path.

  For instance, if Alice (the attacker) knows that the latest Firefox
  derivation in Nixpkgs produces the path

    /nix/store/1a5nyfd4ajxbyy97r1fslhgrv70gj8a7-firefox-5.0.1

  then (provided this path doesn't already exist) she can craft a .drv
  file that creates that path (i.e., has it as one of its outputs),
  add it to the store using "nix-store --add", and build it with
  "nix-store -r".  So the fake .drv could write a Trojan to the
  Firefox path.  Then, if user Bob (the victim) comes along and does

    $ nix-env -i firefox
    $ firefox

  he executes the Trojan injected by Alice.

  The fix is to have the Nix daemon verify that derivation outputs are
  correct (in addValidPath()).  This required some refactoring to move
  the hash computation code to libstore.

9 files changed, 180 insertions, 81 deletions

diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index 7453ac189..7b7fea934 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -188,8 +188,6 @@ void mkPath(Value & v, const char * s);
 void copyContext(const Value & v, PathSet & context);
 
 
-typedef std::map<Path, Hash> DrvHashes;
-
 /* Cache for calls to addToStore(); maps source paths to the store
    paths. */
 typedef std::map<Path, Path> SrcToStore;
@@ -203,8 +201,6 @@ std::ostream & operator << (std::ostream & str, const Value & v);
 class EvalState 
 {
 public:
-    DrvHashes drvHashes; /* normalised derivation hashes */
-
     SymbolTable symbols;
 
     const Symbol sWith, sOutPath, sDrvPath, sType, sMeta, sName,
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index e58f9265f..c01bd6e54 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -246,63 +246,6 @@ static void prim_trace(EvalState & state, Value * * args, Value & v)
  *************************************************************/
 
 
-static bool isFixedOutput(const Derivation & drv)
-{
-    return drv.outputs.size() == 1 &&
-        drv.outputs.begin()->first == "out" &&
-        drv.outputs.begin()->second.hash != "";
-}
-
-
-/* Returns the hash of a derivation modulo fixed-output
-   subderivations.  A fixed-output derivation is a derivation with one
-   output (`out') for which an expected hash and hash algorithm are
-   specified (using the `outputHash' and `outputHashAlgo'
-   attributes).  We don't want changes to such derivations to
-   propagate upwards through the dependency graph, changing output
-   paths everywhere.
-
-   For instance, if we change the url in a call to the `fetchurl'
-   function, we do not want to rebuild everything depending on it
-   (after all, (the hash of) the file being downloaded is unchanged).
-   So the *output paths* should not change.  On the other hand, the
-   *derivation paths* should change to reflect the new dependency
-   graph.
-
-   That's what this function does: it returns a hash which is just the
-   hash of the derivation ATerm, except that any input derivation
-   paths have been replaced by the result of a recursive call to this
-   function, and that for fixed-output derivations we return a hash of
-   its output path. */
-static Hash hashDerivationModulo(EvalState & state, Derivation drv)
-{
-    /* Return a fixed hash for fixed-output derivations. */
-    if (isFixedOutput(drv)) {
-        DerivationOutputs::const_iterator i = drv.outputs.begin();
-        return hashString(htSHA256, "fixed:out:"
-            + i->second.hashAlgo + ":"
-            + i->second.hash + ":"
-            + i->second.path);
-    }
-
-    /* For other derivations, replace the inputs paths with recursive
-       calls to this function.*/
-    DerivationInputs inputs2;
-    foreach (DerivationInputs::const_iterator, i, drv.inputDrvs) {
-        Hash h = state.drvHashes[i->first];
-        if (h.type == htUnknown) {
-            Derivation drv2 = derivationFromPath(i->first);
-            h = hashDerivationModulo(state, drv2);
-            state.drvHashes[i->first] = h;
-        }
-        inputs2[printHash(h)] = i->second;
-    }
-    drv.inputDrvs = inputs2;
-    
-    return hashString(htSHA256, unparseDerivation(drv));
-}
-
-
 /* Construct (as a unobservable side effect) a Nix derivation
    expression that performs the derivation described by the argument
    set.  Returns the original set extended with the following
@@ -497,11 +440,10 @@ static void prim_derivationStrict(EvalState & state, Value * * args, Value & v)
        (‘i->first == "out" ...’) doesn't affect the hash of the
        others.  Is that exploitable? */
     if (!fixedOnly) {
-        Hash h = hashDerivationModulo(state, drv);
+        Hash h = hashDerivationModulo(drv);
         foreach (DerivationOutputs::iterator, i, drv.outputs)
             if (i->second.path == "") {
-                Path outPath = makeStorePath("output:" + i->first, h,
-                    drvName + (i->first == "out" ? "" : "-" + i->first));
+                Path outPath = makeOutputPath(i->first, h, drvName);
                 drv.env[i->first] = outPath;
                 i->second.path = outPath;
             }
@@ -516,7 +458,7 @@ static void prim_derivationStrict(EvalState & state, Value * * args, Value & v)
     /* Optimisation, but required in read-only mode! because in that
        case we don't actually write store derivations, so we can't
        read them later. */
-    state.drvHashes[drvPath] = hashDerivationModulo(state, drv);
+    drvHashes[drvPath] = hashDerivationModulo(drv);
 
     state.mkAttrs(v, 1 + drv.outputs.size());
     mkString(*state.allocAttr(v, state.sDrvPath), drvPath, singleton<PathSet>("=" + drvPath));
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 4df62acea..26f9fcc59 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -1916,14 +1916,9 @@ void DerivationGoal::computeClosure()
            hash). */ 
         if (i->second.hash != "") {
 
-            bool recursive = false;
-            string algo = i->second.hashAlgo;
+            bool recursive; HashType ht; Hash h;
+            i->second.parseHashInfo(recursive, ht, h);
             
-            if (string(algo, 0, 2) == "r:") {
-                recursive = true;
-                algo = string(algo, 2);
-            }
-
             if (!recursive) {
                 /* The output path should be a regular file without
                    execute permission. */
@@ -1934,15 +1929,11 @@ void DerivationGoal::computeClosure()
             }
 
             /* Check the hash. */
-            HashType ht = parseHashType(algo);
-            if (ht == htUnknown)
-                throw BuildError(format("unknown hash algorithm `%1%'") % algo);
-            Hash h = parseHash(ht, i->second.hash);
             Hash h2 = recursive ? hashPath(ht, path).first : hashFile(ht, path);
             if (h != h2)
                 throw BuildError(
                     format("output path `%1%' should have %2% hash `%3%', instead has `%4%'")
-                    % path % algo % printHash(h) % printHash(h2));
+                    % path % i->second.hashAlgo % printHash(h) % printHash(h2));
         }
 
         /* Get rid of all weird permissions. */
diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
index e321ae8aa..db9fc6b8a 100644
--- a/src/libstore/derivations.cc
+++ b/src/libstore/derivations.cc
@@ -2,11 +2,30 @@
 #include "store-api.hh"
 #include "globals.hh"
 #include "util.hh"
+#include "misc.hh"
 
 
 namespace nix {
 
 
+void DerivationOutput::parseHashInfo(bool & recursive, HashType & hashType, Hash & hash) const
+{
+    recursive = false;
+    string algo = hashAlgo;
+        
+    if (string(algo, 0, 2) == "r:") {
+        recursive = true;
+        algo = string(algo, 2);
+    }
+
+    hashType = parseHashType(algo);
+    if (hashType == htUnknown)
+        throw Error(format("unknown hash algorithm `%1%'") % algo);
+        
+    hash = parseHash(hashType, this->hash);
+}
+
+
 Path writeDerivation(const Derivation & drv, const string & name)
 {
     PathSet references;
@@ -171,4 +190,64 @@ bool isDerivation(const string & fileName)
 }
 
  
+bool isFixedOutputDrv(const Derivation & drv)
+{
+    return drv.outputs.size() == 1 &&
+        drv.outputs.begin()->first == "out" &&
+        drv.outputs.begin()->second.hash != "";
+}
+
+
+DrvHashes drvHashes;
+
+
+/* Returns the hash of a derivation modulo fixed-output
+   subderivations.  A fixed-output derivation is a derivation with one
+   output (`out') for which an expected hash and hash algorithm are
+   specified (using the `outputHash' and `outputHashAlgo'
+   attributes).  We don't want changes to such derivations to
+   propagate upwards through the dependency graph, changing output
+   paths everywhere.
+
+   For instance, if we change the url in a call to the `fetchurl'
+   function, we do not want to rebuild everything depending on it
+   (after all, (the hash of) the file being downloaded is unchanged).
+   So the *output paths* should not change.  On the other hand, the
+   *derivation paths* should change to reflect the new dependency
+   graph.
+
+   That's what this function does: it returns a hash which is just the
+   hash of the derivation ATerm, except that any input derivation
+   paths have been replaced by the result of a recursive call to this
+   function, and that for fixed-output derivations we return a hash of
+   its output path. */
+Hash hashDerivationModulo(Derivation drv)
+{
+    /* Return a fixed hash for fixed-output derivations. */
+    if (isFixedOutputDrv(drv)) {
+        DerivationOutputs::const_iterator i = drv.outputs.begin();
+        return hashString(htSHA256, "fixed:out:"
+            + i->second.hashAlgo + ":"
+            + i->second.hash + ":"
+            + i->second.path);
+    }
+
+    /* For other derivations, replace the inputs paths with recursive
+       calls to this function.*/
+    DerivationInputs inputs2;
+    foreach (DerivationInputs::const_iterator, i, drv.inputDrvs) {
+        Hash h = drvHashes[i->first];
+        if (h.type == htUnknown) {
+            Derivation drv2 = derivationFromPath(i->first);
+            h = hashDerivationModulo(drv2);
+            drvHashes[i->first] = h;
+        }
+        inputs2[printHash(h)] = i->second;
+    }
+    drv.inputDrvs = inputs2;
+    
+    return hashString(htSHA256, unparseDerivation(drv));
+}
+
+
 }
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
index c14be48af..4860f708c 100644
--- a/src/libstore/derivations.hh
+++ b/src/libstore/derivations.hh
@@ -4,6 +4,7 @@
 #include <map>
 
 #include "types.hh"
+#include "hash.hh"
 
 
 namespace nix {
@@ -29,6 +30,7 @@ struct DerivationOutput
         this->hashAlgo = hashAlgo;
         this->hash = hash;
     }
+    void parseHashInfo(bool & recursive, HashType & hashType, Hash & hash) const;
 };
 
 typedef std::map<string, DerivationOutput> DerivationOutputs;
@@ -64,7 +66,16 @@ string unparseDerivation(const Derivation & drv);
    derivations. */
 bool isDerivation(const string & fileName);
 
- 
+/* Return true iff this is a fixed-output derivation. */
+bool isFixedOutputDrv(const Derivation & drv);
+
+Hash hashDerivationModulo(Derivation drv);
+
+/* Memoisation of hashDerivationModulo(). */
+typedef std::map<Path, Hash> DrvHashes;
+
+extern DrvHashes drvHashes;
+
 }
 
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 6af34cc77..691069e2b 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -469,6 +469,47 @@ void canonicalisePathMetaData(const Path & path)
 }
 
 
+void LocalStore::checkDerivationOutputs(const Path & drvPath, const Derivation & drv)
+{
+    string drvName = storePathToName(drvPath);
+    assert(isDerivation(drvName));
+    drvName = string(drvName, 0, drvName.size() - drvExtension.size());
+        
+    if (isFixedOutputDrv(drv)) {
+        DerivationOutputs::const_iterator out = drv.outputs.find("out");
+        if (out == drv.outputs.end())
+            throw Error(format("derivation `%1%' does not have an output named `out'") % drvPath);
+
+        bool recursive; HashType ht; Hash h;
+        out->second.parseHashInfo(recursive, ht, h);
+        Path outPath = makeFixedOutputPath(recursive, ht, h, drvName);
+
+        StringPairs::const_iterator j = drv.env.find("out");
+        if (out->second.path != outPath || j == drv.env.end() || j->second != outPath)
+            throw Error(format("derivation `%1%' has incorrect output `%2%', should be `%3%'")
+                % drvPath % out->second.path % outPath);
+    }
+
+    else {
+        Derivation drvCopy(drv);
+        foreach (DerivationOutputs::iterator, i, drvCopy.outputs) {
+            i->second.path = "";
+            drvCopy.env[i->first] = "";
+        }
+
+        Hash h = hashDerivationModulo(drvCopy);
+        
+        foreach (DerivationOutputs::const_iterator, i, drv.outputs) {
+            Path outPath = makeOutputPath(i->first, h, drvName);
+            StringPairs::const_iterator j = drv.env.find(i->first);
+            if (i->second.path != outPath || j == drv.env.end() || j->second != outPath)
+                throw Error(format("derivation `%1%' has incorrect output `%2%', should be `%3%'")
+                    % drvPath % i->second.path % outPath);
+        }
+    }
+}
+
+
 unsigned long long LocalStore::addValidPath(const ValidPathInfo & info)
 {
     SQLiteStmtUse use(stmtRegisterValidPath);
@@ -493,6 +534,14 @@ unsigned long long LocalStore::addValidPath(const ValidPathInfo & info)
        derivation. */
     if (isDerivation(info.path)) {
         Derivation drv = parseDerivation(readFile(info.path));
+        
+        /* Verify that the output paths in the derivation are correct
+           (i.e., follow the scheme for computing output paths from
+           derivations).  Note that if this throws an error, then the
+           DB transaction is rolled back, so the path validity
+           registration above is undone. */
+        checkDerivationOutputs(info.path, drv);
+        
         foreach (DerivationOutputs::iterator, i, drv.outputs) {
             SQLiteStmtUse use(stmtAddDerivationOutput);
             stmtAddDerivationOutput.bind(id);
@@ -925,6 +974,8 @@ void LocalStore::invalidatePath(const Path & path)
 {
     debug(format("invalidating path `%1%'") % path);
 
+    drvHashes.erase(path);
+
     SQLiteStmtUse use(stmtInvalidatePath);
 
     stmtInvalidatePath.bind(path);
diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh
index f270fb723..8cf6b6640 100644
--- a/src/libstore/local-store.hh
+++ b/src/libstore/local-store.hh
@@ -25,6 +25,9 @@ const int nixSchemaVersion = 6;
 extern string drvsLogDir;
 
 
+struct Derivation;
+
+
 struct OptimiseStats
 {
     unsigned long totalFiles;
@@ -255,6 +258,8 @@ private:
         RunningSubstituter & runningSubstituter);
 
     Path createTempDirInStore();
+
+    void checkDerivationOutputs(const Path & drvPath, const Derivation & drv);
 };
 
 
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 4b04f5751..d67ff2c77 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -72,6 +72,13 @@ Path followLinksToStorePath(const Path & path)
 }
 
 
+string storePathToName(const Path & path)
+{
+    assertStorePath(path);
+    return string(path, nixStore.size() + 34);
+}
+
+
 void checkStoreName(const string & name)
 {
     string validChars = "+-._?=";
@@ -101,7 +108,9 @@ void checkStoreName(const string & name)
    
    <name> = a human readable name for the path, typically obtained
      from the name attribute of the derivation, or the name of the
-     source file from which the store path is created
+     source file from which the store path is created.  For derivation
+     outputs other than the default "out" output, the string "-<id>"
+     is suffixed to <name>.
      
    <h> = base-32 representation of the first 160 bits of a SHA-256
      hash of <s>; the hash part of the store name
@@ -120,11 +129,12 @@ void checkStoreName(const string & name)
      "source"
        for paths copied to the store using addToStore() when recursive
        = true and hashAlgo = "sha256"
-     "output:out"
+     "output:<id>"
        for either the outputs created by derivations, OR paths copied
        to the store using addToStore() with recursive != true or
        hashAlgo != "sha256" (in that case "source" is used; it's
-       silly, but it's done that way for compatibility).
+       silly, but it's done that way for compatibility).  <id> is the
+       name of the output (usually, "out").
 
    <h2> = base-16 representation of a SHA-256 hash of:
      if <type> = "text:...":
@@ -174,6 +184,14 @@ Path makeStorePath(const string & type,
 }
 
 
+Path makeOutputPath(const string & id,
+    const Hash & hash, const string & name)
+{
+    return makeStorePath("output:" + id, hash,
+        name + (id == "out" ? "" : "-" + id));
+}
+
+
 Path makeFixedOutputPath(bool recursive,
     HashType hashAlgo, Hash hash, string name)
 {
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 40ac88714..243a6324c 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -250,6 +250,9 @@ void assertStorePath(const Path & path);
 bool isInStore(const Path & path);
 bool isStorePath(const Path & path);
 
+/* Extract the name part of the given store path. */
+string storePathToName(const Path & path);
+    
 void checkStoreName(const string & name);
 
 
@@ -271,6 +274,9 @@ Path followLinksToStorePath(const Path & path);
 Path makeStorePath(const string & type,
     const Hash & hash, const string & name);
     
+Path makeOutputPath(const string & id,
+    const Hash & hash, const string & name);
+
 Path makeFixedOutputPath(bool recursive,
     HashType hashAlgo, Hash hash, string name);