Node crashes when a big sparse array is given to console.log (might cause DoS)

[mik01aj]

var a=[] 
a[1000000000]=1 
console.log(a) 

This ends up with a FATAL ERROR: process out of memory. Wouldn't expect this...

Note that many applications use console.log for logging their stuff, and this can lead to a DoS attack: for example, when an user-specified JSON {"1000000000":"a"} is merged with some pre-existing array and then printed on console. Having an upper bound on printed Array items in console.log seems like an easy fix for this.

I originally reported this to security@nodejs.org, but I got this response:

I don't think we consider this a security issue (it's known and documented) but it's arguably a quality-of-implementation issue. If you'd like to pursue this further, can you file an issue (...)?

So I'm opening an issue 😃

Btw, I can't see this documented anywhere in the console docs, but maybe I'm missing something?

[ChALkeR]

@mik01aj Am I correct that when speaking about DoS, you have console.log(util._extend([],{"1000000000":"a"})) or console.log(Object.assign([],{"1000000000":"a"})) in mind, and this in fact requires someone to use Object.assign or util._extend on an Array?

If so, Object.assign([],{"length":1000000000}) (or util._extend([],{"length":1000000000})) will chew all your memory even without console.log, so you can't blame Node.js here. Edit: I was mistaken here, see below.

[mik01aj]

Yes. I think it can be pretty common in PATCH endpoints, or in deserialization libraries, or Redux reducers. I didn't check for real-world examples, but I believe that console.log should never ever crash the process, regardless of what was passed in.

Note that Object.assign([],{"length":1000000000}) or similar examples create just objects, and this doesn't make Node crash - it's console.log that does (note that Node uses console.log internally in REPL too).

[vkurchatkin]

If so, Object.assign([],{"length":1000000000}) (or util._extend([],{"length":1000000000})) will chew all your memory

How so?

[ChALkeR]

@vkurchatkin My bad, it seems that I was incorrect, sorry.

I was testing this in repl, and it consumed the memory not on that operation, but when trying to print the result.

I will edit my comments above, thanks!

[ChALkeR]

As a possible fix, we could limit the maximum length of an array being printed in util.inspect and replace it with an [Object] (or [Array]), if it's too long — pretty much the same way like it is done when nesting level is too deep.

[mik01aj]

Yes, that's what I meant. 👍

As for DoS risk, I don't have time to think about a good full blown code example, sorry. Maybe later... Or maybe this could convince you that it's certainly possible to make such an array accidentally: lodash/lodash#1685.

[vkurchatkin]

As a possible fix, we could limit the maximum length of an array being printed in util.inspect

+1.

[ChALkeR]

@mscdex This is related to util.format/util.inspect, not to console.

[bnoordhuis]

console.log() and friends call util.format() and util.inspect(), though.

[ChALkeR]

@bnoordhuis This is what I meant, and is exactly why this issue belongs to util, not to console =).

[rvagg]

Seems like the same issue as I was pinged about on Twitter recently

It'd be nice to not have unexpected and non-obvious behaviour like this if we can help it. Fair enough if you're actually using up memory but when you know you're not but it still dies, that's not so great.

/cc @trevnorris

[trevnorris]

I'm not sure what else we could do here. v8 doesn't give the capability to cancel the operation of retrieving property names if the heap is about to run out of memory.

[zeusdeux]

Can we not switch to a streaming model of writing to stdout instead of building up a string in memory and then dumping it to stdout? We could introduce an internal util.format like method that is basically a Any -> String stream that the console family could then use.

I ask because

var a=[] 
a[1000000000]=1 
console.log(a) 

works in browsers.

[trevnorris]

@zeusdeux I can't think of a way to handle circular dependencies that way. the model needs to be built to make sure we won't end up recursively printing objects.

[dcposch]

Retrieving the property names is fast:

> var a=[] 
> a[1000000000]=1
> Object.keys(a)
[ '1000000000' ]

Printing a is slow because formatArray in lib/util.js is producing a ginormous array of empty strings, which reduceToSingleString then tries to concat into a single 4GB string.

To illustrate

> var a=[]
> a[10]=1 // if you replace 10 with 1000000 here, it will crash
> a
[ , , , , , , , , , , 1 ]

If you guys want, I can add a check for spase arrays (eg keys.length * 10 < array.length) and print those differently:

> var a=[]
> a[3]=1
> a
[ , , , 1 ] // existing behavior
> a['swag'] = 1
> a
[ , , , 1, swag: 1 ] // existing behavior
> a[1000000] = 1
> a
[ 3: 1, 1000000: 1, swag: 1] // new behavior, doesn't crash

[zeusdeux]

@trevnorris If you mean how the streaming model would work for objects with circular dependencies, then it would work as is since the already seen value will be in ctx.seen like it is in the current implementation. The stream would end up pushing [Circular] formatted string to it's consumer.

[dcposch]

@zeusdeux the reason var a=[]; a[1000000000]=1; console.log(a) works in browser not because it streams out jiggabytes of text, but because it prints sparse arrays compactly.

util.inspect isn't for machine-to-machine communication -- it's not JSON.stringify. instead it's a debugging aid, meant for human viewing. so i prefer the INSPECT_MAX_BYTES approach (ie, always return a reasonable-size string) to the streaming approach (ie, efficiently output unlimited amounts of text)

[tunniclm]

How about something that operates like this (hacked together this morning):

Options to restrict the output to a maximum number of elements and to elide series of unset values, with the settings available at module and per-invocation scope. Setting either to 0 disables that limit.

> u=require('util'); u.MAX_ARRAY_OUTPUT_ELEMENTS=15; 
   u.MAX_UNSET_ELEMENTS_BEFORE_ELIDE=9;
  a=[]; a[9]=1; for (var i=100; i<10000; ++i) { a[i]=1}
1
> a
[ , , , , , , , , , 1, <90 unset>, 1, 1, 1, <9897 more> ]
> u.inspect(a).split(',').length
15
> console.log(u.inspect(a, {arrayMaxUnsetElementsBeforeElide: 1, 
                            arrayMaxOutputElements: 20}))
[ <9 unset>,
  1,
  <90 unset>,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  1,
  <9884 more> ]
undefined
> console.log(a)
[ , , , , , , , , , 1, <90 unset>, 1, 1, 1, <9897 more> ]
undefined

[mik01aj]

@tunniclm I like your suggestion, but making it the default would need a new semver-major version (as stated by @rvagg in #5070 (comment)). I would say, the main reason is that you change the output format even for small arrays (I mean small for machine, so 10000 is small, it all prints in a split second).

I would suggest to change the output format only for arrays big enough to cause the crash, and make this the default, and push this fix to any relevant versions. I consider making the output format nicer and more human-readable a separate issue (:+1: for it, but not in a minor version)

[tunniclm]

@mik01aj I'm not sure why this would necessarily be semver-major, since it could even be completely disabled by default, if necessary. (I was thinking setting the module options to be disabled by default, and to change the REPL to use the per-invocation values arrayMaxUnsetElementsBeforeElide: 0 and arrayMaxOutputElements: A_SUFFICIENTLY_LARGE_NUMBER by default).

I can see that it doesn't help with selecting the right value for a maximum number of array output elements though -- would a flat value be sufficient? would we want a number calculated at startup depending on the machine? would we want a value calculated at run time based on the amount of free memory?