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()
+})