CVE-2018-7408: Incorrect Permission Assignment for Critical Resource (CWE-732)
value: An issue was discovered in an npm 5.7.0 2018-02-21 pre-release (marked as "next: 5.7.0" and therefore automatically installed by an "npm upgrade -g npm" command, and also announced in the vendor's blog without mention of pre-release status). It might allow local users to bypass intended filesystem access restrictions because ownerships of /etc and /usr directories are being changed unexpectedly, related to a "correctMkdir" issue.
CWE description: The product specifies permissions for a security-critical resource in a way that allows that resource to be read or modified by unintended actors.
CVSS v3 base score: 7.8
Published: 2018-02-22T18:29Z
Repository: https://github.com/npm/npm
Language: JavaScript
Files changed: lib/config/core.js, lib/config/set-user.js, lib/install.js, lib/install/action/extract.js, lib/install/action/finalize.js, lib/install/action/move.js, lib/install/action/remove.js, lib/utils/completion/file-completion.js, lib/utils/correct-mkdir.js
Commit date: 2018-02-22 09:00:45 -0800
Commit message: Revert "*: Switch from mkdirp to correctMkdir to preserve perms and owners"

This reverts commit 94227e15eeced836b3d7b3d2b5e5cc41d4959cff.

Fixes: #19883

Diff: diff --git a/lib/config/core.js b/lib/config/core.js
index c2fe1d17f..50cf4772e 100644
--- a/lib/config/core.js
+++ b/lib/config/core.js
@@ -6,11 +6,11 @@ var once = require('once')
 var fs = require('fs')
 var path = require('path')
 var nopt = require('nopt')
 var ini = require('ini')
 var Umask = configDefs.Umask
-var correctMkdir = require('../utils/correct-mkdir.js')
+var mkdirp = require('mkdirp')
 var umask = require('../utils/umask')
 var isWindows = require('../utils/is-windows.js')
 
 exports.load = load
 exports.Conf = Conf
@@ -151,11 +151,11 @@ function load_ (builtin, rc, cli, cb) {
     // Eg, `npm config get globalconfig --prefix ~/local` should
     // return `~/local/etc/npmrc`
     // annoying humans and their expectations!
     if (conf.get('prefix')) {
       var etc = path.resolve(conf.get('prefix'), 'etc')
-      correctMkdir(etc, function () {
+      mkdirp(etc, function () {
         defaults.globalconfig = path.resolve(etc, 'npmrc')
         defaults.globalignorefile = path.resolve(etc, 'npmignore')
         afterUserContinuation()
       })
     } else {
@@ -233,11 +233,11 @@ Conf.prototype.loadExtras = function (cb) {
   this.setUser(function (er) {
     if (er) return cb(er)
     this.loadUid(function (er) {
       if (er) return cb(er)
       // Without prefix, nothing will ever work
-      correctMkdir(this.prefix, cb)
+      mkdirp(this.prefix, cb)
     }.bind(this))
   }.bind(this))
 }
 
 Conf.prototype.save = function (where, cb) {
@@ -290,11 +290,11 @@ Conf.prototype.save = function (where, cb) {
     fs.unlink(target.path, function () {
       // ignore the possible error (e.g. the file doesn't exist)
       done(null)
     })
   } else {
-    correctMkdir(path.dirname(target.path), function (er) {
+    mkdirp(path.dirname(target.path), function (er) {
       if (er) return then(er)
       fs.writeFile(target.path, data, 'utf8', function (er) {
         if (er) return then(er)
         if (where === 'user' && myUid && myGid) {
           fs.chown(target.path, +myUid, +myGid, then)
diff --git a/lib/config/set-user.js b/lib/config/set-user.js
index 987e552d9..14cc21d2e 100644
--- a/lib/config/set-user.js
+++ b/lib/config/set-user.js
@@ -1,11 +1,11 @@
 module.exports = setUser
 
 var assert = require('assert')
 var path = require('path')
 var fs = require('fs')
-var correctMkdir = require('../utils/correct-mkdir.js')
+var mkdirp = require('mkdirp')
 
 function setUser (cb) {
   var defaultConf = this.root
   assert(defaultConf !== Object.prototype)
 
@@ -17,11 +17,11 @@ function setUser (cb) {
     defaultConf.user = +(process.env.SUDO_UID)
     return cb()
   }
 
   var prefix = path.resolve(this.get('prefix'))
-  correctMkdir(prefix, function (er) {
+  mkdirp(prefix, function (er) {
     if (er) return cb(er)
     fs.stat(prefix, function (er, st) {
       defaultConf.user = st && st.uid
       return cb(er)
     })
diff --git a/lib/install.js b/lib/install.js
index 0e37ba3d2..7a7d75c90 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -100,11 +100,11 @@ var log = require('npmlog')
 var readPackageTree = require('read-package-tree')
 var readPackageJson = require('read-package-json')
 var chain = require('slide').chain
 var asyncMap = require('slide').asyncMap
 var archy = require('archy')
-var correctMkdir = require('./utils/correct-mkdir.js')
+var mkdirp = require('mkdirp')
 var rimraf = require('rimraf')
 var iferr = require('iferr')
 var validate = require('aproba')
 var uniq = require('lodash.uniq')
 var Bluebird = require('bluebird')
@@ -646,11 +646,11 @@ Installer.prototype.saveToDependencies = function (cb) {
 Installer.prototype.readGlobalPackageData = function (cb) {
   validate('F', arguments)
   log.silly('install', 'readGlobalPackageData')
   var self = this
   this.loadArgMetadata(iferr(cb, function () {
-    correctMkdir(self.where, iferr(cb, function () {
+    mkdirp(self.where, iferr(cb, function () {
       var pkgs = {}
       self.args.forEach(function (pkg) {
         pkgs[pkg.name] = true
       })
       readPackageTree(self.where, function (ctx, kid) { return ctx.parent || pkgs[kid] }, iferr(cb, function (currentTree) {
@@ -663,11 +663,11 @@ Installer.prototype.readGlobalPackageData = function (cb) {
 
 Installer.prototype.readLocalPackageData = function (cb) {
   validate('F', arguments)
   log.silly('install', 'readLocalPackageData')
   var self = this
-  correctMkdir(this.where, iferr(cb, function () {
+  mkdirp(this.where, iferr(cb, function () {
     readPackageTree(self.where, iferr(cb, function (currentTree) {
       self.currentTree = currentTree
       self.currentTree.warnings = []
       if (currentTree.error && currentTree.error.code === 'EJSONPARSE') {
         return cb(currentTree.error)
diff --git a/lib/install/action/extract.js b/lib/install/action/extract.js
index a9260cc50..e8d7a6c4f 100644
--- a/lib/install/action/extract.js
+++ b/lib/install/action/extract.js
@@ -2,11 +2,11 @@
 
 const BB = require('bluebird')
 
 const stat = BB.promisify(require('graceful-fs').stat)
 const gentlyRm = BB.promisify(require('../../utils/gently-rm.js'))
-const correctMkdir = BB.promisify(require('../../utils/correct-mkdir'))
+const mkdirp = BB.promisify(require('mkdirp'))
 const moduleStagingPath = require('../module-staging-path.js')
 const move = require('../../utils/move.js')
 const npa = require('npm-package-arg')
 const npm = require('../../npm.js')
 const packageId = require('../../utils/package-id.js')
@@ -125,11 +125,11 @@ function stageBundledModule (bundler, child, staging, parentPath) {
 }
 
 function finishModule (bundler, child, stageTo, stageFrom) {
   // If we were the one's who bundled this module…
   if (child.fromBundle === bundler) {
-    return correctMkdir(path.dirname(stageTo)).then(() => {
+    return mkdirp(path.dirname(stageTo)).then(() => {
       return move(stageFrom, stageTo)
     })
   } else {
     return stat(stageFrom).then(() => gentlyRm(stageFrom), () => {})
   }
diff --git a/lib/install/action/finalize.js b/lib/install/action/finalize.js
index dc08ec06f..e46f1b9d8 100644
--- a/lib/install/action/finalize.js
+++ b/lib/install/action/finalize.js
@@ -1,21 +1,21 @@
 'use strict'
 const path = require('path')
 const fs = require('graceful-fs')
 const Bluebird = require('bluebird')
 const rimraf = Bluebird.promisify(require('rimraf'))
+const mkdirp = Bluebird.promisify(require('mkdirp'))
 const lstat = Bluebird.promisify(fs.lstat)
 const readdir = Bluebird.promisify(fs.readdir)
 const symlink = Bluebird.promisify(fs.symlink)
 const gentlyRm = Bluebird.promisify(require('../../utils/gently-rm'))
 const moduleStagingPath = require('../module-staging-path.js')
 const move = require('move-concurrently')
 const moveOpts = {fs: fs, Promise: Bluebird, maxConcurrency: 4}
 const getRequested = require('../get-requested.js')
 const log = require('npmlog')
 const packageId = require('../../utils/package-id.js')
-const correctMkdir = Bluebird.promisify(require('../../utils/correct-mkdir.js'))
 
 module.exports = function (staging, pkg, log) {
   log.silly('finalize', pkg.realpath)
 
   const extractedTo = moduleStagingPath(staging, pkg)
@@ -46,11 +46,11 @@ module.exports = function (staging, pkg, log) {
       })
       .then(() => rimraf(delpath))
   }
 
   function makeParentPath (dir) {
-    return correctMkdir(path.dirname(dir))
+    return mkdirp(path.dirname(dir))
   }
 
   function moveStagingToDestination () {
     return destinationIsClear()
       .then(actuallyMoveStaging)
@@ -79,11 +79,11 @@ module.exports = function (staging, pkg, log) {
 
   function restoreOldNodeModules () {
     if (!movedDestAway) return
     return readdir(path.join(delpath, 'node_modules')).catch(() => []).then((modules) => {
       if (!modules.length) return
-      return correctMkdir(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
+      return mkdirp(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
         const from = path.join(delpath, 'node_modules', file)
         const to = path.join(pkg.realpath, 'node_modules', file)
         return move(from, to, moveOpts)
       }))
     })
diff --git a/lib/install/action/move.js b/lib/install/action/move.js
index 084ab6127..00d58a159 100644
--- a/lib/install/action/move.js
+++ b/lib/install/action/move.js
@@ -2,11 +2,11 @@
 var fs = require('graceful-fs')
 var path = require('path')
 var chain = require('slide').chain
 var iferr = require('iferr')
 var rimraf = require('rimraf')
-var correctMkdir = require('../../utils/correct-mkdir')
+var mkdirp = require('mkdirp')
 var rmStuff = require('../../unbuild.js').rmStuff
 var lifecycle = require('../../utils/lifecycle.js')
 var move = require('../../utils/move.js')
 
 /*
@@ -65,11 +65,11 @@ function moveModuleOnly (from, to, log, done) {
   }
 
   function makeDestination (next) {
     return function () {
       log.silly('move', 'make sure destination parent exists', path.resolve(to, '..'))
-      correctMkdir(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
+      mkdirp(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
     }
   }
 
   function moveNodeModules (next) {
     return function () {
@@ -85,11 +85,11 @@ function moveModuleOnly (from, to, log, done) {
     }
   }
 
   function moveNodeModulesBack (next) {
     return function () {
-      correctMkdir(from, iferr(done, function () {
+      mkdirp(from, iferr(done, function () {
         log.silly('move', 'put source node_modules back', fromModules)
         move(tempFromModules, fromModules).then(next, done)
       }))
     }
   }
diff --git a/lib/install/action/remove.js b/lib/install/action/remove.js
index 2e2f378d2..a852d10c5 100644
--- a/lib/install/action/remove.js
+++ b/lib/install/action/remove.js
@@ -1,11 +1,11 @@
 'use strict'
 var path = require('path')
 var fs = require('graceful-fs')
 var rimraf = require('rimraf')
 var asyncMap = require('slide').asyncMap
-var correctMkdir = require('../../utils/correct-mkdir')
+var mkdirp = require('mkdirp')
 var npm = require('../../npm.js')
 var andIgnoreErrors = require('../and-ignore-errors.js')
 var move = require('../../utils/move.js')
 var isInside = require('path-is-inside')
 var vacuum = require('fs-vacuum')
@@ -50,11 +50,11 @@ function removeDir (pkg, log, next) {
   }
 
   function makeTarget (readdirEr, files) {
     if (readdirEr) return cleanup()
     if (!files.length) return cleanup()
-    correctMkdir(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
+    mkdirp(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
   }
 
   function moveModules (mkdirEr, files) {
     if (mkdirEr) return next(mkdirEr)
     asyncMap(files, function (file, done) {
diff --git a/lib/utils/completion/file-completion.js b/lib/utils/completion/file-completion.js
index fdbb5a0bf..e16b636a6 100644
--- a/lib/utils/completion/file-completion.js
+++ b/lib/utils/completion/file-completion.js
@@ -1,16 +1,16 @@
 module.exports = fileCompletion
 
-var correctMkdir = require('../correct-mkdir.js')
+var mkdir = require('mkdirp')
 var glob = require('glob')
 
 function fileCompletion (root, req, depth, cb) {
   if (typeof cb !== 'function') {
     cb = depth
     depth = Infinity
   }
-  correctMkdir(root, function (er) {
+  mkdir(root, function (er) {
     if (er) return cb(er)
 
     // can be either exactly the req, or a descendent
     var pattern = root + '/{' + req + ',' + req + '/**/*}'
     var opts = { mark: true, dot: true, maxDepth: depth }
diff --git a/lib/utils/correct-mkdir.js b/lib/utils/correct-mkdir.js
index a0139d6f9..68c4a4ad7 100644
--- a/lib/utils/correct-mkdir.js
+++ b/lib/utils/correct-mkdir.js
@@ -4,25 +4,25 @@ var fs = require('graceful-fs')
 var inflight = require('inflight')
 var log = require('npmlog')
 var mkdirp = require('mkdirp')
 
 // memoize the directories created by this step
+var stats = {}
 var effectiveOwner
 module.exports = function correctMkdir (path, cb) {
   cb = dezalgo(cb)
   cb = inflight('correctMkdir:' + path, cb)
   if (!cb) {
-    return log.silly('correctMkdir', path, 'correctMkdir already in flight; waiting')
+    return log.verbose('correctMkdir', path, 'correctMkdir already in flight; waiting')
   } else {
-    log.silly('correctMkdir', path, 'correctMkdir not in flight; initializing')
+    log.verbose('correctMkdir', path, 'correctMkdir not in flight; initializing')
   }
-  var stats = {}
 
   if (stats[path]) return cb(null, stats[path])
 
   fs.stat(path, function (er, st) {
-    if (er) return makeDirectory(path, stats, cb)
+    if (er) return makeDirectory(path, cb)
 
     if (!st.isDirectory()) {
       log.error('correctMkdir', 'invalid dir %s', path)
       return cb(er)
     }
@@ -58,16 +58,16 @@ function calculateOwner () {
   }
 
   return effectiveOwner
 }
 
-function makeDirectory (path, stats, cb) {
+function makeDirectory (path, cb) {
   cb = inflight('makeDirectory:' + path, cb)
   if (!cb) {
-    return log.silly('makeDirectory', path, 'creation already in flight; waiting')
+    return log.verbose('makeDirectory', path, 'creation already in flight; waiting')
   } else {
-    log.silly('makeDirectory', path, 'creation not in flight; initializing')
+    log.verbose('makeDirectory', path, 'creation not in flight; initializing')
   }
 
   var owner = calculateOwner()
 
   if (!process.getuid) {

Vulnerable code: var correctMkdir = require('../utils/correct-mkdir.js')
      correctMkdir(etc, function () {
      correctMkdir(this.prefix, cb)
    correctMkdir(path.dirname(target.path), function (er) {
var correctMkdir = require('../utils/correct-mkdir.js')
  correctMkdir(prefix, function (er) {
var correctMkdir = require('./utils/correct-mkdir.js')
    correctMkdir(self.where, iferr(cb, function () {
  correctMkdir(this.where, iferr(cb, function () {
const correctMkdir = BB.promisify(require('../../utils/correct-mkdir'))
    return correctMkdir(path.dirname(stageTo)).then(() => {
const correctMkdir = Bluebird.promisify(require('../../utils/correct-mkdir.js'))
    return correctMkdir(path.dirname(dir))
      return correctMkdir(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
var correctMkdir = require('../../utils/correct-mkdir')
      correctMkdir(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
      correctMkdir(from, iferr(done, function () {
var correctMkdir = require('../../utils/correct-mkdir')
    correctMkdir(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
var correctMkdir = require('../correct-mkdir.js')
  correctMkdir(root, function (er) {
    return log.silly('correctMkdir', path, 'correctMkdir already in flight; waiting')
    log.silly('correctMkdir', path, 'correctMkdir not in flight; initializing')
  var stats = {}
    if (er) return makeDirectory(path, stats, cb)
function makeDirectory (path, stats, cb) {
    return log.silly('makeDirectory', path, 'creation already in flight; waiting')
    log.silly('makeDirectory', path, 'creation not in flight; initializing')
Fixed code: var mkdirp = require('mkdirp')
      mkdirp(etc, function () {
      mkdirp(this.prefix, cb)
    mkdirp(path.dirname(target.path), function (er) {
var mkdirp = require('mkdirp')
  mkdirp(prefix, function (er) {
var mkdirp = require('mkdirp')
    mkdirp(self.where, iferr(cb, function () {
  mkdirp(this.where, iferr(cb, function () {
const mkdirp = BB.promisify(require('mkdirp'))
    return mkdirp(path.dirname(stageTo)).then(() => {
const mkdirp = Bluebird.promisify(require('mkdirp'))
    return mkdirp(path.dirname(dir))
      return mkdirp(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
var mkdirp = require('mkdirp')
      mkdirp(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
      mkdirp(from, iferr(done, function () {
var mkdirp = require('mkdirp')
    mkdirp(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
var mkdir = require('mkdirp')
  mkdir(root, function (er) {
var stats = {}
    return log.verbose('correctMkdir', path, 'correctMkdir already in flight; waiting')
    log.verbose('correctMkdir', path, 'correctMkdir not in flight; initializing')
    if (er) return makeDirectory(path, cb)
function makeDirectory (path, cb) {
    return log.verbose('makeDirectory', path, 'creation already in flight; waiting')
    log.verbose('makeDirectory', path, 'creation not in flight; initializing')