efficiently validate tmp tarballs safely

Only validate tarballs when necessary, validate them locally, and make
sure that each one that's being validated is being unpacked to a
different directory. Fixes #6329.

1 files changed, 54 insertions, 85 deletions

diff --git a/lib/cache/add-local-tarball.js b/lib/cache/add-local-tarball.js
index 8bf676cc9..902f3e815 100644
--- a/lib/cache/add-local-tarball.js
+++ b/lib/cache/add-local-tarball.js
@@ -2,10 +2,10 @@ var mkdir = require("mkdirp")
   , assert = require("assert")
   , fs = require("graceful-fs")
   , writeFileAtomic = require("write-file-atomic")
-  , readJson = require("read-package-json")
   , path = require("path")
   , sha = require("sha")
   , npm = require("../npm.js")
+  , log = require("npmlog")
   , tar = require("../utils/tar.js")
   , pathIsInside = require("path-is-inside")
   , getCacheStat = require("./get-stat.js")
@@ -13,62 +13,39 @@ var mkdir = require("mkdirp")
   , inflight = require("inflight")
   , once = require("once")
   , writeStream = require("fs-write-stream-atomic")
+  , randomBytes = require("crypto").pseudoRandomBytes // only need uniqueness
 
 module.exports = addLocalTarball
 
