zlib: prevent uncaught exception in zlibBuffer #1811

targos · 2015-05-27T11:37:05Z

If the final buffer needs to be larger than kMaxLength, the concatenation fails and there is no way to catch the error:

buffer.js:173
    throw new RangeError('Attempt to allocate Buffer larger than maximum ' +
          ^
RangeError: Attempt to allocate Buffer larger than maximum size: 0x3fffffff bytes
    at checked (buffer.js:173:11)
    at fromNumber (buffer.js:56:51)
    at new Buffer (buffer.js:41:5)
    at Function.Buffer.concat (buffer.js:263:16)
    at Gunzip.onEnd (zlib.js:212:22)
    at emitNone (events.js:72:20)
    at Gunzip.emit (events.js:163:7)
    at endReadableNT (_stream_readable.js:890:12)
    at doNTCallback2 (node.js:437:9)
    at process._tickCallback (node.js:351:17)

Testcase : https://gist.github.com/targos/643a802e0ed4fa60f3bd

cjihrig · 2015-05-27T15:31:32Z

Can you add a test?

cjihrig · 2015-05-27T15:34:33Z

lib/zlib.js

+      callback(e);
+    } finally {
+      engine.close();
+    }


Could you do something more like:

err = null; try { buf = Buffer.concat(buffers, nread); } catch (e) { err = e; } buffers = []; engine.close(); callback(err, buf);

targos · 2015-05-27T18:44:22Z

Do you mean test that the error is correctly caught ?
I'm not sure how to do that. The unzipped data has to be more than 1GB, wouldn't it fail on hardware like Raspberry Pi ?

cjihrig · 2015-05-28T02:23:21Z

There is an upcoming Buffer rewrite that, if I understand correctly, will get rid of the upper limit anyway. @trevnorris, do you have any thoughts for a test here?

trevnorris · 2015-06-04T04:10:40Z

The check should be there. But if we're confident that nread is the length of all buffers then only need to check it against kMaxLength, and return an error if greater than. No need for the try catch.

targos · 2015-06-04T07:03:01Z

@trevnorris Good point, I made that change. Also added a check in the sync code to generate the same error.
I still don't know how to test it without allocating more than 1GB of buffers.

trevnorris · 2015-06-04T07:19:46Z

True. And that may be a problem on ARM devices.

@rvagg Thoughts?

rvagg · 2015-06-04T07:32:16Z

yeah, that's not going to be a happy time on the small devices, if it must be tested I guess you might want to first test for the amount of system memory available, but that's also going to be tricky for cross-platform support, the alternative is to just skip it if arch == arm, but there's also mips (tessel) and Windows 10 on rpi and Atom and ...

chrisdickinson · 2015-06-04T08:11:04Z

My vote would be to make an internal module (lib/internal/smalloc, for the sake of argument) that exposes getMaxLength() & override it in the test to return a much smaller value.

trevnorris · 2015-06-04T17:50:11Z

that may be possible by process.binding('smalloc').kMaxLength = <some small value> before requiring zlib. problem is that may have other unknown consequences.

targos · 2015-06-05T13:30:17Z

@trevnorris your proposal is working, thanks! I added a test.

trevnorris · 2015-06-05T16:53:55Z

One more addition to the test. Can you set it back to the original value of 0x3FFFFFFF after requiring zlib? After that let's add this to CI and see how it holds up.

targos · 2015-06-06T06:41:22Z

Done. Can somebody start a CI?

brendanashworth · 2015-06-08T19:14:41Z

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/780/

targos · 2015-06-10T15:10:51Z

All green! LGTY ?

trevnorris · 2015-06-10T15:20:36Z

LGTM

@indutny have any objections to this change?

indutny · 2015-06-10T15:22:20Z

LGTM! It does look like a security concern to me. I think we may want to issue a CVE. What are your thoughts on this?

trevnorris · 2015-06-10T15:38:29Z

I believe the only issue is that a client could forcefully crash a server. That's not a trivial issue by any means. I'm just not sure if it classifies as security vulnerability or not. If you think it is then cool. Let's do a CVE.

indutny · 2015-06-10T15:41:51Z

Yeah, but it is a bit trivial do so (see the test). Anyway, how should we fill a CVE cc @nodejs/tsc

targos · 2015-06-10T15:54:29Z

I'm thinking that we can spare some time and memory by checking the current size in the loop here and here but that may be for another PR.

trevnorris · 2015-06-10T15:59:09Z

Yup. Let's save that for the next one. This one has already been signed off. :-)

targos · 2015-06-15T17:41:48Z

ping @trevnorris : can this be landed ?

trevnorris · 2015-06-15T20:55:17Z

@targos I'm not sure if this should land before the CVE is created. If @indutny doesn't respond by the TSC meeting on Wednesday then I'll bring it up in the meeting.

i.e. Your patch is great. The hold up is missing process around landing a patch that should have a CVE.

dcousens · 2015-06-15T21:06:53Z

Great work all, definitely should have a CVE.

indutny · 2015-06-15T21:14:09Z

I think we should land it anyway, there is no point in releasing stuff without this ;) I don't have any further comments since I wasn't ever involved in the process of filing CVE. cc @piscisaureus

If the accumulation of data for the final Buffer is greater than kMaxLength it will throw an un-catchable RangeError. Instead now pass the generated error to the callback. PR-URL: #1811 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

trevnorris · 2015-06-15T21:57:43Z

Landed in 3806d87. Thanks for the patch.

