Skip to content

Commit

Permalink
fix: prevent path escape using drive-relative paths
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
isaacs committed Aug 19, 2021
1 parent b6162c7 commit 875a37e
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 15 deletions.
14 changes: 12 additions & 2 deletions lib/strip-absolute-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
22 changes: 19 additions & 3 deletions lib/unpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 &&
Expand Down
57 changes: 47 additions & 10 deletions test/strip-absolute-path.js
Original file line number Diff line number Diff line change
@@ -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()
})
83 changes: 83 additions & 0 deletions test/unpack.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})

0 comments on commit 875a37e

Please sign in to comment.