From 875a37e3ec031186fc6599f6807341f56c584598 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 12 Aug 2021 16:42:23 -0700 Subject: [PATCH] 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. --- lib/strip-absolute-path.js | 14 ++++++- lib/unpack.js | 22 ++++++++-- test/strip-absolute-path.js | 57 ++++++++++++++++++++----- test/unpack.js | 83 +++++++++++++++++++++++++++++++++++++ 4 files changed, 161 insertions(+), 15 deletions(-) diff --git a/lib/strip-absolute-path.js b/lib/strip-absolute-path.js index 49161ddc..1aa2d2ae 100644 --- a/lib/strip-absolute-path.js +++ b/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] } diff --git a/lib/unpack.js b/lib/unpack.js index 0783949c..b889f4f8 100644 --- a/lib/unpack.js +++ b/lib/unpack.js @@ -247,7 +247,8 @@ 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, @@ -255,8 +256,7 @@ class Unpack extends Parser { 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 && diff --git a/test/strip-absolute-path.js b/test/strip-absolute-path.js index beb057ff..98c6ec18 100644 --- a/test/strip-absolute-path.js +++ b/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() +}) diff --git a/test/unpack.js b/test/unpack.js index d680638c..93a61bc9 100644 --- a/test/unpack.js +++ b/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() +})