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

DOS: js expression make nodejs crash #9295

Closed
phra opened this issue Oct 26, 2016 · 6 comments
Closed

DOS: js expression make nodejs crash #9295

phra opened this issue Oct 26, 2016 · 6 comments

Comments

@phra
Copy link

phra commented Oct 26, 2016

  • Version: 6.9.1
  • Platform: Linux kali 4.7.0-kali1-686-pae SMP Debian 4.7.6-1kali1 (2016-10-17) i686 GNU/Linux
  • Subsystem: js built in prototypes

hi,

i was doing some javascript sorcery and i found out that the following js expression make the nodejs kill itself because it's tampering the prototype of Array.

Array.prototype.push = Array.prototype.push.bind(Array.prototype);

e.g.

> Array.prototype.push = Array.prototype.push.bind(Array.prototype)                                                  
[Function: bound push]                                       
> internal/process/next_tick.js:67            
      callback();                
      ^                                                

TypeError: callback is not a function  
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)

nodejs is crashing at this line -> https://github.com/nodejs/node/blob/master/lib/internal/process/next_tick.js#L67
i think that making not writable and not configurable the prototypes of built in types can prevent such very evil things.

what do you think about guys?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 26, 2016

Node shouldn't mess with the built ins if possible. By you doing so, you shouldn't be surprised that things break. We might be able to work around it by storing a reference to the original function before user code can run, but I'm not sure if it's worth it.

@phra
Copy link
Author

phra commented Oct 26, 2016

imho it's a security issue called denial of service.
i think that the best solution is a default setting that freeze the builtin prototypes, and if someone has to deal with them can just pass a flag to the node executable to disable the default setting and restore the original behaviour.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 26, 2016

Freezing the builtins would probably break a ton of code. Also, how would an attacker execute such an attack? It seems like they would have to already be in a position to execute arbitrary code, in which case they could do much worse.

@mscdex
Copy link
Contributor

mscdex commented Oct 26, 2016

I agree with @cjihrig. If an unauthorized person is able to modify your server code, then being worried about such a person adding that kind of breaking code is probably the least of your worries.

@phra
Copy link
Author

phra commented Oct 27, 2016

that's for sure, obviously.

@phra phra closed this as completed Oct 27, 2016
@phra
Copy link
Author

phra commented Oct 27, 2016

just saw that the node exits without a SEGV so RCE is out of scope with this expression.
btw if there are changes to some prototype that can make v8/node crash with a SEGV, an open road to RCE with the user running the node instance is possible, so outside the v8 vm context. (beyond ASRL, etc)

for more infos: https://blog.scrt.ch/2013/03/24/mongodb-0-day-ssji-to-rce/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants