fix: resolve symlink chains fully when extracting

lib/utils.js

@@ -5,11 +5,40 @@
 const mkdirp = require('mkdirp');
 const pump = require('pump');
 
+// Matches the kernel's own symlink chain limit closely enough to reject loops
+// that realpath() never sees, without rejecting any realistic layout.
+const MAX_SYMLINK_DEPTH = 32;
+
+// Numeric flags are accepted here per the "File system flags" section of the fs
+// docs. The flag makes open() fail with ELOOP when the final component is a
+// symlink, so the write never resolves one. It is undefined on Windows, where
+// unlinkSymlink() below does the work.
+const NO_FOLLOW_WRITE_FLAGS = typeof fs.constants.O_NOFOLLOW === 'number'
+  ? fs.constants.O_NOFOLLOW | fs.constants.O_CREAT | fs.constants.O_TRUNC | fs.constants.O_WRONLY
+  : 'w';
+
+/**
+ * Remove a symlink sitting at the exact path an entry is about to be written to.
+ * Extraction replaces such a link rather than writing through to whatever it
+ * points at, which is how tar(1), node-tar and libarchive all behave.
+ * @param {string} target - Absolute path of the entry destination
+ * @param {function} callback - callback(err)
+ */
+function unlinkSymlink(target, callback) {
+  fs.lstat(target, function(err, stat) {
+    if (err) return callback(err.code === 'ENOENT' ? null : err);
+    if (!stat.isSymbolicLink()) return callback(null);
+    fs.unlink(target, function(err) {
+      callback(err && err.code !== 'ENOENT' ? err : null);
+    });
+  });
+}
+
 /**
  * Check if childPath is within parentPath (prevents path traversal attacks)
  * @param {string} childPath - The path to check
  * @param {string} parentPath - The parent directory path
  * @returns {boolean} - True if childPath is within parentPath
  */
 function isPathWithinParent(childPath, parentPath) {
   const normalizedChild = path.resolve(childPath);
@@ -30,16 +59,34 @@
  * @param {string} parentDir - Absolute path of the extraction root
  * @param {string} realParentDir - Pre-resolved real path of parentDir (handles OS-level symlinks like /var -> /private/var on macOS)
  * @param {function} callback - callback(err, safe)
+ * @param {number} [depth] - Recursion depth when re-walking a dangling symlink's target
  */
-function isRealPathSafe(targetPath, parentDir, realParentDir, callback) {
+function isRealPathSafe(targetPath, parentDir, realParentDir, callback, depth) {
+  depth = depth || 0;
+  // realpath() rejects long chains with ELOOP, but the dangling branch below resolves
+  // hop by hop without the kernel's help, so it needs its own bound.
+  if (depth >= MAX_SYMLINK_DEPTH) return callback(null, false);
+
   function isWithinParent(p) {
     return isPathWithinParent(p, parentDir) || isPathWithinParent(p, realParentDir);
   }
 
-  const relative = path.relative(parentDir, targetPath);
+  // A link target may be written in either namespace when the two differ, as with
+  // /var -> /private/var on macOS. Walk from whichever root actually contains it,
+  // or the relative path below would climb out through '..' and reject a safe link.
+  let baseDir;
+  if (isPathWithinParent(targetPath, parentDir)) {
+    baseDir = parentDir;
+  } else if (isPathWithinParent(targetPath, realParentDir)) {
+    baseDir = realParentDir;
+  } else {
+    return callback(null, false);
+  }
+
+  const relative = path.relative(baseDir, targetPath);
   const segments = relative.split(path.sep);
   let i = 0;
-  let current = parentDir;
+  let current = baseDir;
 
   function checkNext() {
     if (i >= segments.length) return callback(null, true);
@@ -47,7 +94,7 @@
     if (!segment || segment === '.') return checkNext();
 
     current = path.join(current, segment);
     fs.lstat(current, function(err, stat) {
       if (err) {
         if (err.code === 'ENOENT') return callback(null, true); // doesn't exist yet, safe
         // Fail closed: unexpected filesystem errors are unsafe
@@ -55,14 +102,18 @@
       }
       if (!stat.isSymbolicLink()) return checkNext();
 
       fs.realpath(current, function(err, resolved) {
         if (err) {
           if (err.code === 'ENOENT') {
-            // Dangling symlink - check textual target
+            // Dangling symlink: realpath() gave up, so resolve the textual target
+            // ourselves. Checking the target string alone is not enough, because the
+            // target may itself be a symlink, or sit under a directory that is one,
+            // and both get resolved when the entry is actually written.
             return fs.readlink(current, function(err, linkTarget) {
               if (err) return callback(null, false);
               const absTarget = path.resolve(path.dirname(current), linkTarget);
-              callback(null, isWithinParent(absTarget));
+              if (!isWithinParent(absTarget)) return callback(null, false);
+              isRealPathSafe(absTarget, parentDir, realParentDir, callback, depth + 1);
             });
           }
           // Fail closed: unexpected errors during symlink resolution are unsafe
@@ -214,11 +265,18 @@
                   mkdirp(dir, err => {
                     if (err) return reject(err);
 
-                    entryCount++;
-                    pump(stream, fs.createWriteStream(destFilePath, { mode: opts.mode || header.mode }), err => {
+                    unlinkSymlink(destFilePath, err => {
                       if (err) return reject(err);
-                      successCount++;
-                      done();
+
+                      entryCount++;
+                      pump(stream, fs.createWriteStream(destFilePath, {
+                        flags: NO_FOLLOW_WRITE_FLAGS,
+                        mode: opts.mode || header.mode,
+                      }), err => {
+                        if (err) return reject(err);
+                        successCount++;
+                        done();
+                      });
                     });
                   });
                 } else if (header.type === 'symlink') {

test/tar/symlink-resolution.test.js

@@ -0,0 +1,283 @@
+'use strict';
+
+const fs = require('fs');
+const os = require('os');
+const path = require('path');
+const zlib = require('zlib');
+const uuid = require('uuid');
+const assert = require('assert');
+const compressing = require('../..');
+const { createTarBuffer, createZipBuffer } = require('../util');
+
+// Extraction resolves a symlink chain hop by hop when realpath() cannot, so an
+// entry whose destination passes through several links still lands where the
+// resolved chain actually points, and never outside the extraction directory.
+describe('test/tar/symlink-resolution.test.js', () => {
+  let tempDir;
+
+  beforeEach(() => {
+    tempDir = path.join(os.tmpdir(), uuid.v4());
+    fs.mkdirSync(tempDir, { recursive: true });
+  });
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true });
+  });
+
+  function gzipBuffer(buf) {
+    return new Promise((resolve, reject) => {
+      zlib.gzip(buf, (err, result) => {
+        if (err) return reject(err);
+        resolve(result);
+      });
+    });
+  }
+
+  // destDir/entry -> destDir/hop -> outsideDir/other.txt, which does not exist,
+  // so realpath() cannot resolve the chain and each hop is walked by hand.
+  function setupChain(destDir, outsideDir) {
+    fs.mkdirSync(outsideDir, { recursive: true });
+    fs.mkdirSync(destDir, { recursive: true });
+    fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
+    fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop'));
+  }
+
+  // destDir/entry -> linkedDir/other.txt, where destDir/linkedDir -> outsideDir
+  function setupLinkedDir(destDir, outsideDir) {
+    fs.mkdirSync(outsideDir, { recursive: true });
+    fs.mkdirSync(destDir, { recursive: true });
+    fs.symlinkSync(outsideDir, path.join(destDir, 'linkedDir'));
+    fs.symlinkSync(path.join('linkedDir', 'other.txt'), path.join(destDir, 'entry'));
+  }
+
+  describe('a chain whose first hop stays inside destDir', () => {
+    it('should not write past the end of the chain', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.existsSync(path.join(outsideDir, 'other.txt')),
+        false,
+        'The entry should not be written at the end of the chain'
+      );
+    });
+
+    it('should handle a chain longer than two hops', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      fs.mkdirSync(outsideDir, { recursive: true });
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'hop1'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'hop2'), path.join(destDir, 'hop1'));
+      fs.symlinkSync(path.join(outsideDir, 'other.txt'), path.join(destDir, 'hop2'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in tgz extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+      await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in zip extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupChain(destDir, outsideDir);
+
+      const zipBuffer = await createZipBuffer([
+        { name: 'entry', content: 'content' },
+      ]);
+      await compressing.zip.uncompress(zipBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+  });
+
+  describe('a chain passing through a linked directory', () => {
+    it('should resolve the directory component of the link target', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.existsSync(path.join(outsideDir, 'other.txt')),
+        false,
+        'The linked directory in the target should be resolved, not taken literally'
+      );
+    });
+
+    it('should behave the same in tgz extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+      await compressing.tgz.uncompress(await gzipBuffer(tarBuffer), destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+
+    it('should behave the same in zip extraction', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const outsideDir = path.join(tempDir, 'outside');
+      setupLinkedDir(destDir, outsideDir);
+
+      const zipBuffer = await createZipBuffer([
+        { name: 'entry', content: 'content' },
+      ]);
+      await compressing.zip.uncompress(zipBuffer, destDir);
+
+      assert.strictEqual(fs.existsSync(path.join(outsideDir, 'other.txt')), false);
+    });
+  });
+
+  describe('a symlink at the entry destination', () => {
+    it('should be replaced by the entry instead of written through', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'hop'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'final.txt'), path.join(destDir, 'hop'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(),
+        false,
+        'The symlink at the destination should have been replaced by a regular file'
+      );
+      assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'content');
+      assert.strictEqual(
+        fs.existsSync(path.join(destDir, 'final.txt')),
+        false,
+        'The chain should not have been followed to its target'
+      );
+    });
+
+    it('should leave the file the symlink points at untouched', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      const target = path.join(destDir, 'target.txt');
+      fs.writeFileSync(target, 'ORIGINAL_CONTENT');
+      fs.symlinkSync(target, path.join(destDir, 'entry'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'new content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(target, 'utf8'),
+        'ORIGINAL_CONTENT',
+        'Writing an entry must not reach through a symlink to its target'
+      );
+      assert.strictEqual(fs.readFileSync(path.join(destDir, 'entry'), 'utf8'), 'new content');
+    });
+  });
+
+  describe('linked directories inside destDir', () => {
+    it('should still be traversed when writing an entry beneath them', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      const realDir = path.join(destDir, 'real');
+      fs.mkdirSync(realDir, { recursive: true });
+      fs.symlinkSync(realDir, path.join(destDir, 'linkDir'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'linkDir/final.txt', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(path.join(realDir, 'final.txt'), 'utf8'),
+        'content',
+        'A linked directory inside destDir should still be traversed'
+      );
+    });
+  });
+
+  describe('an extraction directory reached through a symlink', () => {
+    // destDir is given as linkBase/dest while its real path is realBase/dest, the
+    // shape /var -> /private/var produces on macOS. A link target written in the
+    // real namespace must still be recognised as living inside destDir.
+    //
+    // Skipped on Windows, where a dangling link resolves differently and the entry
+    // is skipped regardless. That behaviour predates this change, and the namespace
+    // divergence covered here is a POSIX shape.
+    const itPosix = process.platform === 'win32' ? it.skip : it;
+
+    itPosix('should accept a dangling target written in the real namespace', async () => {
+      const realBase = path.join(tempDir, 'realBase');
+      const linkBase = path.join(tempDir, 'linkBase');
+      fs.mkdirSync(path.join(realBase, 'dest'), { recursive: true });
+      fs.symlinkSync(realBase, linkBase);
+
+      const destDir = path.join(linkBase, 'dest');
+      // realpathSync, not the realBase path: tempDir may itself sit behind a symlink.
+      const realDest = fs.realpathSync(path.join(realBase, 'dest'));
+      fs.symlinkSync(path.join(realDest, 'final.txt'), path.join(destDir, 'entry'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(
+        fs.readFileSync(path.join(destDir, 'entry'), 'utf8'),
+        'content',
+        'A target inside destDir should be accepted whichever namespace names it'
+      );
+    });
+  });
+
+  describe('symlink cycles', () => {
+    it('should terminate rather than loop', async () => {
+      const destDir = path.join(tempDir, 'dest');
+      fs.mkdirSync(destDir, { recursive: true });
+      fs.symlinkSync(path.join(destDir, 'b'), path.join(destDir, 'entry'));
+      fs.symlinkSync(path.join(destDir, 'entry'), path.join(destDir, 'b'));
+
+      const tarBuffer = await createTarBuffer([
+        { name: 'entry', type: 'file', content: 'content' },
+      ]);
+
+      await compressing.tar.uncompress(tarBuffer, destDir);
+
+      assert.strictEqual(fs.lstatSync(path.join(destDir, 'entry')).isSymbolicLink(), true);
+    });
+  });
+});