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

deserialize() can be abused to achieve arbitrary code injection with an IIFE #1

Closed
ajinabraham opened this Issue Feb 9, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@ajinabraham

ajinabraham commented Feb 9, 2017

var payload = "{e: (function(){ eval('console.log(`exploited`)') })() }"
var serialize = require('serialize-to-js');
serialize.deserialize(payload);

I don't know if this is a functionality as you are using new Function() internally, but the module should not execute code on deserialization.

@ajinabraham ajinabraham changed the title from Unserialize() can be abused to achieve arbitrary code injection with an IIFE to deserialize() can be abused to achieve arbitrary code injection with an IIFE Feb 9, 2017

@commenthol

This comment has been minimized.

Show comment
Hide comment
@commenthol

commenthol Feb 9, 2017

Owner

Thanks for rising. A patch is on its way. Would be nice if you could do some review on the changes.
I still would not consider the fix as being safe. So I really appreciate your opinion.

Owner

commenthol commented Feb 9, 2017

Thanks for rising. A patch is on its way. Would be nice if you could do some review on the changes.
I still would not consider the fix as being safe. So I really appreciate your opinion.

@ajinabraham

This comment has been minimized.

Show comment
Hide comment
@ajinabraham

ajinabraham Feb 10, 2017

Happy to help. I will take a look on the fix and keep you updated.

ajinabraham commented Feb 10, 2017

Happy to help. I will take a look on the fix and keep you updated.

@matt-

This comment has been minimized.

Show comment
Hide comment
@matt-

matt- Feb 15, 2017

This doesn't really have anything to do with an IIFE. Simply the fact that the Function constructor works like an eval.

str = 'console.log(`exploited`)';
var res = deserialize(str);

The line in deserialize will invoke it for you..

  return (new Function('"use strict"; return ' + str))()
}

matt- commented Feb 15, 2017

This doesn't really have anything to do with an IIFE. Simply the fact that the Function constructor works like an eval.

str = 'console.log(`exploited`)';
var res = deserialize(str);

The line in deserialize will invoke it for you..

  return (new Function('"use strict"; return ' + str))()
}
@matt-

This comment has been minimized.

Show comment
Hide comment
@matt-

matt- Feb 16, 2017

the tokenizer will allow for things like:
object properties as strings like:

'{str: global["eval"]("console.log(/exploited/)")}';

constructors of function to get Function class refs

'{str: unescape.constructor()("console.log(/exploited/)")()()}'

I think esprima is the right move, but I would consider using if for parsing then building rules on top of that instead of the tokenizer.

matt- commented Feb 16, 2017

the tokenizer will allow for things like:
object properties as strings like:

'{str: global["eval"]("console.log(/exploited/)")}';

constructors of function to get Function class refs

'{str: unescape.constructor()("console.log(/exploited/)")()()}'

I think esprima is the right move, but I would consider using if for parsing then building rules on top of that instead of the tokenizer.

@commenthol

This comment has been minimized.

Show comment
Hide comment
@commenthol

commenthol Feb 18, 2017

Owner

Hi @matt- , thanks for you comments so far. You are right with the fact that this has nothing to do with IIFE, its just that new Function may inject into global context. I have thought about it and lacking the time an approach with esprima, as you suggest, is not affordable for me. I have chosen a different one, using the same bad new Function but with setting global vars explicitly before evaluation.
Maybe you have some time and check the project safer-eval.
The current master branch includes now safer-eval but isn't yet on npm.
Would be nice to get you opinions on this.

Owner

commenthol commented Feb 18, 2017

Hi @matt- , thanks for you comments so far. You are right with the fact that this has nothing to do with IIFE, its just that new Function may inject into global context. I have thought about it and lacking the time an approach with esprima, as you suggest, is not affordable for me. I have chosen a different one, using the same bad new Function but with setting global vars explicitly before evaluation.
Maybe you have some time and check the project safer-eval.
The current master branch includes now safer-eval but isn't yet on npm.
Would be nice to get you opinions on this.

@DavidBruant

This comment has been minimized.

Show comment
Hide comment
@DavidBruant

DavidBruant Feb 19, 2017

@commenthol Your safer-eval projects has many similarities with Caja.
Please take a look at the confine function

Among other things, some people involved in Caja have been involved in evolving ECMAScript; their involvement led to the creation of strict mode, Object.freeze & friends, WeakMap and Proxies.

edit : better link to the caja project : https://developers.google.com/caja/

DavidBruant commented Feb 19, 2017

@commenthol Your safer-eval projects has many similarities with Caja.
Please take a look at the confine function

Among other things, some people involved in Caja have been involved in evolving ECMAScript; their involvement led to the creation of strict mode, Object.freeze & friends, WeakMap and Proxies.

edit : better link to the caja project : https://developers.google.com/caja/

@commenthol

This comment has been minimized.

Show comment
Hide comment
@commenthol

commenthol Feb 19, 2017

Owner

@DavidBruant Thanks for the notice. Caja is indeed an interesting project, I did not know so far. Unfortunately there is no chance to run the compiler in a browser.

Owner

commenthol commented Feb 19, 2017

@DavidBruant Thanks for the notice. Caja is indeed an interesting project, I did not know so far. Unfortunately there is no chance to run the compiler in a browser.

@DavidBruant

This comment has been minimized.

Show comment
Hide comment
@DavidBruant

DavidBruant Feb 20, 2017

Unfortunately there is no chance to run the compiler in a browser.

Yeah... they're a bit behind in communication. Caja started as a compiler that transformed JS code into a safer version. Since then, they shared their findings and influenced ECMAScript.
JS has evolved, it's been implemented correctly by browser JS engines. Nowadays, the only thing you need to do to use Caja is load the startSES.js script and call the confine function I mentioned above with the untrusted code. No compiling neither AOT nor JIT.

I probably should have shared ses directly, sorry.

This is a bit off-topic, but I'm happy to continue this discussion elsewhere (safer-eval repo?)

DavidBruant commented Feb 20, 2017

Unfortunately there is no chance to run the compiler in a browser.

Yeah... they're a bit behind in communication. Caja started as a compiler that transformed JS code into a safer version. Since then, they shared their findings and influenced ECMAScript.
JS has evolved, it's been implemented correctly by browser JS engines. Nowadays, the only thing you need to do to use Caja is load the startSES.js script and call the confine function I mentioned above with the untrusted code. No compiling neither AOT nor JIT.

I probably should have shared ses directly, sorry.

This is a bit off-topic, but I'm happy to continue this discussion elsewhere (safer-eval repo?)

@commenthol

This comment has been minimized.

Show comment
Hide comment
@commenthol

commenthol Mar 5, 2017

Owner

v1.1.0 now uses safer-eval for deserialization. eval and Function are disallowed there and built-in objects get wrapped to prevent "sandbox" breakout. On node vm.runInContext is used for real sandboxing. In the browser trust the wrappers or consider sandboxr as alternative. Personally I haven't tried ses.

Owner

commenthol commented Mar 5, 2017

v1.1.0 now uses safer-eval for deserialization. eval and Function are disallowed there and built-in objects get wrapped to prevent "sandbox" breakout. On node vm.runInContext is used for real sandboxing. In the browser trust the wrappers or consider sandboxr as alternative. Personally I haven't tried ses.

@commenthol commenthol closed this Mar 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment