0

Curious as to what unforeseen issues this type of code might present, if executed on the server. Or if there are any non eval alternatives.

var a = {b:1, c:2, d:3, e:[1,2,3]};
(function(path) { return eval('this'+path) }).call(a, '.e[2]');
Victor S
  • 5,098
  • 5
  • 44
  • 62
  • where does `path` come from? besides, `return this[path]` works – dandavis Jun 01 '15 at 18:46
  • `path` comes in a param from client. – Victor S Jun 01 '15 at 18:47
  • Are you sure `this[path]` works? – Victor S Jun 01 '15 at 18:48
  • 1
    then it's very dangerous, slow, and unneeded, just use the array syntax shown. – dandavis Jun 01 '15 at 18:48
  • if path doesn't have a dot, it will. you can use `this[path.slice(1)]` if it starts with a dot... but inside eval, this==this outside. if your object is deeper, use a resolve() function. – dandavis Jun 01 '15 at 18:49
  • yes that only works if `.` is the only dellimiter, but it won't if more complex cases like `.e[2].b[3].c`, etc. – Victor S Jun 01 '15 at 18:50
  • Can you explain why it's dangerous and slow? I have read up on eval and I'm pretty sure I understand the basics, but I don't think it's slow, (the object a, in this case is not very large, and if there isn't a chance for eval to escape it's closure and access anything except the `this` object, which is the provided "a" object, then I don't see any security issues. But if someone can prove me wrong with proof I would like to learn... – Victor S Jun 01 '15 at 18:56
  • try `path=".d=0;require('fs').unlink('./app.js');123"; eval('this'+path) });` (well, don't try it actually) eval itself is slower than normal code because it takes so much work to build and tear down a new environ, compared to a single prop lookup, which on V8 is near instant. – dandavis Jun 01 '15 at 19:01
  • That's not how I run eval, eval is in a closure, also you could have provided a less harmful example. I've tried an alternative: `path=".d;console.log('hello'))" ".d;console.log('hello'))" (function(path) { return eval('this'+path) }).call(a, path); VM413:1 Uncaught SyntaxError: Unexpected token ) at Object. (:2:37) at :2:47` – Victor S Jun 01 '15 at 19:05
  • it looks like you'll need a try/catch too, which also slows down V8. i think your test "XSS" is mal-formed (an extra ")"), but i'm sure the right combo of escaping and nesting can execute arbitrary code. will anyone ever pull your card? maybe not, but why leave the trap door open? – dandavis Jun 01 '15 at 19:11
  • Yes, with the correct path=".d;console.log('hello')", it prints "hello" in the console... some suggestions I got was to only allow `alphanumeric`, `[]` and `.` – Victor S Jun 01 '15 at 19:16
  • 1
    don't play cat and mouse. do yourself a favor just get rid of the eval and use a resolve(). look up how to access object properties by a string, it's been answered a lot. godspeed. – dandavis Jun 01 '15 at 19:19

1 Answers1

1

Given that path is a static value (".e[2]") and a does not have any malicious accessors or so, there is nothing insecure here except that it's totally unnecessary.

However, if path does come from a client or some other untrusted source, then passing it to eval is the worst thing you can do. It can do everything that JS code can do in node - and that is enough to harm you severely.

And yes, there are tons of non-eval alternatives.

Community
  • 1
  • 1
Bergi
  • 630,263
  • 148
  • 957
  • 1,375