search

asvd / jailed

execute untrusted code with custom permissions
MIT License
1.01k stars 69 forks source link

Fix constructor exploit on NodeJS #37

Open gpascualg opened 8 years ago

gpascualg commented 8 years ago

Attemps to fix https://github.com/asvd/jailed/issues/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

asvd commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

you are right, missed that

lu4 commented 8 years ago

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

asvd commented 8 years ago

@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 commented 7 years ago

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?