Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when using fromBuffer() to read corrupt zip files that specify out of bounds file offsets #157

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Fix crash when using fromBuffer() to read corrupt zip files that specify out of bounds file offsets #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
363a345
link to hexdump-zip
thejoshwolfe Apr 18, 2024
7318aa6
test suite runs error tests through all open functions
thejoshwolfe Apr 19, 2024
1d9ccb6
fix buffer out of bounds crashing instead of emitting a clean error
thejoshwolfe Apr 18, 2024
49840a2
release notess. closes #156
thejoshwolfe Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -836,3 +839,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.
27 changes: 22 additions & 5 deletions fd-slicer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
};

Expand Down
156 changes: 86 additions & 70 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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");
}
});
});
});

Expand Down Expand Up @@ -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();
});
});
Expand All @@ -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);
});
});
Expand Down