From 82eac952f7c10765969ed464e549375854b26edc 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 | 58 ++++++++++++++++++++++++++++++------- 3 files changed, 79 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 8660c9a7..d56f9039 100644 --- a/lib/unpack.js +++ b/lib/unpack.js @@ -246,7 +246,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, @@ -254,8 +255,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 @@ -271,6 +271,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..c50e2858 100644 --- a/test/strip-absolute-path.js +++ b/test/strip-absolute-path.js @@ -1,14 +1,52 @@ const t = require('tap') const stripAbsolutePath = require('../lib/strip-absolute-path.js') +const cwd = process.cwd() +const requireInject = require('require-inject') -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 = requireInject('../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() +})