Zip bomb prevention?

[timotm]

Hi,

How is one supposed to abort processing of zip entry / file while processing entries?

Some background: I want to prevent a zip bomb from hogging CPU/memory resources, and would like to check for actual, cumulative uncompressed size while uncompressing an entry. For that, I implemented my own Writable stream which raises an error (through callback) when it gets too much data. I then catch this error and currently I call .close() for the readStream I got in yauzl's entry callback.

However, this seems to trigger a bug in node's zlib implementation (I tried both 0.10.28 and 0.12.2) and aborts the execution:

Assertion failed: (ctx->mode_ != NONE && "already finalized"), function Write, file ../src/node_zlib.cc, line 147.
Abort trap: 6

While I theoretically could patch my way around this, I naturally wouldn't want to fork both zlib.js and your library. So can I abort the processing of an entry / entire zip file by some other way cleanly, without any excessive CPU or memory usage?

Full sample code available at https://github.com/timotm/node-zip-bomb

[thejoshwolfe]

Entry objects have an uncompressedSize field. what about reading that instead of counting the streamed bytes?

[timotm]

Unfortunately that field cannot be trusted as it can be forged by an
attacker. The only sure way is to count the bytes while inflating.

On Mon, Apr 27, 2015 at 8:42 PM Josh Wolfe notifications@github.com wrote:

Entry objects have an uncompressedSize field. what about reading that
instead of counting the streamed bytes?

—
Reply to this email directly or view it on GitHub
#13 (comment).

[thejoshwolfe]

Hm. I think you're right.

One thing you can do without patching any modules is to unpipe the stream, and repipe it to a null sink. This allows all the bytes to flow from yauzl and zlib without either of them suspecting anything is wrong. This will surely avoid any annoying stream errors in yauzl and zlib, but it's not ideal for CPU and memory usage. (I've actually never tried to use unpipe() before, but I assume this is how it works.)

Another idea is for yauzl to count the bytes flowing out of zlib and make sure it matches the uncompressedSize reported in the metadata. If we get too many bytes (or too few bytes), end the streams and emit an error. With this approach, you could check for probable zip bombs by simply checking the uncompressedSize field and listening for errors. Would this be acceptable for your requirements?

[timotm]

Thanks for the ideas! I did experiment with re-piping, but as you said, it will consume a lot of CPU and memory while shoveling away all the unneeded bits and therefore causing a sort of denial-of-service.

The second suggestion sounds great! (Although I'm guessing you'll then run into the zlib bug mentioned in the sample code..)

[thejoshwolfe]

I'm having trouble reproducing this zlib crash. I'm using node v0.10.37:

1064960 'over the limit'
ws error [Error: zip bomb, zip bomb, you're my zip bomb]

/home/josh/dev/yauzl/sample.js:30
s.on('error', function (e) { console.log('ws error', e); readStream.close() })
                                                                    ^
TypeError: Object #<AssertByteCountStream> has no method 'close'
    at Writable.<anonymous> (/home/josh/dev/yauzl/sample.js:30:76)
    at Writable.emit (events.js:117:20)
    at onwriteError (_stream_writable.js:239:10)
    at onwrite (_stream_writable.js:257:5)
    at WritableState.onwrite (_stream_writable.js:97:5)
    at Writable.ws._write (/home/josh/dev/yauzl/sample.js:24:11)
    at doWrite (_stream_writable.js:226:10)
    at writeOrBuffer (_stream_writable.js:216:5)
    at Writable.write (_stream_writable.js:183:11)
    at write (_stream_readable.js:602:24)

I want to make sure the zlib crash isn't a problem with the solution I'm writing for this issue. I actually have a test case already that provokes a zlib error by trying to decompress corrupted data:

$ node test/dump.js test/failure/invalid\ code\ lengths\ set.zip 
...

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: invalid code lengths set
    at Zlib._binding.onerror (zlib.js:296:17)

Does npm test pass on your system? npm test makes sure that the above error is catchable by clients, and doesn't cause a zlib crash like you're describing.

[timotm]

With the release version of yauzl, variable readStream the library gives in openReadStream()'s callback is what zlib.createInflateRaw() returns and it has the close() method I'm calling in the sample code to signal end of processing. So just replace that call with whatever is then the correct way to clean up that counting stream?

(To me it seems that calling the close() on causes node to call the underlying zlib's write without checking that it already has been uninitialized https://github.com/joyent/node/blob/v0.12.2/lib/zlib.js#L595 )

[timotm]

If it is of any help, here's an updated reproduction sample with invalid uncompressed sizes: https://github.com/timotm/node-zip-bomb/tree/invalid_size

[thejoshwolfe]

oh. uh. right. my bad. I was using some local modifications. I can reproduce the zlib crash now.

I'm hoping there's a "correct" way to interrupt the zlib stream and suppress any errors due to the force close. I'll keep investigating.

[thejoshwolfe]

So it looks like all you need to do is emit an error on one of the streams in the pipe, and all the streams shut down cleanly.

[timotm]

Yes, that was the first thing I tried, too, but that does result in excessive CPU and memory usage in case of a large zip file. The only way I figured out to not cause resource leakage was to explicitly close the zip stream (which then hits the node bug).

Here are two simple CPU and memory visualizations from an express web server that accepts a zip file upload and then extracts it. In both plots, I upload a zip file twice. The zip contains a file that's 4 GB uncompressed, but has been marked in the central directory as 8240 bytes, thus triggering the new check in yauzl.

With out-of-the-box yauzl version 2.3.0 resource usage looks like this

And if I alter yauzl code to have (and patch zlib.js to overcome the node crash)

checkerStream.on("error", function(err) {
    deflateFilter.close()
})

it looks like this

In both of the images, the leftmost Y-axis is node process' virtual memory size in MB and rightmost Y-axis is CPU utilization in %. X-axis is just time in seconds.

[thejoshwolfe]

@timotm, despite my silence and inaction, I didn't forget about this issue. I may actually have a solution in mind, but I think it's going to be complicated. See #26.