search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.38k stars 29.5k forks source link

zlib crashes when given windowBits: 8 #14847

Closed jcoglan closed 6 years ago

jcoglan commented 7 years ago

Version: 4.8.2, 6.10.2, 7.6+, 8.0+ Platform: Darwin liskov.home 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64 Subsystem: zlib

On the above versions of Node, this program crashes:

var zlib = require('zlib');
zlib.createDeflateRaw({windowBits: 8});

I believe this coincides with bumping zlib from 1.2.8 to 1.2.11, but I'm not 100% sure. I'm not sure if 8 is no longer a legal value, but zlib.Z_MIN_WINDOWBITS equals 8 on affected platforms.

NickNaso commented 7 years ago

From zlib documentation seems that current implementation of deflate algorithm do not support windowsBits value of 8. I report what i found from here https://www.zlib.net/manual.html

For the current implementation of deflate(), a windowBits value of 8 (a window size of 256 bytes) is not supported.

This behaviour is documented only on version 8 documentation: https://nodejs.org/dist/latest-v8.x/docs/api/zlib.html#zlib_zlib_createdeflateraw_options

addaleax commented 7 years ago

This is a duplicate of https://github.com/nodejs/node/issues/13082, I think.

Version: 4.8.2, 6.10.2, 7.6+, 8.0+

On 8+ it should throw an error, though? Can you make sure?

jcoglan commented 7 years ago

From zlib documentation seems that current implementation of deflate algorithm do not support windowsBits value of 8.

This is going to be a problem; permessage-deflate and the Autobahn test suite require that if a server is asked to only use a window size of N to send messages to the client, it commits to using either N or a lower value, including N=8.

This is a duplicate of #13082, I think.

It might be; I saw that issue and was unsure. Autobahn is the reason I found the error -- I maintain the permessage-deflate, websocket-driver and faye-websocket packages.

On 8+ it should throw an error, though? Can you make sure?

It does, and also on the latest 4.x and 6.x. On 7.x the error cannot be caught, and on 5.x the problem does not exist. My issue isn't so much about whether it throws a catchable exception so much as that we cannot set windowBits to 8.

Is this a new permanent limitation in zlib? Will other software need to take this limitation into account, and will Z_MIN_WINDOWBITS be updated?

refack commented 7 years ago

@jcoglan see https://github.com/nodejs/node/pull/13098#issuecomment-302361621 which leads to http://zlib.net/manual.html#Advanced (For the current implementation of deflate(), a windowBits value of 8 (a window size of 256 bytes) is not supported.)

jcoglan commented 7 years ago

@refack I've read that thread but there's a couple of things I still don't know:

addaleax commented 7 years ago

@jcoglan You might be interested in looking at https://github.com/madler/zlib/issues/171 :)

jcoglan commented 7 years ago

@addaleax Thank you, that was really helpful and has got me to make progress with my issue.

I now know from this source and the Node changelog that:

Does anybody know if this windowBits change is related to the CVEs, of if it's an unrelated change swept up in these releases? I'm having trouble understanding the details of the security reports.

MylesBorins commented 7 years ago

If the change is unrelated to the CVE should we try and patch support for LTS?

addaleax commented 7 years ago

I think we should patch that in LTS, yes (I assume just using 9 when 8 was passed would be the easiest and most sensible thing). @MylesBorins Can you do the PR(s)?

jcoglan commented 7 years ago

Does anyone know why zlib stopped accepting 8, if it's not related to a security thing? Only reason I hesitated to do this replacement in my library is that I figured there must be some reason zlib removed this behaviour.

addaleax commented 7 years ago

I think the linked zlib thread contains the relevant thoughts on that. If you want to be certain, I’d recommend asking them more directly, I don’t think any collaborators here know for sure.

MylesBorins commented 7 years ago

I'll try and dig into this a bit tomorrow unless someone is really amped to do it 😎

MylesBorins commented 7 years ago

SOOOOO this is fun

It would appear that zlib is already uses 9bits. The problems people are running into are edge cases that cause things to asplode.

This patch over on the optipng sourceforge claims to be a fix fix. I'm unsure of how this code would be licensed so I'm not provided a patch at this time. The copyright might belong to @ctruta who I believe works at IBM with @mhdawson.

The patch can be applied it to zlib master with minor conflicts. It was then possible to both build zlib and run the test suite. This patch could be cherry-picked to any of our release lines. I tested both master and v6.x and every thing seems alright. We did not appear to be testing this edge case, so we should try and get a test together that proves things are working for this specific window size.