misterdjules · 2015-06-17T16:54:24Z

@indutny @trevnorris @nodejs/tsc If I understand correctly, this issue also exists in node v0.10.x and node v0.12.x, so before issuing a CVE, I would suggest getting ready to backport and release the changes in this PR into those branches. Is that something you could help with?

misterdjules · 2015-06-17T16:57:33Z

Created nodejs/node-v0.x-archive#25536 to track that.

trevnorris · 2015-06-17T17:03:22Z

@misterdjules The fix itself is straight forward, but currently the way to test uses properties on the smalloc module. Not sure how this could be tested on v0.10 without needing to use 1GB of memory.

PR-URL: #1996 Notable changes * module: The number of syscalls made during a require() have been significantly reduced again (see #1801 from v2.2.0 for previous work), which should lead to a performance improvement (Pierre Inglebert) #1920. * npm: - Upgrade to v2.11.2 (Rebecca Turner) #1956. - Upgrade to v2.11.3 (Forrest L Norvell) #2018. * zlib: A bug was discovered where the process would abort if the final part of a zlib decompression results in a buffer that would exceed the maximum length of 0x3fffffff bytes (~1GiB). This was likely to only occur during buffered decompression (rather than streaming). This is now fixed and will instead result in a thrown RangeError (Michaël Zasso) #1811.

mscdex added the zlib Issues and PRs related to the zlib subsystem. label May 27, 2015

cjihrig reviewed May 27, 2015
View reviewed changes

zlib: prevent uncaught exception in zlibBuffer

4692e75

targos force-pushed the fix-zlib branch from 3b055f8 to 4692e75 Compare May 27, 2015 18:33

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

check size in zlib

ce770de

add regression test

6970045

targos force-pushed the fix-zlib branch from 661c740 to 6970045 Compare June 6, 2015 06:39

trevnorris added the tsc-agenda label Jun 15, 2015

trevnorris closed this Jun 15, 2015

targos deleted the fix-zlib branch June 15, 2015 23:13

brendanashworth mentioned this pull request Jun 16, 2015

util: fix isBuffer #1988

Closed

rvagg mentioned this pull request Jun 16, 2015

Release proposal: v2.3.1 #1996

Closed

trevnorris mentioned this pull request Jun 17, 2015

Node Foundation TSC Meeting 2015-06-17 nodejs/node-convergence-archive#56

Closed

misterdjules mentioned this pull request Jun 17, 2015

Backport potential zlib security fix from io.js nodejs/node-v0.x-archive#25536

Closed

rvagg removed the tsc-agenda label Jun 24, 2015

cclauss mentioned this pull request Sep 30, 2019

build: ongoing list of actions for Python 3 compatibility #25789

Closed

63 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: prevent uncaught exception in zlibBuffer #1811

zlib: prevent uncaught exception in zlibBuffer #1811

targos commented May 27, 2015

cjihrig commented May 27, 2015

cjihrig May 27, 2015

targos May 27, 2015

targos commented May 27, 2015

cjihrig commented May 28, 2015

trevnorris commented Jun 4, 2015

targos commented Jun 4, 2015

trevnorris commented Jun 4, 2015

rvagg commented Jun 4, 2015

chrisdickinson commented Jun 4, 2015

trevnorris commented Jun 4, 2015

targos commented Jun 5, 2015

trevnorris commented Jun 5, 2015

targos commented Jun 6, 2015

brendanashworth commented Jun 8, 2015

targos commented Jun 10, 2015

trevnorris commented Jun 10, 2015

indutny commented Jun 10, 2015

trevnorris commented Jun 10, 2015

indutny commented Jun 10, 2015

targos commented Jun 10, 2015

trevnorris commented Jun 10, 2015

targos commented Jun 15, 2015

trevnorris commented Jun 15, 2015

dcousens commented Jun 15, 2015

indutny commented Jun 15, 2015

trevnorris commented Jun 15, 2015

misterdjules commented Jun 17, 2015

misterdjules commented Jun 17, 2015

trevnorris commented Jun 17, 2015

zlib: prevent uncaught exception in zlibBuffer #1811

zlib: prevent uncaught exception in zlibBuffer #1811

Conversation

targos commented May 27, 2015

cjihrig commented May 27, 2015

cjihrig May 27, 2015

Choose a reason for hiding this comment

targos May 27, 2015

Choose a reason for hiding this comment

targos commented May 27, 2015

cjihrig commented May 28, 2015

trevnorris commented Jun 4, 2015

targos commented Jun 4, 2015

trevnorris commented Jun 4, 2015

rvagg commented Jun 4, 2015

chrisdickinson commented Jun 4, 2015

trevnorris commented Jun 4, 2015

targos commented Jun 5, 2015

trevnorris commented Jun 5, 2015

targos commented Jun 6, 2015

brendanashworth commented Jun 8, 2015

targos commented Jun 10, 2015

trevnorris commented Jun 10, 2015

indutny commented Jun 10, 2015

trevnorris commented Jun 10, 2015

indutny commented Jun 10, 2015

targos commented Jun 10, 2015

trevnorris commented Jun 10, 2015

targos commented Jun 15, 2015

trevnorris commented Jun 15, 2015

dcousens commented Jun 15, 2015

indutny commented Jun 15, 2015

trevnorris commented Jun 15, 2015

misterdjules commented Jun 17, 2015

misterdjules commented Jun 17, 2015

trevnorris commented Jun 17, 2015