jailed icon indicating copy to clipboard operation
jailed copied to clipboard

Fix constructor exploit on NodeJS

Open gpascualg opened this issue 8 years ago • 6 comments

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

gpascualg avatar Jul 03 '16 00:07 gpascualg

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

asvd avatar Jul 03 '16 00:07 asvd

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

gpascualg avatar Jul 03 '16 00:07 gpascualg

you are right, missed that

asvd avatar Jul 03 '16 00:07 asvd

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

lu4 avatar Oct 30 '16 14:10 lu4

@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

asvd avatar Oct 30 '16 22:10 asvd

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?

lehni avatar Jul 03 '17 17:07 lehni