From 363a34504b04ef98f0c18ddd63f0a616624c72dd Mon Sep 17 00:00:00 2001 From: Josh Wolfe Date: Thu, 18 Apr 2024 09:12:04 -0400 Subject: [PATCH 1/4] link to hexdump-zip --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index 65f35bc..ab28f8c 100644 --- a/README.md +++ b/README.md @@ -836,3 +836,11 @@ The zip file specification has several ambiguities inherent in its design. Yikes * Fix bug with using `iconv`. * 2.0.0 * Initial release. + +## Development + +One of the trickiest things in development is crafting test cases located in `test/{success,failure}/`. +These are zip files that have been specifically generated or design to test certain conditions in this library. +I recommend using [hexdump-zip](https://github.com/thejoshwolfe/hexdump-zip) to examine the structure of a zipfile. + +For making new error cases, I typically start by copying `test/success/linux-info-zip.zip`, and then editing a few bytes with a hex editor. From 7318aa60f4ef21ba691d0d6bd28d5caf8818cd09 Mon Sep 17 00:00:00 2001 From: Josh Wolfe Date: Fri, 19 Apr 2024 07:00:07 -0400 Subject: [PATCH 2/4] test suite runs error tests through all open functions --- test/test.js | 156 ++++++++++++++++++++++++++++----------------------- 1 file changed, 86 insertions(+), 70 deletions(-) diff --git a/test/test.js b/test/test.js index ef86264..bf94560 100644 --- a/test/test.js +++ b/test/test.js @@ -25,6 +25,19 @@ function shouldDoTest(testPath) { return args.indexOf(testPath) !== -1; } +var openFunctions = [ + function(zipfilePath, testId, options, callback) { yauzl.open(zipfilePath, options, callback); }, + function(zipfilePath, testId, options, callback) { yauzl.fromBuffer(fs.readFileSync(zipfilePath), options, callback); }, + function(zipfilePath, testId, options, callback) { openWithRandomAccess(zipfilePath, options, true, testId, callback); }, + function(zipfilePath, testId, options, callback) { openWithRandomAccess(zipfilePath, options, false, testId, callback); }, +]; +var openFunctionNames = [ + "fd", + "buffer", + "randomAccess", + "minimalRandomAccess", +]; + // success tests listZipFiles([path.join(__dirname, "success"), path.join(__dirname, "wrong-entry-sizes")]).forEach(function(zipfilePath) { if (!shouldDoTest(zipfilePath)) return; @@ -38,15 +51,9 @@ listZipFiles([path.join(__dirname, "success"), path.join(__dirname, "wrong-entry options.validateEntrySizes = false; }); } - var openFunctions = [ - function(testId, options, callback) { yauzl.open(zipfilePath, options, callback); }, - function(testId, options, callback) { yauzl.fromBuffer(fs.readFileSync(zipfilePath), options, callback); }, - function(testId, options, callback) { openWithRandomAccess(zipfilePath, options, true, testId, callback); }, - function(testId, options, callback) { openWithRandomAccess(zipfilePath, options, false, testId, callback); }, - ]; openFunctions.forEach(function(openFunction, i) { optionConfigurations.forEach(function(options, j) { - var testId = zipfilePath + "(" + ["fd", "buffer", "randomAccess", "minimalRandomAccess"][i] + "," + j + "): "; + var testId = zipfilePath + "(" + openFunctionNames[i] + "," + j + "): "; var expectedPathPrefix = zipfilePath.replace(/\.zip$/, ""); var expectedArchiveContents = {}; var DIRECTORY = 1; // not a string @@ -77,7 +84,7 @@ listZipFiles([path.join(__dirname, "success"), path.join(__dirname, "wrong-entry } } pend.go(function(zipfileCallback) { - openFunction(testId, options, function(err, zipfile) { + openFunction(zipfilePath, testId, options, function(err, zipfile) { if (err) throw err; zipfile.readEntry(); zipfile.on("entry", function(entry) { @@ -163,68 +170,77 @@ listZipFiles([path.join(__dirname, "success"), path.join(__dirname, "wrong-entry listZipFiles([path.join(__dirname, "failure")]).forEach(function(zipfilePath) { if (!shouldDoTest(zipfilePath)) return; var expectedErrorMessage = path.basename(zipfilePath).replace(/(_\d+)?\.zip$/, ""); - var failedYet = false; - var emittedError = false; - pend.go(function(cb) { - var operationsInProgress = 0; - if (/invalid characters in fileName/.test(zipfilePath)) { - // this error can only happen when you specify an option - yauzl.open(zipfilePath, {strictFileNames: true}, onZipFile); - } else { - yauzl.open(zipfilePath, onZipFile); - } - return; - - function onZipFile(err, zipfile) { - if (err) return checkErrorMessage(err); - zipfile.on("error", function(err) { - noEventsAllowedAfterError(); - emittedError = true; - checkErrorMessage(err); - }); - zipfile.on("entry", function(entry) { - noEventsAllowedAfterError(); - // let's also try to read directories, cuz whatever. - operationsInProgress += 1; - zipfile.openReadStream(entry, function(err, stream) { - if (err) return checkErrorMessage(err); - stream.on("error", function(err) { - checkErrorMessage(err); - }); - stream.on("data", function(data) { - // ignore - }); - stream.on("end", function() { - doneWithSomething(); + + openFunctions.forEach(function(openFunction, i) { + var testId = zipfilePath + "(" + openFunctionNames[i] + "): "; + var failedYet = false; + var emittedError = false; + pend.go(function(cb) { + var operationsInProgress = 0; + var options = null; + if (/invalid characters in fileName/.test(zipfilePath)) { + // this error can only happen when you specify an option + options = {strictFileNames: true}; + } + openFunction(zipfilePath, testId, options, onZipFile); + + function onZipFile(err, zipfile) { + if (err) return checkErrorMessage(err); + zipfile.on("error", function(err) { + noEventsAllowedAfterError(); + emittedError = true; + checkErrorMessage(err); + }); + zipfile.on("entry", function(entry) { + noEventsAllowedAfterError(); + // let's also try to read directories, cuz whatever. + operationsInProgress += 1; + zipfile.openReadStream(entry, function(err, stream) { + if (err) return checkErrorMessage(err); + stream.on("error", function(err) { + checkErrorMessage(err); + }); + stream.on("data", function(data) { + // ignore + }); + stream.on("end", function() { + doneWithSomething(); + }); }); }); - }); - operationsInProgress += 1; - zipfile.on("end", function() { - noEventsAllowedAfterError(); - doneWithSomething(); - }); - function doneWithSomething() { - operationsInProgress -= 1; - if (operationsInProgress !== 0) return; - if (!failedYet) { - throw new Error(zipfilePath + ": expected failure"); + operationsInProgress += 1; + zipfile.on("end", function() { + noEventsAllowedAfterError(); + doneWithSomething(); + }); + function doneWithSomething() { + operationsInProgress -= 1; + if (operationsInProgress !== 0) return; + if (!failedYet) { + throw new Error(testId + "expected failure"); + } } } - } - function checkErrorMessage(err) { - var actualMessage = err.message.replace(/[^0-9A-Za-z-]+/g, " ").trimRight(); - if (actualMessage !== expectedErrorMessage) { - throw new Error(zipfilePath + ": wrong error message: " + actualMessage); + function checkErrorMessage(err) { + var actualMessage = err.message.replace(/[^0-9A-Za-z-]+/g, " ").trimRight(); + if (actualMessage !== expectedErrorMessage) { + if (i !== 0) { + // The error messages are tuned for the common case. + // Sometimes other open functions give slightly different error messages, and that's ok, + // as long as we're still getting some error. + } else { + throw new Error(testId + "wrong error message: " + actualMessage); + } + } + console.log(testId + "pass"); + failedYet = true; + operationsInProgress = -Infinity; + cb(); } - console.log(zipfilePath + ": pass"); - failedYet = true; - operationsInProgress = -Infinity; - cb(); - } - function noEventsAllowedAfterError() { - if (emittedError) throw new Error("events emitted after error event"); - } + function noEventsAllowedAfterError() { + if (emittedError) throw new Error(testId + "events emitted after error event"); + } + }); }); }); @@ -471,11 +487,11 @@ function openWithRandomAccess(zipfilePath, options, implementRead, testId, callb if (implementRead) { InefficientRandomAccessReader.prototype.read = function(buffer, offset, length, position, callback) { fs.open(zipfilePath, "r", function(err, fd) { - if (err) throw err; + if (err) return callback(err); fs.read(fd, buffer, offset, length, position, function(err, bytesRead) { - if (bytesRead < length) throw new Error("unexpected EOF"); + if (bytesRead < length) return callback(new Error("unexpected EOF")); fs.close(fd, function(err) { - if (err) throw err; + if (err) return callback(err); callback(); }); }); @@ -488,10 +504,10 @@ function openWithRandomAccess(zipfilePath, options, implementRead, testId, callb }; fs.stat(zipfilePath, function(err, stats) { - if (err) throw err; + if (err) return callback(err); var reader = new InefficientRandomAccessReader(); yauzl.fromRandomAccessReader(reader, stats.size, options, function(err, zipfile) { - if (err) throw err; + if (err) return callback(err); callback(null, zipfile); }); }); From 1d9ccb697033bb834af34459c3e07c8fc3fcd905 Mon Sep 17 00:00:00 2001 From: Josh Wolfe Date: Thu, 18 Apr 2024 09:04:24 -0400 Subject: [PATCH 3/4] fix buffer out of bounds crashing instead of emitting a clean error --- fd-slicer.js | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/fd-slicer.js b/fd-slicer.js index 56f557b..956a979 100644 --- a/fd-slicer.js +++ b/fd-slicer.js @@ -195,12 +195,29 @@ function BufferSlicer(buffer, options) { } BufferSlicer.prototype.read = function(buffer, offset, length, position, callback) { - var end = position + length; - var delta = end - this.buffer.length; - var written = (delta > 0) ? delta : length; - this.buffer.copy(buffer, offset, position, end); + if (!(0 <= offset && offset <= buffer.length)) throw new RangeError("offset outside buffer: 0 <= " + offset + " <= " + buffer.length); + if (position < 0) throw new RangeError("position is negative: " + position); + if (offset + length > buffer.length) { + // The caller's buffer can't hold all the bytes they're trying to read. + // Clamp the length instead of giving an error. + // The callback will be informed of fewer than expected bytes written. + length = buffer.length - offset; + } + if (position + length > this.buffer.length) { + // Clamp any attempt to read past the end of the source buffer. + length = this.buffer.length - position; + } + if (length <= 0) { + // After any clamping, we're fully out of bounds or otherwise have nothing to do. + // This isn't an error; it's just zero bytes written. + setImmediate(function() { + callback(null, 0); + }); + return; + } + this.buffer.copy(buffer, offset, position, position + length); setImmediate(function() { - callback(null, written); + callback(null, length); }); }; From 49840a2cb04f4da6b2db4f81168ff2a378457fae Mon Sep 17 00:00:00 2001 From: Josh Wolfe Date: Fri, 19 Apr 2024 07:09:45 -0400 Subject: [PATCH 4/4] release notess. closes #156 --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index ab28f8c..8c99e66 100644 --- a/README.md +++ b/README.md @@ -763,6 +763,9 @@ The zip file specification has several ambiguities inherent in its design. Yikes ## Change History + * 3.1.3 + * Fixed a crash when using `fromBuffer()` to read corrupt zip files that specify out of bounds file offsets. [issue #156](https://github.com/thejoshwolfe/yauzl/pull/156) + * Enahnced the test suite to run the error tests through `fromBuffer()` and `fromRandomAccessReader()` in addition to `open()`, which would have caught the above. * 3.1.2 * Fixed handling non-64 bit entries (similar to the version 3.1.1 fix) that actually have exactly 0xffffffff values in the fields. This fixes erroneous "expected zip64 extended information extra field" errors. [issue #109](https://github.com/thejoshwolfe/yauzl/pull/109) * 3.1.1