GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: resolve symlink chains fully when extracting (#142) · node-modules/compressing@90b2e54

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • lib
    • utils.js
  • test
    • tar
      • symlink-resolution.test.js
    • util.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/utils.js‎

Copy file name to clipboardExpand all lines: lib/utils.js
+67-9Lines 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') {
Collapse file

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

Copy file name to clipboard
+283Lines 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 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.