-function addLocalTarball (p, pkgData, shasum, cb_) {
+function addLocalTarball (p, pkgData, shasum, cb) {
   assert(typeof p === "string", "must have path")
-  assert(typeof cb_ === "function", "must have callback")
+  assert(typeof cb === "function", "must have callback")
 
   if (!pkgData) pkgData = {}
-  var name = pkgData.name || ""
 
-  // If we don't have a shasum yet, then get the shasum now.
+  // If we don't have a shasum yet, compute it.
   if (!shasum) {
     return sha.get(p, function (er, shasum) {
-      if (er) return cb_(er)
-      addLocalTarball(p, pkgData, shasum, cb_)
+      if (er) return cb(er)
+      log.silly("addLocalTarball", "shasum (computed)", shasum)
+      addLocalTarball(p, pkgData, shasum, cb)
     })
   }
 
-  // if it's a tar, and not in place,
-  // then unzip to .tmp, add the tmp folder, and clean up tmp
-  if (pathIsInside(p, npm.tmp))
-    return addTmpTarball(p, pkgData, shasum, cb_)
-
   if (pathIsInside(p, npm.cache)) {
-    if (path.basename(p) !== "package.tgz") return cb_(new Error(
-      "Not a valid cache tarball name: "+p))
-    return addPlacedTarball(p, pkgData, shasum, cb_)
+    if (path.basename(p) !== "package.tgz") {
+      return cb(new Error("Not a valid cache tarball name: "+p))
+    }
+    log.verbose("addLocalTarball", "adding from inside cache", p)
+    return addPlacedTarball(p, pkgData, shasum, cb)
   }
 
-  function cb (er, data) {
+  addTmpTarball(p, pkgData, shasum, function (er, data) {
     if (data) {
       data._resolved = p
       data._shasum = data._shasum || shasum
     }
-    return cb_(er, data)
-  }
-
-  // just copy it over and then add the temp tarball file.
-  var tmp = path.join(npm.tmp, name + Date.now()
-                             + "-" + Math.random(), "tmp.tgz")
-  mkdir(path.dirname(tmp), function (er) {
-    if (er) return cb(er)
-    var from = fs.createReadStream(p)
-      , to = writeStream(tmp, { mode: npm.modes.file })
-      , errState = null
-    function errHandler (er) {
-      if (errState) return
-      cb(errState = er)
-    }
-    from.on("error", errHandler)
-    to.on("error", errHandler)
-    to.on("close", function () {
-      if (errState) return
-      addTmpTarball(tmp, pkgData, shasum, cb)
-    })
-    from.pipe(to)
+    return cb(er, data)
   })
 }
 
@@ -109,54 +86,58 @@ function addPlacedTarball_ (p, pkgData, uid, gid, resolvedSum, cb) {
 
 function addTmpTarball (tgz, pkgData, shasum, cb) {
   assert(typeof cb === "function", "must have callback function")
-  assert(shasum, "should have shasum by now")
+  assert(shasum, "must have shasum by now")
 
   cb = inflight("addTmpTarball:" + tgz, cb)
-  if (!cb) return
+  if (!cb) return undefined
 
   // we already have the package info, so just move into place
   if (pkgData && pkgData.name && pkgData.version) {
-    addTmpTarball_(tgz, pkgData, shasum, cb)
+    log.verbose("addTmpTarball", "already have metadata; skipping unpack")
+    return addTmpTarball_(tgz, pkgData, shasum, cb)
   }
 
-  // This is a tarball we probably downloaded from the internet.
-  // The shasum's already been checked, but we haven't ever had
-  // a peek inside, so we unpack it here just to make sure it is
-  // what it says it is.
-  // Note: we might not have any clue what we think it is, for
-  // example if the user just did `npm install ./foo.tgz`
-
-  var target = tgz + "-unpack"
-  getCacheStat(function (er, cs) {
-    tar.unpack(tgz, target, null, null, cs.uid, cs.gid, next)
-  })
+  // This is a tarball we probably downloaded from the internet.  The shasum's
+  // already been checked, but we haven't ever had a peek inside, so we unpack
+  // it here just to make sure it is what it says it is.
+  //
+  // NOTE: we might not have any clue what we think it is, for example if the
+  // user just did `npm install ./foo.tgz`
 
-  function next (er) {
+  // generate a unique filename
+  randomBytes(6, function (er, random) {
     if (er) return cb(er)
-    var pj = path.join(target, "package.json")
-    readJson(pj, function (er, data) {
-      // XXX dry with similar stanza in add-local.js
-      er = needName(er, data)
-      er = needVersion(er, data)
-      // check that this is what we expected.
-      if (!er && pkgData.name && pkgData.name !== data.name) {
-        er = new Error( "Invalid Package: expected "
-                      + pkgData.name + " but found "
-                      + data.name )
-      }
-
-      if (!er && pkgData.version && pkgData.version !== data.version) {
-        er = new Error( "Invalid Package: expected "
-                      + pkgData.name + "@" + pkgData.version
-                      + " but found "
-                      + data.name + "@" + data.version )
-      }
 
+    var target = path.join(npm.tmp, "unpack-" + random.toString("hex"))
+    getCacheStat(function (er, cs) {
       if (er) return cb(er)
 
-      addTmpTarball_(tgz, data, shasum, cb)
+      log.verbose("addTmpTarball", "validating metadata from", tgz)
+      tar.unpack(tgz, target, null, null, cs.uid, cs.gid, function (er, data) {
+        if (er) return cb(er)
+
+        // check that this is what we expected.
+        if (!data.name) {
+          return cb(new Error("No name provided"))
+        }
+        else if (pkgData.name && data.name !== pkgData.name) {
+          return cb(new Error("Invalid Package: expected " + pkgData.name +
+                              " but found " + data.name))
+        }
+
+        if (!data.version) {
+          return cb(new Error("No version provided"))
+        }
+        else if (pkgData.version && data.version !== pkgData.version) {
+          return cb(new Error("Invalid Package: expected " +
+                              pkgData.name + "@" + pkgData.version +
+                              " but found " + data.name + "@" + data.version))
+        }
+
+        addTmpTarball_(tgz, data, shasum, cb)
+      })
     })
-  }
+  })
 }
 
 function addTmpTarball_ (tgz, data, shasum, cb) {
@@ -196,15 +177,3 @@ function addTmpTarball_ (tgz, data, shasum, cb) {
     cb(null, data)
   }
 }
-
-function needName(er, data) {
-  return er ? er
-       : (data && !data.name) ? new Error("No name provided")
-       : null
-}
-
-function needVersion(er, data) {
-  return er ? er
-       : (data && !data.version) ? new Error("No version provided")
-       : null
-}