fix: prevent path escape using drive-relative paths

On Windows, a path like `c:foo` is not considered "absolute", but if the
cwd it's being resolved against is on a different drive letter, then
`resolve(cwd, path)` will not end up contained within `cwd`, even in the
absence of `..` portions.

This change strips path roots from all paths prior to being resolved
against the extraction target folder, even if such paths are not
"absolute".

Additionally, a path starting with a drive letter and then two dots,
like `c:../`, would bypass the check for `..` path portions.  This is
now being checked properly.

Finally, a defense in depth check is added, such that if the
entry.absolute is outside of the extraction taret, and we are not in
preservePaths:true mode, a warning is raised on that entry, and it is
skipped.  Currently, it is believed that this check is redundant, but it
did catch some oversights in development.

Author: isaacs

lib/strip-absolute-path.js

@@ -2,13 +2,23 @@
 const { isAbsolute, parse } = require('path').win32
 
 // returns [root, stripped]
+// Note that windows will think that //x/y/z/a has a "root" of //x/y, and in
+// those cases, we want to sanitize it to x/y/z/a, not z/a, so we strip /
+// explicitly if it's the first character.
+// drive-specific relative paths on Windows get their root stripped off even
+// though they are not absolute, so `c:../foo` becomes ['c:', '../foo']
 module.exports = path => {
   let r = ''
-  while (isAbsolute(path)) {
+
+  let parsed = parse(path)
+  while (isAbsolute(path) || parsed.root) {
     // windows will think that //x/y/z has a "root" of //x/y/
-    const root = path.charAt(0) === '/' ? '/' : parse(path).root
+    // but strip the //?/C:/ off of //?/C:/path
+    const root = path.charAt(0) === '/' && path.slice(0, 4) !== '//?/' ? '/'
+      : parsed.root
     path = path.substr(root.length)
     r += root
+    parsed = parse(path)
   }
   return [r, path]
 }

lib/unpack.js

@@ -247,16 +247,16 @@ class Unpack extends Parser {
 
     if (!this.preservePaths) {
       const p = normPath(entry.path)
-      if (p.split('/').includes('..')) {
+      const parts = p.split('/')
+      if (parts.includes('..') || isWindows && /^[a-z]:\.\.$/i.test(parts[0])) {
         this.warn('TAR_ENTRY_ERROR', `path contains '..'`, {
           entry,
           path: p,
         })
         return false
       }
 
-      // absolutes on posix are also absolutes on win32
-      // so we only need to test this one to get both
+      // strip off the root
       const [root, stripped] = stripAbsolutePath(p)
       if (root) {
         entry.path = stripped
@@ -272,6 +272,22 @@ class Unpack extends Parser {
     else
       entry.absolute = normPath(path.resolve(this.cwd, entry.path))
 
+    // if we somehow ended up with a path that escapes the cwd, and we are
+    // not in preservePaths mode, then something is fishy!  This should have
+    // been prevented above, so ignore this for coverage.
+    /* istanbul ignore if - defense in depth */
+    if (!this.preservePaths &&
+        entry.absolute.indexOf(this.cwd + '/') !== 0 &&
+        entry.absolute !== this.cwd) {
+      this.warn('TAR_ENTRY_ERROR', 'path escaped extraction target', {
+        entry,
+        path: normPath(entry.path),
+        resolvedPath: entry.absolute,
+        cwd: this.cwd,
+      })
+      return false
+    }
+
     // an archive can set properties on the extraction directory, but it
     // may not replace the cwd with a different kind of thing entirely.
     if (entry.absolute === this.cwd &&

test/strip-absolute-path.js

@@ -1,14 +1,51 @@
 const t = require('tap')
 const stripAbsolutePath = require('../lib/strip-absolute-path.js')
+const cwd = process.cwd()
 
-const cases = {
-  '/': ['/', ''],
-  '////': ['////', ''],
-  'c:///a/b/c': ['c:///', 'a/b/c'],
-  '\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
-  '//foo//bar//baz': ['//', 'foo//bar//baz'],
-  'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
-}
+t.test('basic', t => {
+  const cases = {
+    '/': ['/', ''],
+    '////': ['////', ''],
+    'c:///a/b/c': ['c:///', 'a/b/c'],
+    '\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
+    '//foo//bar//baz': ['//', 'foo//bar//baz'],
+    'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
+  }
 
-for (const [input, [root, stripped]] of Object.entries(cases))
-  t.strictSame(stripAbsolutePath(input), [root, stripped], input)
+  for (const [input, [root, stripped]] of Object.entries(cases))
+    t.strictSame(stripAbsolutePath(input, cwd), [root, stripped], input)
+  t.end()
+})
+
+t.test('drive-local paths', t => {
+  const env = process.env
+  t.teardown(() => process.env = env)
+  const cwd = 'D:\\safety\\land'
+  const realPath = require('path')
+  // be windowsy
+  const path = {
+    ...realPath.win32,
+    win32: realPath.win32,
+    posix: realPath.posix,
+  }
+  const stripAbsolutePath = t.mock('../lib/strip-absolute-path.js', { path })
+  const cases = {
+    '/': ['/', ''],
+    '////': ['////', ''],
+    'c:///a/b/c': ['c:///', 'a/b/c'],
+    '\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
+    '//foo//bar//baz': ['//', 'foo//bar//baz'],
+    'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
+    'c:..\\system\\explorer.exe': ['c:', '..\\system\\explorer.exe'],
+    'd:..\\..\\unsafe\\land': ['d:', '..\\..\\unsafe\\land'],
+    'c:foo': ['c:', 'foo'],
+    'D:mark': ['D:', 'mark'],
+    '//?/X:/y/z': ['//?/X:/', 'y/z'],
+    '\\\\?\\X:\\y\\z': ['\\\\?\\X:\\', 'y\\z'],
+  }
+  for (const [input, [root, stripped]] of Object.entries(cases)) {
+    if (!t.strictSame(stripAbsolutePath(input, cwd), [root, stripped], input))
+      break
+  }
+  t.end()
+})

test/unpack.js

@@ -3144,3 +3144,86 @@ t.test('dircache prune all on windows when symlink encountered', t => {
 
   t.end()
 })
+
+t.test('recognize C:.. as a dot path part', t => {
+  if (process.platform !== 'win32') {
+    process.env.TESTING_TAR_FAKE_PLATFORM = 'win32'
+    t.teardown(() => {
+      delete process.env.TESTING_TAR_FAKE_PLATFORM
+    })
+  }
+  const Unpack = t.mock('../lib/unpack.js', {
+    path: {
+      ...path.win32,
+      win32: path.win32,
+      posix: path.posix,
+    },
+  })
+  const UnpackSync = Unpack.Sync
+
+  const data = makeTar([
+    {
+      type: 'File',
+      path: 'C:../x/y/z',
+      size: 1,
+    },
+    'z',
+    {
+      type: 'File',
+      path: 'x:..\\y\\z',
+      size: 1,
+    },
+    'x',
+    {
+      type: 'File',
+      path: 'Y:foo',
+      size: 1,
+    },
+    'y',
+    '',
+    '',
+  ])
+
+  const check = (path, warnings, t) => {
+    t.equal(fs.readFileSync(`${path}/foo`, 'utf8'), 'y')
+    t.strictSame(warnings, [
+      [
+        'TAR_ENTRY_ERROR',
+        "path contains '..'",
+        'C:../x/y/z',
+        'C:../x/y/z',
+      ],
+      ['TAR_ENTRY_ERROR', "path contains '..'", 'x:../y/z', 'x:../y/z'],
+      [
+        'TAR_ENTRY_INFO',
+        'stripping Y: from absolute path',
+        'Y:foo',
+        'foo',
+      ],
+    ])
+    t.end()
+  }
+
+  t.test('async', t => {
+    const warnings = []
+    const path = t.testdir()
+    new Unpack({
+      cwd: path,
+      onwarn: (c, w, { entry, path }) => warnings.push([c, w, path, entry.path]),
+    })
+      .on('close', () => check(path, warnings, t))
+      .end(data)
+  })
+
+  t.test('sync', t => {
+    const warnings = []
+    const path = t.testdir()
+    new UnpackSync({
+      cwd: path,
+      onwarn: (c, w, { entry, path }) => warnings.push([c, w, path, entry.path]),
+    }).end(data)
+    check(path, warnings, t)
+  })
+
+  t.end()
+})