zlib crashes when given windowBits: 8 #14847

jcoglan · 2017-08-16T00:33:46Z

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.

The text was updated successfully, but these errors were encountered:

NickNaso · 2017-08-16T08:04:37Z

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 · 2017-08-16T08:46:30Z

This is a duplicate of #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 · 2017-08-16T17:58:32Z

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 · 2017-08-16T18:14:13Z

@jcoglan see #13098 (comment)
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 · 2017-08-16T22:27:42Z

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

Why is 8 no longer a valid windowBits value?
What are my options as someone maintaining a library that is supposed to support windowBits=8?

addaleax · 2017-08-16T22:32:28Z

@jcoglan You might be interested in looking at madler/zlib#171 :)

jcoglan · 2017-08-17T21:24:17Z

@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:

in Node v4.8.2, v6.10.2, and v8.0.0, zlib is bumped from 1.2.8 to 1.2.11 due to some CVEs
some time between these versions, zlib stopped accepting windowBits=8 for raw streams

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 · 2017-08-17T21:58:23Z

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

addaleax · 2017-08-17T22:49:06Z

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 · 2017-08-17T22:54:18Z

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 · 2017-08-17T22:56:17Z

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 · 2017-08-18T02:06:54Z

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

MylesBorins · 2017-08-18T09:43:42Z

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 · 2017-08-18T09:48:44Z

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 · 2017-08-18T10:25:35Z

Found the commit the disabled 256 bytes explicitly.

madler/zlib@049578f

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

Leave it as is because it is expected behavior in zlib
float a revert to madler/zlib@049578f
fix windowBits = 8 by landing optipng fix, reverting madler/zlib@049578f, and removing this line

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 · 2017-08-18T10:34:27Z

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

Qix- · 2017-08-18T10:37:48Z

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 · 2017-08-18T11:24:19Z

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

lpinca · 2017-08-18T12:01:17Z

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

I don't think floating a revert of madler/zlib@049578f is a good idea.

MylesBorins · 2017-08-18T19:21:48Z

/cc @nodejs/ctc @nodejs/lts

indutny · 2017-08-18T19:44:36Z

Unintentional major change, huh? 😉

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 · 2017-08-18T21:19:22Z

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

madler/zlib#291

refack · 2017-08-19T04:50:39Z

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:

We can't extend our test suite enough to cover the deps
Even if we would have run zlib's test suite I'm assuming they would have changed any tests that use windowBits: 8
Human eyes reviewing release notes are error prone

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:

29 Mar, 2017 - abe9132 zlib 1.2.11 is backported to v6.x
04 Apr, 2017 - v6.10.2 released with zlib 1.2.11
17 May, 2017 - zlib assertion error #13082 reports v6.10.2 crashes
18 May, 2017 - zlib: fix node crashing on invalid options #13098 It's attributed to a legitimate behaviour of the lib and guarded against.
17 Jun, 2017 - guard is backported.
15 Aug, 2017 - @jcoglan asks "why no windowBits: 8?"

MylesBorins · 2017-10-26T03:43:24Z

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

addaleax · 2017-10-26T10:38:23Z

@MylesBorins After the digging I did on Tuesday, I think we should apply the same patch to master: #16511

Fixes: nodejs#14847 Original-PR-URL: nodejs#16511 Original-Reviewed-By: Luigi Pinca <[email protected]> Original-Reviewed-By: James M Snell <[email protected]> Original-Reviewed-By: Colin Ihrig <[email protected]> Original-Reviewed-By: Michael Dawson <[email protected]> Original-Reviewed-By: Refael Ackermann <[email protected]>

Fixes: nodejs#14847 PR-URL: nodejs#16511 Backport-PR-URL: nodejs#16623 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Fixes: #14847 PR-URL: #16511 Backport-PR-URL: #16623 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Fixes: nodejs/node#14847 PR-URL: nodejs/node#16511 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Fixes: #14847 PR-URL: #16511 Backport-PR-URL: #16623 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Fixes: nodejs/node#14847 PR-URL: nodejs/node#16511 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Aug 16, 2017

addaleax added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Aug 16, 2017

refack added question Issues that look for answers. and removed duplicate Issues and PRs that are duplicates of other issues or PRs. labels Aug 19, 2017

addaleax closed this as completed in 25ef9d2 Oct 29, 2017

addaleax mentioned this issue Oct 30, 2017

[v8.x] doc: more accurate zlib windowBits information #16623

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib crashes when given windowBits: 8 #14847

zlib crashes when given windowBits: 8 #14847

jcoglan commented Aug 16, 2017

NickNaso commented Aug 16, 2017 •

edited

Loading

addaleax commented Aug 16, 2017

jcoglan commented Aug 16, 2017

refack commented Aug 16, 2017

jcoglan commented Aug 16, 2017

addaleax commented Aug 16, 2017

jcoglan commented Aug 17, 2017

MylesBorins commented Aug 17, 2017

addaleax commented Aug 17, 2017

jcoglan commented Aug 17, 2017

addaleax commented Aug 17, 2017

MylesBorins commented Aug 18, 2017

MylesBorins commented Aug 18, 2017 •

edited

Loading

addaleax commented Aug 18, 2017

MylesBorins commented Aug 18, 2017 •

edited

Loading

addaleax commented Aug 18, 2017

Qix- commented Aug 18, 2017 •

edited

Loading

MylesBorins commented Aug 18, 2017 •

edited

Loading

lpinca commented Aug 18, 2017

MylesBorins commented Aug 18, 2017

indutny commented Aug 18, 2017

MylesBorins commented Aug 18, 2017

refack commented Aug 19, 2017 •

edited

Loading

MylesBorins commented Oct 26, 2017

addaleax commented Oct 26, 2017

zlib crashes when given windowBits: 8 #14847

zlib crashes when given windowBits: 8 #14847

Comments

jcoglan commented Aug 16, 2017

NickNaso commented Aug 16, 2017 • edited Loading

addaleax commented Aug 16, 2017

jcoglan commented Aug 16, 2017

refack commented Aug 16, 2017

jcoglan commented Aug 16, 2017

addaleax commented Aug 16, 2017

jcoglan commented Aug 17, 2017

MylesBorins commented Aug 17, 2017

addaleax commented Aug 17, 2017

jcoglan commented Aug 17, 2017

addaleax commented Aug 17, 2017

MylesBorins commented Aug 18, 2017

MylesBorins commented Aug 18, 2017 • edited Loading

addaleax commented Aug 18, 2017

MylesBorins commented Aug 18, 2017 • edited Loading

addaleax commented Aug 18, 2017

Qix- commented Aug 18, 2017 • edited Loading

MylesBorins commented Aug 18, 2017 • edited Loading

lpinca commented Aug 18, 2017

MylesBorins commented Aug 18, 2017

indutny commented Aug 18, 2017

MylesBorins commented Aug 18, 2017

refack commented Aug 19, 2017 • edited Loading

MylesBorins commented Oct 26, 2017

addaleax commented Oct 26, 2017

NickNaso commented Aug 16, 2017 •

edited

Loading

MylesBorins commented Aug 18, 2017 •

edited

Loading

MylesBorins commented Aug 18, 2017 •

edited

Loading

Qix- commented Aug 18, 2017 •

edited

Loading

MylesBorins commented Aug 18, 2017 •

edited

Loading

refack commented Aug 19, 2017 •

edited

Loading