The patch was originally meant to be applied 1.1.3 with the hope it would land in 1.1.4. A partial fix landed in 1.1.4 with a promise for a full fix in 1.1.5. The change was reverted in the next release 1.2.0. In 1.2.0.2 windowBit = 8 would be rounded up to 9 bits, this is the current state of master.

@mandler is there history about this patch that we should know?

edit: it is dawning on me right now that even with all my spelunking that I simply needed to test... 

var zlib = require('zlib');
zlib.createDeflateRaw({windowBits: 8});

and this was broken between 1.2.8 to 1.2.11...

going to do some bisecting. That was some fun history though 😄 ... still curious why the fix never landed

addaleax commented 7 years ago

It would appear that zlib is already uses 9bits. The problems people are running into are edge cases that cause things to asplode.

Just to make sure, you saw the check in the conditional before that, right? That comes from the change in 1.2.8 → 1.2.11 that broke behaviour for people in LTS.

Also, guessing here, but I think you may have wanted to ping @madler :)

MylesBorins commented 7 years ago

Found the commit the disabled 256 bytes explicitly.

https://github.com/madler/zlib/commit/049578f0a1849f502834167e233f4c1d52ddcbcc

Reject a window size of 256 bytes if not using the zlib wrapper.
There is a bug in deflate for windowBits == 8 (256-byte window).
As a result, zlib silently changes a request for 8 to a request
for 9 (512-byte window), and sets the zlib header accordingly so
that the decompressor knows to use a 512-byte window. However if
deflateInit2() is used for raw deflate or gzip streams, then there
is no indication that the request was not honored, and the
application might assume that it can use a 256-byte window when
decompressing. This commit returns an error if the user requests
a 256-byte window when using raw deflate or gzip encoding.

Reverting that patch gets things back to the behavior prior to the security update... it also look like there would not be security implications with allowing this... it is just a bit foot gun'y.

We have a couple options

We should likely figure out why the heck the original patch didn't land before doing anything. And get some tests together.

I'd like to suggest that for now we float a patch on v4.x and v6.x to revert expected behavior and do work with upstream to try and get the windowBit bug fixed

Thoughts?

addaleax commented 7 years ago

I think I’d prefer to float the revert of madler/zlib@049578f until this has a proper upstream fix.

Qix- commented 7 years ago

Do we know the window bits limitation doesn't have a security implication? I'd be wary if it was changed within a security release.

Since others have asked the same question, who authored the commit? Can we rope them into this discussion?

MylesBorins commented 7 years ago

@Qix- this was not changed in a security release or included in any of the CVEs mentioned above

lpinca commented 7 years ago

There is a bug in deflate for windowBits == 8 (256-byte window).

I don't think floating a revert of https://github.com/madler/zlib/commit/049578f0a1849f502834167e233f4c1d52ddcbcc is a good idea.

MylesBorins commented 7 years ago

/cc @nodejs/ctc @nodejs/lts

indutny commented 7 years ago

Unintentional major change, huh? :wink:

I think we should programmatically cancel this in our code in src/ folder. There is no need in patching zlib as it may make backporting further patches more painful.

MylesBorins commented 7 years ago

I created a PR upstream with the optipng fix to see what the deal is

https://github.com/madler/zlib/pull/291

refack commented 7 years ago

Unintentional major change, huh? 😉

So this regression was introduced when we bumped zlib from 1.2.8 → 1.2.11?

I always like to ask, what can we learn in order for something like this to not happen again? Assuming:

Only thing I can think of is to audit the deps' test suites, or do some sort of mix and match - try to run the old test suite on the new version...

@nodejs/lts any ideas?

Quick recap:

  1. 29 Mar, 2017 - https://github.com/nodejs/node/commit/abe913201180f106e054e1803eaa8a857ec3c423 zlib 1.2.11 is backported to v6.x
  2. 04 Apr, 2017 - v6.10.2 released with zlib 1.2.11
  3. 17 May, 2017 - https://github.com/nodejs/node/issues/13082 reports v6.10.2 crashes
  4. 18 May, 2017 - https://github.com/nodejs/node/pull/13098 It's attributed to a legitimate behaviour of the lib and guarded against.
  5. 17 Jun, 2017 - guard is backported.
  6. 15 Aug, 2017 - @jcoglan asks "why no windowBits: 8?"
MylesBorins commented 7 years ago

should we close this or keep it open until we decide what to do with master?

addaleax commented 7 years ago

@MylesBorins After the digging I did on Tuesday, I think we should apply the same patch to master: https://github.com/nodejs/node/pull/16511