zlib: prevent uncaught exception in zlibBuffer by targos · Pull Request #1811 · nodejs/node · GitHub

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

Can you add a test?

Could you do something more like:

err = null;

try {
  buf = Buffer.concat(buffers, nread);
} catch (e) {
  err = e;
}

buffers = [];
engine.close();
callback(err, buf);

done

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 ?

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?

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.

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

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

@rvagg Thoughts?

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 ...

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.

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

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

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.

Done. Can somebody start a CI?

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

All green! LGTY ?

LGTM

@indutny have any objections to this change?

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?

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.

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

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.

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

ping @trevnorris : can this be landed ?

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

Great work all, definitely should have a CVE.

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 <fedor@indutny.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Landed in 3806d87. Thanks for the patch.

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

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

@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

zlib: prevent uncaught exception in zlibBuffer by targos · Pull Request #1811 · nodejs/node · GitHub

Uh oh!

Uh oh!

zlib: prevent uncaught exception in zlibBuffer - #1811

zlib: prevent uncaught exception in zlibBuffer#1811targos wants to merge 3 commits intonodejs:masternodejs/node:masterfrom targos:fix-zlibtargos/node:fix-zlibCopy head branch name to clipboard

Conversation

targos commented May 27, 2015

Uh oh!

cjihrig commented May 27, 2015

Uh oh!

cjihrig May 27, 2015

Choose a reason for hiding this comment

Uh oh!

targos May 27, 2015

Choose a reason for hiding this comment

Uh oh!

targos commented May 27, 2015

Uh oh!

cjihrig commented May 28, 2015

Uh oh!

trevnorris commented Jun 4, 2015

Uh oh!

targos commented Jun 4, 2015

Uh oh!

trevnorris commented Jun 4, 2015

Uh oh!

rvagg commented Jun 4, 2015

Uh oh!

chrisdickinson commented Jun 4, 2015

Uh oh!

trevnorris commented Jun 4, 2015

Uh oh!

targos commented Jun 5, 2015

Uh oh!

trevnorris commented Jun 5, 2015

Uh oh!

targos commented Jun 6, 2015

Uh oh!

brendanashworth commented Jun 8, 2015

Uh oh!

targos commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

indutny commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

indutny commented Jun 10, 2015

Uh oh!

targos commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

targos commented Jun 15, 2015

Uh oh!

trevnorris commented Jun 15, 2015

Uh oh!

dcousens commented Jun 15, 2015

Uh oh!

indutny commented Jun 15, 2015

Uh oh!

trevnorris commented Jun 15, 2015

Uh oh!

misterdjules commented Jun 17, 2015

Uh oh!

misterdjules commented Jun 17, 2015

Uh oh!

trevnorris commented Jun 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

zlib: prevent uncaught exception in zlibBuffer#1811
targos wants to merge 3 commits into
nodejs:masterfrom
targos:fix-zlib