cbor-sync icon indicating copy to clipboard operation
cbor-sync copied to clipboard

Fixes a DoS exploit in the decoder

Open headlessme opened this issue 4 years ago • 6 comments

Adds a check before trying to allocate an unbounded length buffer, which could be used to drain all the free memory from a server.

headlessme avatar Nov 10 '20 21:11 headlessme

The issue also lies in the decoding of map/array lengths, causes crashes like:

<--- Last few GCs --->
[769:0x4d0ea20]   236975 ms: Scavenge 890.4 (907.8) -> 874.4 (907.8) MB, 1.1 / 0.0 ms  (average mu = 0.723, current mu = 0.687) allocation failure 
[769:0x4d0ea20]   236984 ms: Scavenge 890.4 (907.8) -> 874.4 (907.8) MB, 1.2 / 0.0 ms  (average mu = 0.723, current mu = 0.687) allocation failure 
[769:0x4d0ea20]   236992 ms: Scavenge 890.4 (907.8) -> 874.4 (907.8) MB, 0.8 / 0.0 ms  (average mu = 0.723, current mu = 0.687) allocation failure 


<--- JS stacktrace --->

FATAL ERROR: invalid array length Allocation failed - JavaScript heap out of memory
 1: 0xa03530 node::Abort() [/usr/local/bin/node]
 2: 0x94e471 node::FatalError(char const*, char const*) [/usr/local/bin/node]
 3: 0xb7773e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
 4: 0xb77ab7 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
 5: 0xd32345  [/usr/local/bin/node]
 6: 0xd0ae85  [/usr/local/bin/node]
 7: 0xe9469e  [/usr/local/bin/node]
 8: 0xe947ed  [/usr/local/bin/node]
 9: 0x1030b03 v8::internal::Runtime_GrowArrayElements(int, unsigned long*, v8::internal::Isolate*) [/usr/local/bin/node]
10: 0x13ff179  [/usr/local/bin/node]

Can repro by:

const CBOR = require('cbor-sync');
CBOR.decode(Buffer.from('BAFFFFFFFF', 'hex'));

headlessme avatar Nov 11 '20 19:11 headlessme

I'm not sure I'm meant to still have access to an mbed repo, but looks good to me 😛

geraintluff avatar Nov 12 '20 18:11 geraintluff

Happy to merge once it's no longer a WiP

thegecko avatar Nov 12 '20 18:11 thegecko

A fix turned out to be more complex than the one in this PR, the issue is here:

https://github.com/ARMmbed/cbor-sync/blob/35fdf44ce469ec34b7985f8463c806876fdb8d31/main.js#L223-L232

The array length is trusted and the loop continues to fill the result array even when the input buffer has been used up, for stupidly long lengths this uses up all the memory. I'd guess the right fix would be to check if this.pos in the BufferReader overflows the input buffer length whenever it's updated, and throw an exception at that point.

In light of that, we switched out to a different decoder, but hopefully the debugging so far will be useful for someone motivated to fix it properly!

headlessme avatar Nov 12 '20 19:11 headlessme

Found some time to do a better fix – @geraintluff might be worth a sanity check!

headlessme avatar Nov 13 '20 18:11 headlessme

Looks like this fixes the DoS exploit. Could @geraintluff review it and release if you still have access?

anupvarghese avatar Dec 16 '20 23:12 anupvarghese