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

Fix constructor exploit on NodeJS #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gpascualg
Copy link

@gpascualg gpascualg commented Jul 3, 2016

Attemps to fix #33 by wrapping all exposed objects into a Proxy object, which restricts get/set operations.

It is tested but not extensibly. Solves the main issue with constructor, and maybe some others with prototype, but might introduce some others (I'm new to this, so who knows).

On the downside, requires https://github.com/tvcutsem/harmony-reflect to port Proxy to NodeJS.

EDIT (Already fixed): Accidentaly I commited the exception code too, let me fix it.


The recursivity in get/set is to also fix attemps like:

application.whenConnected.toString.constructor

That would otherwise also work

@gpascualg gpascualg force-pushed the secure branch 2 times, most recently from 170eee1 to e99fe4e Compare July 3, 2016 00:10
@asvd
Copy link
Owner

asvd commented Jul 3, 2016

@gpascualg as far as I understand, you only secure the exposed methods, but not the application.whenConnected method hacked in #33. Did you test it with that case?

@gpascualg
Copy link
Author

@asvd application is secured from the start, it is an exposed object. See that get also calls secureObject on the requested value, so when whenConnected is retrieved it is also a Proxied object, thus the call to constructor returns the default Object.constructor.

It is tested and no longer can be exploited this way.

@asvd
Copy link
Owner

asvd commented Jul 3, 2016

you are right, missed that

@lu4
Copy link

lu4 commented Oct 30, 2016

The code looks good to me, why isn't it being merged?

@asvd
Copy link
Owner

asvd commented Oct 30, 2016

@lu4 for the given issue (#33) I currently see two directions towards the solution:

  1. Build a system-based sandbox using chrooted shell;
  2. Use language-provided means suggested in this pull request.

I like the first approach more, and am going to investigate into that direction. As for this pull request, I did not merge it because I would prefer to avoid external dependencies (harmony-reflect), especially for such a basic case. More proper way would be to figure out how Proxy is emulated by harmony-reflect (and if it is secure enough for the purpose by the way), and reuse that trick right in place.

If someone reimplements this pull request in the way I described, before I implement the first point, I think I would merge it and drop my further investigations

@lehni
Copy link

lehni commented Jul 3, 2017

The Proxy class is available in Node v6 and above, perhaps it's time to reconsider this PR and remove the external dependency, until the chroot approach has been tested?

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

Successfully merging this pull request may close these issues.

Node.js sandbox is broken
4 participants