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 bb93ba2 commit 52b09e3
Show file tree
Hide file tree
Showing 3 changed files with 79 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 @@ -236,13 +236,13 @@ 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(`path contains '..'`, 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 s = stripAbsolutePath(p)
if (s[0]) {
entry.path = s[1]
Expand All @@ -255,6 +255,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
58 changes: 48 additions & 10 deletions test/strip-absolute-path.js
Original file line number Diff line number Diff line change
@@ -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()
})

0 comments on commit 52b09e3

Please sign in to comment.