Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mik01aj opened this issue Jan 27, 2016 · 27 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module.

Comments

@mik01aj
Copy link

mik01aj commented Jan 27, 2016

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 [email protected], 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?

@vkurchatkin vkurchatkin added the util Issues and PRs related to the built-in util module. label Jan 27, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jan 27, 2016

@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
Copy link
Author

mik01aj commented Jan 27, 2016

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
Copy link
Contributor

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

How so?

@ChALkeR
Copy link
Member

ChALkeR commented Jan 27, 2016

@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
Copy link
Member

ChALkeR commented Jan 27, 2016

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
Copy link
Author

mik01aj commented Jan 27, 2016

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
Copy link
Contributor

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

+1.

@mscdex mscdex added console Issues and PRs related to the console subsystem. and removed util Issues and PRs related to the built-in util module. labels Jan 27, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jan 27, 2016

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

@bnoordhuis
Copy link
Member

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

@ChALkeR
Copy link
Member

ChALkeR commented Jan 27, 2016

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

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jan 27, 2016
@rvagg
Copy link
Member

rvagg commented Jan 28, 2016

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

screen shot 2016-01-28 at 10 22 40 pm

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
Copy link
Contributor

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
Copy link
Contributor

zeusdeux commented Feb 3, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

dcposch commented Feb 4, 2016

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

dcposch added a commit to dcposch/node that referenced this issue Feb 4, 2016
This prevents `console.log(arr)` from crashing node when given a sparse
array with large length. Fixes nodejs#4905
@zeusdeux
Copy link
Contributor

zeusdeux commented Feb 5, 2016

@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
Copy link
Contributor

dcposch commented Feb 5, 2016

@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
Copy link
Contributor

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
Copy link
Author

mik01aj commented Mar 22, 2016

@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
Copy link
Contributor

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

This was referenced Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. console Issues and PRs related to the console subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests