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.

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

security: zlib inflate methods might be vulnerable to zip bombs #27253

Closed
jorangreef opened this issue Apr 16, 2019 · 3 comments
Closed

security: zlib inflate methods might be vulnerable to zip bombs #27253

jorangreef opened this issue Apr 16, 2019 · 3 comments
Labels
security Issues and PRs related to security. zlib Issues and PRs related to the zlib subsystem.

Comments

@jorangreef
Copy link
Contributor

From the documentation, it doesn't appear that there is any way to limit zlib's inflate methods max uncompressed size?

Without a way to limit the maximum amount of data to be uncompressed, Node's zlib inflate methods could be vulnerable to zip bombs, where a few megabytes of input could result in hundreds of megabytes of uncompressed data.

For parsers such as https://github.com/thejoshwolfe/yauzl, which parse the zip format, and which know the compressed and uncompressed sizes exactly, ahead of time, it should be possible to pass this information on to zlib's inflate methods, so that these can stop when they uncompress more than the zip container allows.

zlib's inflate methods should throw an error when more than maxUncompressedSize has been inflated.

See also: thejoshwolfe/yauzl#107

@addaleax addaleax added security Issues and PRs related to security. zlib Issues and PRs related to the zlib subsystem. labels Apr 16, 2019
@addaleax
Copy link
Member

addaleax commented Apr 16, 2019

With the streaming zlib methods, it is possible to limit output size – the chunkSize option controls the output buffer size (although there currently doesn’t seem to be documentation on what it does, only that it exists), and no new input will not be consumed when no output data is being read from the stream.

I don’t think there’s anything like this for the synchronous methods – it should be possible to add options for them as well, if you think that that helps.

@jorangreef
Copy link
Contributor Author

Thanks @addaleax , an option for the sync methods, as well as for the asynchronous one-shot methods, e.g. zlib.inflateRaw(), would help.

@jasnell
Copy link
Member

jasnell commented Apr 17, 2019

We can definitely look at this for the sync methods, but in the future, for potential security issues, even if you're not entirely sure if it is a security issue, please use our hackerone account to report it more discreetly :-) https://hackerone.com/nodejs

rosaxxny added a commit to rosaxxny/node that referenced this issue May 22, 2020
mhdawson pushed a commit to mhdawson/io.js that referenced this issue Jun 9, 2020
Fixes: nodejs#27253

PR-URL: nodejs#33516
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Fixes: #27253

PR-URL: #33516
Reviewed-By: Anna Henningsen <anna@addaleax.net>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Fixes: #27253

PR-URL: #33516
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Fixes: #27253

PR-URL: #33516
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Issues and PRs related to security. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants