zlib: reject ambiguous ZIP archive ends

ZIP archives can contain more than one structurally plausible EOCD
record. Selecting based on whether a comment reaches EOF can make the
chosen central directory depend on trailing padding.

Inspect all candidates in the common tail window and reject archives
with multiple plausible interpretations.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #65007
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Author: mcollina

lib/internal/zip/headers.js

@@ -27,6 +27,7 @@ const {
   MADE_BY_UNIX,
   SENTINEL16,
   SENTINEL32,
+  TAIL_LENGTH,
   ZIP64_EOCD_MAX_LENGTH,
   S_IFLNK,
   S_IFMT,
@@ -310,6 +311,51 @@ class LocalFileHeader {
   }
 }
 
+// Returns whether an EOCD-looking record could describe an archive this
+// implementation supports. This is deliberately only a cheap preflight: the
+// selected record still receives the complete Zip64 and central-directory
+// validation below.
+function isPlausibleArchiveEnd(buffer, eocdPos, scanStart) {
+  const diskNumber = buffer.readUInt16LE(eocdPos + 4);
+  const centralDirectoryDiskNumber = buffer.readUInt16LE(eocdPos + 6);
+  const diskRecords = buffer.readUInt16LE(eocdPos + 8);
+  const totalRecords = buffer.readUInt16LE(eocdPos + 10);
+  const centralDirectorySize = buffer.readUInt32LE(eocdPos + 12);
+  const centralDirectoryOffset = buffer.readUInt32LE(eocdPos + 16);
+  const needsZip64 =
+    diskNumber === SENTINEL16 ||
+    centralDirectoryDiskNumber === SENTINEL16 ||
+    diskRecords === SENTINEL16 ||
+    totalRecords === SENTINEL16 ||
+    centralDirectorySize === SENTINEL32 ||
+    centralDirectoryOffset === SENTINEL32;
+
+  const locatorPos = eocdPos - 20;
+  const hasZip64Locator = locatorPos >= 0 &&
+    buffer.readUInt32LE(locatorPos) === SIG_ZIP64_EOCD_LOCATOR;
+  if (needsZip64) return hasZip64Locator;
+  // A Zip64 end record may accompany authoritative, non-sentinel classic
+  // fields. Its central directory does not immediately precede this EOCD.
+  if (hasZip64Locator) return true;
+  if (
+    diskNumber !== 0 ||
+    centralDirectoryDiskNumber !== 0 ||
+    diskRecords !== totalRecords ||
+    totalRecords * 46 > centralDirectorySize
+  ) {
+    return false;
+  }
+
+  const centralDirectoryPos = eocdPos - centralDirectorySize;
+  if (centralDirectoryPos < scanStart) {
+    // Use the same logical tail window for memory- and file-backed archives.
+    // The directory is not available for this cheap preflight in either case.
+    return true;
+  }
+  if (totalRecords === 0) return centralDirectorySize === 0;
+  return buffer.readUInt32LE(centralDirectoryPos) === SIG_CENTRAL_FILE_HEADER;
+}
+
 /**
  * Locates and validates the end-of-archive structures (EOCD, and the Zip64
  * EOCD locator/record when present) in `buffer`. `base` is the absolute
@@ -334,26 +380,30 @@ function findArchiveEnd(buffer, base = 0) {
   if (buffer.length < 22) {
     throw new ERR_ZIP_INVALID_ARCHIVE('no end of central directory record found');
   }
-  const min = MathMax(0, buffer.length - (22 + SENTINEL16));
+  // Use the same tail-sized search window for full buffers and ZipFile's tail
+  // reads. Besides keeping candidate selection consistent, the extra tail
+  // slack permits a maximum-length comment followed by modest writer padding.
+  const min = MathMax(0, buffer.length - TAIL_LENGTH);
   let eocdPos = -1;
-  // Pass 1: the comment must reach exactly to the end of the buffer (this
-  // rejects a stray EOCD-looking signature inside an earlier comment).
+  let fallbackPos = -1;
+  let exactFallbackPos = -1;
   for (let pos = buffer.length - 22; pos >= min; pos--) {
     if (buffer.readUInt32LE(pos) !== SIG_EOCD) continue;
-    if (pos + 22 + buffer.readUInt16LE(pos + 20) !== buffer.length) continue;
+    const end = pos + 22 + buffer.readUInt16LE(pos + 20);
+    if (end > buffer.length) continue;
+    if (fallbackPos < 0) fallbackPos = pos;
+    if (end === buffer.length && exactFallbackPos < 0) exactFallbackPos = pos;
+    if (!isPlausibleArchiveEnd(buffer, pos, min)) continue;
+    if (eocdPos >= 0) {
+      throw new ERR_ZIP_INVALID_ARCHIVE(
+        'ambiguous end of central directory records');
+    }
     eocdPos = pos;
-    break;
   }
+  // Preserve the targeted validation errors for a sole malformed or
+  // unsupported candidate. Plausible candidates always take precedence.
   if (eocdPos < 0) {
-    // Pass 2: tolerate trailing padding after the EOCD (some streaming
-    // writers pad their output to a fixed block size); take the last
-    // candidate found.
-    for (let pos = buffer.length - 22; pos >= min; pos--) {
-      if (buffer.readUInt32LE(pos) !== SIG_EOCD) continue;
-      if (pos + 22 + buffer.readUInt16LE(pos + 20) > buffer.length) continue;
-      eocdPos = pos;
-      break;
-    }
+    eocdPos = exactFallbackPos >= 0 ? exactFallbackPos : fallbackPos;
   }
   if (eocdPos < 0) {
     throw new ERR_ZIP_INVALID_ARCHIVE('no end of central directory record found');
@@ -474,7 +524,10 @@ function findArchiveEnd(buffer, base = 0) {
   if (prefix < 0) {
     throw new ERR_ZIP_INVALID_ARCHIVE('central directory does not fit inside the archive');
   }
-  if (totalRecords * 46 > centralDirectorySize) {
+  if (
+    (totalRecords === 0 && centralDirectorySize !== 0) ||
+    totalRecords * 46 > centralDirectorySize
+  ) {
     throw new ERR_ZIP_INVALID_ARCHIVE(
       'central directory record count is inconsistent with its size');
   }

test/parallel/test-zlib-zip-hardening.js

@@ -3,8 +3,10 @@
 require('../common');
 
 const assert = require('node:assert');
+const fs = require('node:fs');
 const zlib = require('node:zlib');
 const { test } = require('node:test');
+const tmpdir = require('../common/tmpdir');
 
 async function buildArchive(entries, comment) {
   const chunks = [];
@@ -47,13 +49,71 @@ test('an EOCD-looking signature inside a trailing comment is not mistaken for th
   // before it reaches the genuine EOCD signature; embedding 4 bytes that
   // look like one partway through must not be mistaken for the real record.
   const fakeSignature = String.fromCharCode(0x50, 0x4b, 0x05, 0x06);
-  const archive = await buildArchive([entry], `before ${fakeSignature} after`);
+  const archive = await buildArchive(
+    [entry], `before ${fakeSignature} this is not a valid EOCD record after`);
 
   const read = [...zlib.ZipEntry.read(archive)];
   assert.strictEqual(read.length, 1);
   assert.strictEqual(read[0].name, 'f.txt');
 });
 
+test('multiple plausible EOCD records describing different archives are rejected', async () => {
+  const first = await buildArchive([
+    await zlib.ZipEntry.create('install.sh', Buffer.from('malicious'), { method: 'store' }),
+  ]);
+  const second = await buildArchive([
+    await zlib.ZipEntry.create('install.sh', Buffer.from('benign'), { method: 'store' }),
+  ]);
+  const archive = Buffer.concat([first, second, Buffer.from([0])]);
+  const firstEocd = first.length - 22;
+
+  // Make the first EOCD exact-to-EOF by treating the second archive and its
+  // padding as a comment. The second EOCD remains a plausible archive end for
+  // readers which tolerate trailing padding and select the rightmost record.
+  archive.writeUInt16LE(archive.length - firstEocd - 22, firstEocd + 20);
+
+  const expected = {
+    code: 'ERR_ZIP_INVALID_ARCHIVE',
+    message: /ambiguous end of central directory/,
+  };
+  assert.throws(() => [...zlib.ZipEntry.read(archive)], expected);
+  assert.throws(() => new zlib.ZipBuffer(archive), expected);
+
+  tmpdir.refresh();
+  const file = tmpdir.resolve('ambiguous.zip');
+  fs.writeFileSync(file, archive);
+  await assert.rejects(zlib.ZipFile.open(file), expected);
+  assert.throws(() => zlib.ZipFile.openSync(file), expected);
+});
+
+test('an exact EOCD embedded in a genuine comment is rejected as ambiguous', async () => {
+  const archive = await buildArchive([
+    await zlib.ZipEntry.create('f.txt', Buffer.from('content'), { method: 'store' }),
+  ]);
+  const nested = Buffer.concat([archive, buildEocd()]);
+  nested.writeUInt16LE(22, archive.length - 2);
+
+  assert.throws(() => [...zlib.ZipEntry.read(nested)], {
+    code: 'ERR_ZIP_INVALID_ARCHIVE',
+    message: /ambiguous end of central directory/,
+  });
+});
+
+test('multiple padded EOCD records are rejected as ambiguous', async () => {
+  const first = await buildArchive([
+    await zlib.ZipEntry.create('a.txt', Buffer.from('first'), { method: 'store' }),
+  ]);
+  const second = await buildArchive([
+    await zlib.ZipEntry.create('b.txt', Buffer.from('second'), { method: 'store' }),
+  ]);
+  const archive = Buffer.concat([first, second, Buffer.from('\0\0')]);
+
+  assert.throws(() => new zlib.ZipBuffer(archive), {
+    code: 'ERR_ZIP_INVALID_ARCHIVE',
+    message: /ambiguous end of central directory/,
+  });
+});
+
 test('a declared-size mismatch is rejected as corrupt', async () => {
   const entry = await zlib.ZipEntry.create('f.txt', Buffer.from('hello world'), { method: 'store' });
   const archive = await buildArchive([entry]);
@@ -212,7 +272,7 @@ test('every possible truncation of an archive is rejected, deterministically', a
 
 test('trailing padding after the EOCD is tolerated', async () => {
   // Some streaming writers pad their output to a block size; CPython
-  // tolerates trailing newlines/NULs and so does the pass-2 EOCD scan.
+  // tolerates trailing newlines/NULs and so does the EOCD scan.
   const archive = await buildArchive(
     [await zlib.ZipEntry.create('f.txt', Buffer.from('hi'), { method: 'store' })]);
   const padded = Buffer.concat([archive, Buffer.from('\r\n\0\0\0')]);
@@ -221,6 +281,15 @@ test('trailing padding after the EOCD is tolerated', async () => {
   assert.strictEqual((await entry.content()).toString(), 'hi');
 });
 
+test('a maximum-length comment followed by block padding is tolerated', async () => {
+  const comment = 'x'.repeat(0xffff);
+  const archive = await buildArchive([], comment);
+  const padded = Buffer.concat([archive, Buffer.alloc(4096)]);
+  const zip = new zlib.ZipBuffer(padded);
+
+  assert.strictEqual(zip.comment, comment);
+});
+
 test('junk appended past a declared comment is tolerated and the comment preserved', async () => {
   const archive = await buildArchive(
     [await zlib.ZipEntry.create('f.txt', Buffer.from('hi'), { method: 'store' })],
@@ -262,6 +331,10 @@ test('a record count inconsistent with the directory size is rejected', () => {
   const archive = Buffer.concat([Buffer.alloc(46), eocd]);
   assert.throws(() => [...zlib.ZipEntry.read(archive)],
                 { code: 'ERR_ZIP_INVALID_ARCHIVE', message: /inconsistent/ });
+
+  const zeroRecords = Buffer.concat([Buffer.alloc(46), buildEocd({ cdSize: 46 })]);
+  assert.throws(() => [...zlib.ZipEntry.read(zeroRecords)],
+                { code: 'ERR_ZIP_INVALID_ARCHIVE', message: /inconsistent/ });
 });
 
 test('a corrupted or overrunning central directory header is rejected', async () => {