Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 875a37e

Browse filesBrowse files
isaacsisaacs
committed
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.
1 parent b6162c7 commit 875a37e
Copy full SHA for 875a37e

File tree

Expand file treeCollapse file tree

4 files changed

+161
-15
lines changed
Filter options
Expand file treeCollapse file tree

4 files changed

+161
-15
lines changed

‎lib/strip-absolute-path.js

Copy file name to clipboardExpand all lines: lib/strip-absolute-path.js
+12-2Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,23 @@
22
const { isAbsolute, parse } = require('path').win32
33

44
// returns [root, stripped]
5+
// Note that windows will think that //x/y/z/a has a "root" of //x/y, and in
6+
// those cases, we want to sanitize it to x/y/z/a, not z/a, so we strip /
7+
// explicitly if it's the first character.
8+
// drive-specific relative paths on Windows get their root stripped off even
9+
// though they are not absolute, so `c:../foo` becomes ['c:', '../foo']
510
module.exports = path => {
611
let r = ''
7-
while (isAbsolute(path)) {
12+
13+
let parsed = parse(path)
14+
while (isAbsolute(path) || parsed.root) {
815
// windows will think that //x/y/z has a "root" of //x/y/
9-
const root = path.charAt(0) === '/' ? '/' : parse(path).root
16+
// but strip the //?/C:/ off of //?/C:/path
17+
const root = path.charAt(0) === '/' && path.slice(0, 4) !== '//?/' ? '/'
18+
: parsed.root
1019
path = path.substr(root.length)
1120
r += root
21+
parsed = parse(path)
1222
}
1323
return [r, path]
1424
}

‎lib/unpack.js

Copy file name to clipboardExpand all lines: lib/unpack.js
+19-3Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -247,16 +247,16 @@ class Unpack extends Parser {
247247

248248
if (!this.preservePaths) {
249249
const p = normPath(entry.path)
250-
if (p.split('/').includes('..')) {
250+
const parts = p.split('/')
251+
if (parts.includes('..') || isWindows && /^[a-z]:\.\.$/i.test(parts[0])) {
251252
this.warn('TAR_ENTRY_ERROR', `path contains '..'`, {
252253
entry,
253254
path: p,
254255
})
255256
return false
256257
}
257258

258-
// absolutes on posix are also absolutes on win32
259-
// so we only need to test this one to get both
259+
// strip off the root
260260
const [root, stripped] = stripAbsolutePath(p)
261261
if (root) {
262262
entry.path = stripped
@@ -272,6 +272,22 @@ class Unpack extends Parser {
272272
else
273273
entry.absolute = normPath(path.resolve(this.cwd, entry.path))
274274

275+
// if we somehow ended up with a path that escapes the cwd, and we are
276+
// not in preservePaths mode, then something is fishy! This should have
277+
// been prevented above, so ignore this for coverage.
278+
/* istanbul ignore if - defense in depth */
279+
if (!this.preservePaths &&
280+
entry.absolute.indexOf(this.cwd + '/') !== 0 &&
281+
entry.absolute !== this.cwd) {
282+
this.warn('TAR_ENTRY_ERROR', 'path escaped extraction target', {
283+
entry,
284+
path: normPath(entry.path),
285+
resolvedPath: entry.absolute,
286+
cwd: this.cwd,
287+
})
288+
return false
289+
}
290+
275291
// an archive can set properties on the extraction directory, but it
276292
// may not replace the cwd with a different kind of thing entirely.
277293
if (entry.absolute === this.cwd &&

‎test/strip-absolute-path.js

Copy file name to clipboard
+47-10Lines changed: 47 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,51 @@
11
const t = require('tap')
22
const stripAbsolutePath = require('../lib/strip-absolute-path.js')
3+
const cwd = process.cwd()
34

4-
const cases = {
5-
'/': ['/', ''],
6-
'////': ['////', ''],
7-
'c:///a/b/c': ['c:///', 'a/b/c'],
8-
'\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
9-
'//foo//bar//baz': ['//', 'foo//bar//baz'],
10-
'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
11-
}
5+
t.test('basic', t => {
6+
const cases = {
7+
'/': ['/', ''],
8+
'////': ['////', ''],
9+
'c:///a/b/c': ['c:///', 'a/b/c'],
10+
'\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
11+
'//foo//bar//baz': ['//', 'foo//bar//baz'],
12+
'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
13+
}
1214

13-
for (const [input, [root, stripped]] of Object.entries(cases))
14-
t.strictSame(stripAbsolutePath(input), [root, stripped], input)
15+
for (const [input, [root, stripped]] of Object.entries(cases))
16+
t.strictSame(stripAbsolutePath(input, cwd), [root, stripped], input)
17+
t.end()
18+
})
19+
20+
t.test('drive-local paths', t => {
21+
const env = process.env
22+
t.teardown(() => process.env = env)
23+
const cwd = 'D:\\safety\\land'
24+
const realPath = require('path')
25+
// be windowsy
26+
const path = {
27+
...realPath.win32,
28+
win32: realPath.win32,
29+
posix: realPath.posix,
30+
}
31+
const stripAbsolutePath = t.mock('../lib/strip-absolute-path.js', { path })
32+
const cases = {
33+
'/': ['/', ''],
34+
'////': ['////', ''],
35+
'c:///a/b/c': ['c:///', 'a/b/c'],
36+
'\\\\foo\\bar\\baz': ['\\\\foo\\bar\\', 'baz'],
37+
'//foo//bar//baz': ['//', 'foo//bar//baz'],
38+
'c:\\c:\\c:\\c:\\\\d:\\e/f/g': ['c:\\c:\\c:\\c:\\\\d:\\', 'e/f/g'],
39+
'c:..\\system\\explorer.exe': ['c:', '..\\system\\explorer.exe'],
40+
'd:..\\..\\unsafe\\land': ['d:', '..\\..\\unsafe\\land'],
41+
'c:foo': ['c:', 'foo'],
42+
'D:mark': ['D:', 'mark'],
43+
'//?/X:/y/z': ['//?/X:/', 'y/z'],
44+
'\\\\?\\X:\\y\\z': ['\\\\?\\X:\\', 'y\\z'],
45+
}
46+
for (const [input, [root, stripped]] of Object.entries(cases)) {
47+
if (!t.strictSame(stripAbsolutePath(input, cwd), [root, stripped], input))
48+
break
49+
}
50+
t.end()
51+
})

‎test/unpack.js

Copy file name to clipboardExpand all lines: test/unpack.js
+83Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3144,3 +3144,86 @@ t.test('dircache prune all on windows when symlink encountered', t => {
31443144

31453145
t.end()
31463146
})
3147+
3148+
t.test('recognize C:.. as a dot path part', t => {
3149+
if (process.platform !== 'win32') {
3150+
process.env.TESTING_TAR_FAKE_PLATFORM = 'win32'
3151+
t.teardown(() => {
3152+
delete process.env.TESTING_TAR_FAKE_PLATFORM
3153+
})
3154+
}
3155+
const Unpack = t.mock('../lib/unpack.js', {
3156+
path: {
3157+
...path.win32,
3158+
win32: path.win32,
3159+
posix: path.posix,
3160+
},
3161+
})
3162+
const UnpackSync = Unpack.Sync
3163+
3164+
const data = makeTar([
3165+
{
3166+
type: 'File',
3167+
path: 'C:../x/y/z',
3168+
size: 1,
3169+
},
3170+
'z',
3171+
{
3172+
type: 'File',
3173+
path: 'x:..\\y\\z',
3174+
size: 1,
3175+
},
3176+
'x',
3177+
{
3178+
type: 'File',
3179+
path: 'Y:foo',
3180+
size: 1,
3181+
},
3182+
'y',
3183+
'',
3184+
'',
3185+
])
3186+
3187+
const check = (path, warnings, t) => {
3188+
t.equal(fs.readFileSync(`${path}/foo`, 'utf8'), 'y')
3189+
t.strictSame(warnings, [
3190+
[
3191+
'TAR_ENTRY_ERROR',
3192+
"path contains '..'",
3193+
'C:../x/y/z',
3194+
'C:../x/y/z',
3195+
],
3196+
['TAR_ENTRY_ERROR', "path contains '..'", 'x:../y/z', 'x:../y/z'],
3197+
[
3198+
'TAR_ENTRY_INFO',
3199+
'stripping Y: from absolute path',
3200+
'Y:foo',
3201+
'foo',
3202+
],
3203+
])
3204+
t.end()
3205+
}
3206+
3207+
t.test('async', t => {
3208+
const warnings = []
3209+
const path = t.testdir()
3210+
new Unpack({
3211+
cwd: path,
3212+
onwarn: (c, w, { entry, path }) => warnings.push([c, w, path, entry.path]),
3213+
})
3214+
.on('close', () => check(path, warnings, t))
3215+
.end(data)
3216+
})
3217+
3218+
t.test('sync', t => {
3219+
const warnings = []
3220+
const path = t.testdir()
3221+
new UnpackSync({
3222+
cwd: path,
3223+
onwarn: (c, w, { entry, path }) => warnings.push([c, w, path, entry.path]),
3224+
}).end(data)
3225+
check(path, warnings, t)
3226+
})
3227+
3228+
t.end()
3229+
})

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.