Skip to content

Commit 745ad77

Browse files
mcollinaRafaelGSS
authored andcommitted
zlib: avoid caching invalid ZIP local headers
The lazy file-backed ZIP resolver cached a parsed local header before checking it against the central directory. A failed check therefore left the entry with a cached header and unresolved content offset, allowing a later read to use the file descriptor's current position. Validate the local header and calculate its content offset before committing either value to the entry. Assisted-by: Pi Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#981 Refs: https://hackerone.com/reports/4004290 Reviewed-By: Robert Nagy <ronagy@icloud.com> CVE-ID: CVE-2026-106070
1 parent ad788cc commit 745ad77

2 files changed

Lines changed: 123 additions & 8 deletions

File tree

‎lib/internal/zip/entry.js‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -410,12 +410,14 @@ class ZipEntry {
410410
if (length > 30) {
411411
await readFdFully(fd, full.subarray(30), this.#localOffset + 30);
412412
}
413-
this.#local = new LocalFileHeader(full, 0);
413+
const local = new LocalFileHeader(full, 0);
414414
if (this.#central !== null) {
415-
assertConsistentLocalHeader(this.#local, this.#central);
415+
assertConsistentLocalHeader(local, this.#central);
416416
}
417-
this.#contentOffset = this.#localOffset + length;
418-
return this.#local;
417+
const contentOffset = this.#localOffset + length;
418+
this.#local = local;
419+
this.#contentOffset = contentOffset;
420+
return local;
419421
}
420422
// Sync counterpart of #resolveLocalHeader().
421423
#resolveLocalHeaderSync() {
@@ -433,12 +435,14 @@ class ZipEntry {
433435
if (length > 30) {
434436
readFdFullySync(fd, full.subarray(30), this.#localOffset + 30);
435437
}
436-
this.#local = new LocalFileHeader(full, 0);
438+
const local = new LocalFileHeader(full, 0);
437439
if (this.#central !== null) {
438-
assertConsistentLocalHeader(this.#local, this.#central);
440+
assertConsistentLocalHeader(local, this.#central);
439441
}
440-
this.#contentOffset = this.#localOffset + length;
441-
return this.#local;
442+
const contentOffset = this.#localOffset + length;
443+
this.#local = local;
444+
this.#contentOffset = contentOffset;
445+
return local;
442446
}
443447
// The offset where this file-backed entry's compressed data begins, reading
444448
// the local header first if that has not been resolved yet.

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

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ require('../common');
1818
const assert = require('assert');
1919
const { test } = require('node:test');
2020
const zlib = require('zlib');
21+
const fs = require('fs');
2122
const path = require('path');
2223
const { spawnSync } = require('child_process');
2324
const tmpdir = require('../common/tmpdir');
@@ -76,6 +77,116 @@ test('a local/central encryption-flag disagreement is rejected', () => {
7677
readsThrow(buf, 'a.txt');
7778
});
7879

80+
// 1d. A failed lazy local-header check must not leave a file-backed entry in
81+
// partially resolved state. Otherwise, a later read uses -1 as its file offset
82+
// and reads from the descriptor's current position.
83+
test('failed local-header validation does not poison file-backed entries', async () => {
84+
const decoy = Buffer.from('SCANNER-SEES-BENIGN!!');
85+
const payload = Buffer.from('UNZIP-SEES-PAYLOAD!!!');
86+
const entry = zlib.ZipEntry.createSync(
87+
'entry.txt', payload, { method: 'store' });
88+
const chunks = [];
89+
for (const chunk of zlib.createZipArchiveSync(
90+
[entry], { baseOffset: decoy.length })) {
91+
chunks.push(chunk);
92+
}
93+
const buf = Buffer.concat([decoy, ...chunks]);
94+
buf.writeUInt32LE(zlib.crc32(decoy), centralOffset(buf) + 16);
95+
96+
tmpdir.refresh();
97+
const file = path.join(tmpdir.path, 'mismatched.zip');
98+
fs.writeFileSync(file, buf);
99+
const invalid = { code: 'ERR_ZIP_INVALID_ARCHIVE' };
100+
101+
{
102+
const zip = zlib.ZipFile.openSync(file);
103+
try {
104+
const fileEntry = zip.getSync('entry.txt');
105+
assert.ok(fileEntry.modified instanceof Date);
106+
assert.throws(() => fileEntry.contentSync(), invalid);
107+
assert.throws(() => fileEntry.contentSync(), invalid);
108+
} finally {
109+
zip.closeSync();
110+
}
111+
}
112+
113+
{
114+
const zip = zlib.ZipFile.openSync(file);
115+
try {
116+
const fileEntry = zip.getSync('entry.txt');
117+
assert.throws(() => fileEntry.contentSync(), invalid);
118+
assert.throws(() => fileEntry.contentSync(), invalid);
119+
} finally {
120+
zip.closeSync();
121+
}
122+
}
123+
124+
{
125+
const zip = await zlib.ZipFile.open(file);
126+
try {
127+
const fileEntry = await zip.get('entry.txt');
128+
assert.ok(fileEntry.modified instanceof Date);
129+
await assert.rejects(fileEntry.content(), invalid);
130+
await assert.rejects(fileEntry.content(), invalid);
131+
} finally {
132+
await zip.close();
133+
}
134+
}
135+
136+
{
137+
const zip = await zlib.ZipFile.open(file);
138+
try {
139+
const fileEntry = await zip.get('entry.txt');
140+
await assert.rejects(fileEntry.content(), invalid);
141+
await assert.rejects(fileEntry.content(), invalid);
142+
} finally {
143+
await zip.close();
144+
}
145+
}
146+
147+
{
148+
const zip = await zlib.ZipFile.open(file);
149+
try {
150+
const fileEntry = await zip.get('entry.txt');
151+
assert.ok(fileEntry.modified instanceof Date);
152+
await assert.rejects(async () => {
153+
for await (const chunk of fileEntry.contentIterator()) {
154+
assert.ok(Buffer.isBuffer(chunk));
155+
}
156+
}, invalid);
157+
await assert.rejects(async () => {
158+
for await (const chunk of await zip.stream('entry.txt')) {
159+
assert.ok(Buffer.isBuffer(chunk));
160+
}
161+
}, invalid);
162+
} finally {
163+
await zip.close();
164+
}
165+
}
166+
167+
{
168+
const zip = zlib.ZipFile.openSync(file);
169+
try {
170+
assert.throws(() => zip.compactSync(), invalid);
171+
} finally {
172+
zip.closeSync();
173+
}
174+
}
175+
176+
{
177+
const zip = await zlib.ZipFile.open(file);
178+
try {
179+
await assert.rejects(async () => {
180+
for await (const chunk of zip.compact()) {
181+
assert.ok(Buffer.isBuffer(chunk));
182+
}
183+
}, invalid);
184+
} finally {
185+
await zip.close();
186+
}
187+
}
188+
});
189+
79190
// 2. zipFiles must reject a FIFO source rather than block on open() forever.
80191
test('zipFiles rejects a FIFO source instead of hanging', () => {
81192
if (process.platform === 'win32') return; // no mkfifo

0 commit comments

Comments
 (0)