Skip to content

Commit

Permalink
Clarify on eval usage
Browse files Browse the repository at this point in the history
  • Loading branch information
@felixge
felixge committed Oct 8, 2012
1 parent 4680b21 commit 18eaffc
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion README.md
View file
Expand Up @@ -234,7 +234,7 @@ solving the same problem as me:
* big switch statements = bad
* function calls are *really* cheap
* buffering / concatenating buffers is ok
* eval is awesome
* eval is awesome (when using its twin `new Function()`, `eval()` itself sucks)

Here is the eval example:

Expand All @@ -261,6 +261,12 @@ code += '};\n';
var parseRow = new Function('columns', 'parser', code);
```

This optimization turned out to be a huge success, and it's what allowed my
new parser to gain another 20% of performance after being very fast already.

Of course this could turn into a security problem, but that can be easily fixed
by escaping the `column.name` properly.

### Data analysis

The next thing you should do is analyze your data. And for the love of god,
Expand Down

5 comments on commit 18eaffc

@danpalmer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something, but if you are going to eval (or use new Function, which is essentially the same, it parses arbitrary code) on data from an untrusted source, it doesn't matter what has been escaped?

It's not about SQL exploits, it's about parsing whatever text you add to that 'code' variable, which might contain malicious javascript.

@felixge
Copy link
Owner Author

@felixge felixge commented on 18eaffc Oct 8, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eval() gives the executed code access to the current scope, new Function does not. This has additional security and performance implications.

@danpalmer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I realise you don't get access to the current scope, but surely an attacker could send this:

while (true) {}

And a more malicious attacker could probably use child_process.spawn() to open up a reverse shell back to their machine.

@felixge
Copy link
Owner Author

@felixge felixge commented on 18eaffc Oct 9, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I realise you don't get access to the current scope, but surely an attacker could send this:

Please read lines 267 - 268 in the patch above.

@danpalmer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now. Although I still think it's probably best to steer clear of things like this where there is any possibility of getting code executed. Thanks for clearing up the issue.

Please sign in to comment.