FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: resolve symlink chains fully when extracting (#142) · node-modules/compressing@90b2e54 · GitHub

  1. node-modules
  2. compressing

Repository navigation

Commit 90b2e54

Browse files
fengmk2
authored
fix: resolve symlink chains fully when extracting (#142)
Backport of #140 to 1.x. `isRealPathSafe()` stopped walking as soon as `realpath()` failed on a dangling link, checking only that link's immediate target. A destination reached through several hops, or through a linked directory, was only partially resolved, so an entry could land somewhere the check had not accounted for. It now resolves the remaining hops itself, bounded by `MAX_SYMLINK_DEPTH`, and walks from whichever extraction root actually contains the target so a link named in the real namespace is not rejected. Behaviour change worth noting: a file entry landing on a symlink now replaces that link instead of writing through to whatever it points at. This matches tar(1), node-tar and libarchive. Where the platform has it, the write also opens with `O_NOFOLLOW`. Linked directories inside the extraction directory are still traversed. Written in the callback style the surrounding 1.x code uses, so it stays compatible with the branch's Node range. Suite is 166 passing on this branch.
1 parent 60fa3af commit 90b2e54

3 files changed

Lines changed: 378 additions & 9 deletions

File tree

‎lib/utils.js‎

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,35 @@ const path = require('path');
55
const mkdirp = require('mkdirp');
66
const pump = require('pump');
77

8+
// Matches the kernel's own symlink chain limit closely enough to reject loops
9+
// that realpath() never sees, without rejecting any realistic layout.
10+
const MAX_SYMLINK_DEPTH = 32;
11+
12+
// Numeric flags are accepted here per the "File system flags" section of the fs
13+
// docs. The flag makes open() fail with ELOOP when the final component is a
14+
// symlink, so the write never resolves one. It is undefined on Windows, where
15+
// unlinkSymlink() below does the work.
16+
const NO_FOLLOW_WRITE_FLAGS = typeof fs.constants.O_NOFOLLOW === 'number'
17+
? fs.constants.O_NOFOLLOW | fs.constants.O_CREAT | fs.constants.O_TRUNC | fs.constants.O_WRONLY
18+
: 'w';
19+
20+
/**
21+
* Remove a symlink sitting at the exact path an entry is about to be written to.
22+
* Extraction replaces such a link rather than writing through to whatever it
23+
* points at, which is how tar(1), node-tar and libarchive all behave.
24+
* @param {string} target - Absolute path of the entry destination
25+
* @param {function} callback - callback(err)
26+
*/
27+
function unlinkSymlink(target, callback) {
28+
fs.lstat(target, function(err, stat) {
29+
if (err) return callback(err.code === 'ENOENT' ? null : err);
30+
if (!stat.isSymbolicLink()) return callback(null);
31+
fs.unlink(target, function(err) {
32+
callback(err && err.code !== 'ENOENT' ? err : null);
33+
});
34+
});
35+
}
36+
837
/**
938
* Check if childPath is within parentPath (prevents path traversal attacks)
1039
* @param {string} childPath - The path to check
@@ -30,16 +59,34 @@ function isPathWithinParent(childPath, parentPath) {
3059
* @param {string} parentDir - Absolute path of the extraction root
3160
* @param {string} realParentDir - Pre-resolved real path of parentDir (handles OS-level symlinks like /var -> /private/var on macOS)
3261
* @param {function} callback - callback(err, safe)
62+
* @param {number} [depth] - Recursion depth when re-walking a dangling symlink's target
3363
*/
34-
function isRealPathSafe(targetPath, parentDir, realParentDir, callback) {
64+
function isRealPathSafe(targetPath, parentDir, realParentDir, callback, depth) {
65+
depth = depth || 0;
66+
// realpath() rejects long chains with ELOOP, but the dangling branch below resolves
67+
// hop by hop without the kernel's help, so it needs its own bound.
68+
if (depth >= MAX_SYMLINK_DEPTH) return callback(null, false);
69+
3570
function isWithinParent(p) {
3671
return isPathWithinParent(p, parentDir) || isPathWithinParent(p, realParentDir);
3772
}
3873

39-
const relative = path.relative(parentDir, targetPath);
74+
// A link target may be written in either namespace when the two differ, as with
75+
// /var -> /private/var on macOS. Walk from whichever root actually contains it,
76+
// or the relative path below would climb out through '..' and reject a safe link.
77+
let baseDir;
78+
if (isPathWithinParent(targetPath, parentDir)) {
79+
baseDir = parentDir;
80+
} else if (isPathWithinParent(targetPath, realParentDir)) {
81+
baseDir = realParentDir;
82+
} else {
83+
return callback(null, false);
84+
}
85+
86+
const relative = path.relative(baseDir, targetPath);
4087
const segments = relative.split(path.sep);
4188
let i = 0;
42-
let current = parentDir;
89+
let current = baseDir;
4390

4491
function checkNext() {
4592
if (i >= segments.length) return callback(null, true);
@@ -58,11 +105,15 @@ function isRealPathSafe(targetPath, parentDir, realParentDir, callback) {
58105
fs.realpath(current, function(err, resolved) {
59106
if (err) {
60107
if (err.code === 'ENOENT') {
61-
// Dangling symlink - check textual target
108+
// Dangling symlink: realpath() gave up, so resolve the textual target
109+
// ourselves. Checking the target string alone is not enough, because the
110+
// target may itself be a symlink, or sit under a directory that is one,
111+
// and both get resolved when the entry is actually written.
62112
return fs.readlink(current, function(err, linkTarget) {
63113
if (err) return callback(null, false);
64114
const absTarget = path.resolve(path.dirname(current), linkTarget);
65-
callback(null, isWithinParent(absTarget));
115+
if (!isWithinParent(absTarget)) return callback(null, false);
116+
isRealPathSafe(absTarget, parentDir, realParentDir, callback, depth + 1);
66117
});
67118
}
68119
// Fail closed: unexpected errors during symlink resolution are unsafe
@@ -214,11 +265,18 @@ exports.makeUncompressFn = StreamClass => {
214265
mkdirp(dir, err => {
215266
if (err) return reject(err);
216267

217-
entryCount++;
218-
pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => {
268+
unlinkSymlink(destFilePath, err => {
219269
if (err) return reject(err);
220-
successCount++;
221-
done();
270+
271+
entryCount++;
272+
pump(stream, fs.createWriteStream(destFilePath, {
273+
flags: NO_FOLLOW_WRITE_FLAGS,
274+
mode: opts.mode || header.mode,
275+
}), err => {
276+
if (err) return reject(err);
277+
successCount++;
278+
done();
279+
});
222280
});
223281
});
224282
} else if (header.type === 'symlink') {

‎test/tar/symlink-resolution.test.js‎

Lines changed: 283 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,283 @@
1+
'use strict';
2+
3+
const fs = require('fs');
4+
const os = require('os');
5+
const path = require('path');
6+
const zlib = require('zlib');
7+
const uuid = require('uuid');
8+
const assert = require('assert');
9+
const compressing = require('../..');
10+
const { createTarBuffer, createZipBuffer } = require('../util');
11+
12+
// Extraction resolves a symlink chain hop by hop when realpath() cannot, so an
13+
// entry whose destination passes through several links still lands where the
14+
// resolved chain actually points, and never outside the extraction directory.
15+
describe('test/tar/symlink-resolution.test.js', () => {
16+
let tempDir;
17+
18+
beforeEach(() => {
19+
tempDir = path.join(os.tmpdir(), uuid.v4());
20+
fs.mkdirSync(tempDir, { recursive: true });
21+
});
22+
23+
afterEach(() => {
24+
fs.rmSync(tempDir, { recursive: true, force: true });
25+
});
26+
27+
function gzipBuffer(buf) {
28+
return new Promise((resolve, reject) => {
29+
zlib.gzip(buf, (err, result) => {
30+
if (err) return reject(err);
31+
resolve(result);
32+
});
33+
});
34+
}
35+
36+
// destDir/entry -> destDir/hop -> outsideDir/other.txt, which does not exist,
37+
// so realpath() cannot resolve the chain and each hop is walked by hand.
38+
function setupChain(destDir, outsideDir) {
39+
fs.mkdirSync(outsideDir, { recursive: true });
40+
fs.mkdirSync(destDir, { recursive: true });
41+
fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
42+
fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop'));
43+
}
44+
45+
// destDir/entry -> linkedDir/other.txt, where destDir/linkedDir -> outsideDir
46+
function setupLinkedDir(destDir, outsideDir) {
47+
fs.mkdirSync(outsideDir, { recursive: true });
48+
fs.mkdirSync(destDir, { recursive: true });
49+
fs.symlinkSync(outsideDir, path.join(destDir, 'linkedDir'));
50+
fs.symlinkSync(path.join('linkedDir', 'other.txt'), path.join(destDir, 'entry'));
51+
}
52+
53+
describe('a chain whose first hop stays inside destDir', () => {
54+
it('should not write past the end of the chain', async () => {
55+
const destDir = path.join(tempDir, 'dest');
56+
const outsideDir = path.join(tempDir, 'outside');
57+
setupChain(destDir, outsideDir);
58+
59+
const tarBuffer = await createTarBuffer([
60+
{ name: 'entry', type: 'file', content: 'content' },
61+
]);
62+
63+
await compressing.tar.uncompress(tarBuffer, destDir);
64+
65+
assert.strictEqual(
66+
fs.existsSync(path.join(outsideDir, 'other.txt')),
67+
false,
68+
'The entry should not be written at the end of the chain'
69+
);
70+
});
71+
72+
it('should handle a chain longer than two hops', async () => {
73+
const destDir = path.join(tempDir, 'dest');
74+
const outsideDir = path.join(tempDir, 'outside');
75+
fs.mkdirSync(outsideDir, { recursive: true });
76+
fs.mkdirSync(destDir, { recursive: true });
77+
fs.symlinkSync(path.join(destDir, 'hop1'), path.join(destDir, 'entry'));
78+
fs.symlinkSync(path.join(destDir, 'hop2'), path.join(destDir, 'hop1'));
79+
fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop2'));
80+
81+
const tarBuffer = await createTarBuffer([
82+
{ name: 'entry', type: 'file', content: 'content' },
83+
]);
84+
85+
await compressing.tar.uncompress(tarBuffer, destDir);
86+
87+
assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
88+
});
89+
90+
it('should behave the same in tgz extraction', async () => {
91+
const destDir = path.join(tempDir, 'dest');
92+
const outsideDir = path.join(tempDir, 'outside');
93+
setupChain(destDir, outsideDir);
94+
95+
const tarBuffer = await createTarBuffer([
96+
{ name: 'entry', type: 'file', content: 'content' },
97+
]);
98+
await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
99+
100+
assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
101+
});
102+
103+
it('should behave the same in zip extraction', async () => {
104+
const destDir = path.join(tempDir, 'dest');
105+
const outsideDir = path.join(tempDir, 'outside');
106+
setupChain(destDir, outsideDir);
107+
108+
const zipBuffer = await createZipBuffer([
109+
{ name: 'entry', content: 'content' },
110+
]);
111+
await compressing.zip.uncompress(zipBuffer, destDir);
112+
113+
assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
114+
});
115+
});
116+
117+
describe('a chain passing through a linked directory', () => {
118+
it('should resolve the directory component of the link target', async () => {
119+
const destDir = path.join(tempDir, 'dest');
120+
const outsideDir = path.join(tempDir, 'outside');
121+
setupLinkedDir(destDir, outsideDir);
122+
123+
const tarBuffer = await createTarBuffer([
124+
{ name: 'entry', type: 'file', content: 'content' },
125+
]);
126+
127+
await compressing.tar.uncompress(tarBuffer, destDir);
128+
129+
assert.strictEqual(
130+
fs.existsSync(path.join(outsideDir, 'other.txt')),
131+
false,
132+
'The linked directory in the target should be resolved, not taken literally'
133+
);
134+
});
135+
136+
it('should behave the same in tgz extraction', async () => {
137+
const destDir = path.join(tempDir, 'dest');
138+
const outsideDir = path.join(tempDir, 'outside');
139+
setupLinkedDir(destDir, outsideDir);
140+
141+
const tarBuffer = await createTarBuffer([
142+
{ name: 'entry', type: 'file', content: 'content' },
143+
]);
144+
await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
145+
146+
assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
147+
});
148+
149+
it('should behave the same in zip extraction', async () => {
150+
const destDir = path.join(tempDir, 'dest');
151+
const outsideDir = path.join(tempDir, 'outside');
152+
setupLinkedDir(destDir, outsideDir);
153+
154+
const zipBuffer = await createZipBuffer([
155+
{ name: 'entry', content: 'content' },
156+
]);
157+
await compressing.zip.uncompress(zipBuffer, destDir);
158+
159+
assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
160+
});
161+
});
162+
163+
describe('a symlink at the entry destination', () => {
164+
it('should be replaced by the entry instead of written through', async () => {
165+
const destDir = path.join(tempDir, 'dest');
166+
fs.mkdirSync(destDir, { recursive: true });
167+
fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
168+
fs.symlinkSync(path.join(destDir, 'final.txt'), path.join(destDir, 'hop'));
169+
170+
const tarBuffer = await createTarBuffer([
171+
{ name: 'entry', type: 'file', content: 'content' },
172+
]);
173+
174+
await compressing.tar.uncompress(tarBuffer, destDir);
175+
176+
assert.strictEqual(
177+
fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(),
178+
false,
179+
'The symlink at the destination should have been replaced by a regular file'
180+
);
181+
assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'content');
182+
assert.strictEqual(
183+
fs.existsSync(path.join(destDir, 'final.txt')),
184+
false,
185+
'The chain should not have been followed to its target'
186+
);
187+
});
188+
189+
it('should leave the file the symlink points at untouched', async () => {
190+
const destDir = path.join(tempDir, 'dest');
191+
fs.mkdirSync(destDir, { recursive: true });
192+
const target = path.join(destDir, 'target.txt');
193+
fs.writeFileSync(target, 'ORIGINAL_CONTENT');
194+
fs.symlinkSync(target, path.join(destDir, 'entry'));
195+
196+
const tarBuffer = await createTarBuffer([
197+
{ name: 'entry', type: 'file', content: 'new content' },
198+
]);
199+
200+
await compressing.tar.uncompress(tarBuffer, destDir);
201+
202+
assert.strictEqual(
203+
fs.readFileSync(target, 'utf8'),
204+
'ORIGINAL_CONTENT',
205+
'Writing an entry must not reach through a symlink to its target'
206+
);
207+
assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'new content');
208+
});
209+
});
210+
211+
describe('linked directories inside destDir', () => {
212+
it('should still be traversed when writing an entry beneath them', async () => {
213+
const destDir = path.join(tempDir, 'dest');
214+
const realDir = path.join(destDir, 'real');
215+
fs.mkdirSync(realDir, { recursive: true });
216+
fs.symlinkSync(realDir, path.join(destDir, 'linkDir'));
217+
218+
const tarBuffer = await createTarBuffer([
219+
{ name: 'linkDir/final.txt', type: 'file', content: 'content' },
220+
]);
221+
222+
await compressing.tar.uncompress(tarBuffer, destDir);
223+
224+
assert.strictEqual(
225+
fs.readFileSync(path.join(realDir, 'final.txt'), 'utf8'),
226+
'content',
227+
'A linked directory inside destDir should still be traversed'
228+
);
229+
});
230+
});
231+
232+
describe('an extraction directory reached through a symlink', () => {
233+
// destDir is given as linkBase/dest while its real path is realBase/dest, the
234+
// shape /var -> /private/var produces on macOS. A link target written in the
235+
// real namespace must still be recognised as living inside destDir.
236+
//
237+
// Skipped on Windows, where a dangling link resolves differently and the entry
238+
// is skipped regardless. That behaviour predates this change, and the namespace
239+
// divergence covered here is a POSIX shape.
240+
const itPosix = process.platform === 'win32' ? it.skip : it;
241+
242+
itPosix('should accept a dangling target written in the real namespace', async () => {
243+
const realBase = path.join(tempDir, 'realBase');
244+
const linkBase = path.join(tempDir, 'linkBase');
245+
fs.mkdirSync(path.join(realBase, 'dest'), { recursive: true });
246+
fs.symlinkSync(realBase, linkBase);
247+
248+
const destDir = path.join(linkBase, 'dest');
249+
// realpathSync, not the realBase path: tempDir may itself sit behind a symlink.
250+
const realDest = fs.realpathSync(path.join(realBase, 'dest'));
251+
fs.symlinkSync(path.join(realDest, 'final.txt'), path.join(destDir, 'entry'));
252+
253+
const tarBuffer = await createTarBuffer([
254+
{ name: 'entry', type: 'file', content: 'content' },
255+
]);
256+
257+
await compressing.tar.uncompress(tarBuffer, destDir);
258+
259+
assert.strictEqual(
260+
fs.readFileSync(path.join(destDir, 'entry'), 'utf8'),
261+
'content',
262+
'A target inside destDir should be accepted whichever namespace names it'
263+
);
264+
});
265+
});
266+
267+
describe('symlink cycles', () => {
268+
it('should terminate rather than loop', async () => {
269+
const destDir = path.join(tempDir, 'dest');
270+
fs.mkdirSync(destDir, { recursive: true });
271+
fs.symlinkSync(path.join(destDir, 'b'), path.join(destDir, 'entry'));
272+
fs.symlinkSync(path.join(destDir, 'entry'), path.join(destDir, 'b'));
273+
274+
const tarBuffer = await createTarBuffer([
275+
{ name: 'entry', type: 'file', content: 'content' },
276+
]);
277+
278+
await compressing.tar.uncompress(tarBuffer, destDir);
279+
280+
assert.strictEqual(fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(), true);
281+
});
282+
});
283+
});

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL