8 ms·
The `eval` alone should be enough of a red flag
by minus7 6mo ago
The `eval` alone should be enough of a red flag
- godelski 6mo agoI'm not a JS person, but taking the line at face value shouldn't it to nothing? Which, if I understand correctly, should never be merged. Why would you merge no-ops?
- kordlessagain 6mo agoNo it’s not.
- SahAssar 6mo agoIt really is. There are very few proper use-cases for eval.
- nswango 6mo agoFor a long time the standard way of loading JSON was using eval.
- _flux 6mo agoAnd why do we not anymore make use of it, but instead implemented separate JSON loading functionality in JavaScript? Can you think of any reasons beyond performance?
- bulbar 6mo agoWhy did you opt in for such a comment while a straight forward response without belittling tone would have achieved the same?
- _flux 6mo agoI actually gave it some thought. I had written the actual reason first, but I realized that the person I was responding to must know this, yet keeps arguing in that eval is just fine. I would say they are arguing that in bad faith, so I wanted to enter a dialogue where they are either forced to agree, or more likely, not respond at all.
- bawolff 6mo agoI'd be surprised if there is a performance benefit of processing json with eval(). Browsers optimize the heck out of JSON.
- fhars 6mo agoYou are arguing against the opposite of what the comment you answered to said.
- bawolff 6mo agoAm i? "Can you think of any reasons beyond performance?" implies that the comment author thinks performance would be a valid reason.
- _flux 6mo agoQuoting my original message: > And why do we not anymore make use of it, but instead implemented separate JSON loading functionality in JavaScript? In other words: I'm asking for reasons why was native JSON JavaScript module created, if we already had eval. > Can you think of any reasons beyond performance? One of the reasons is that native JSON parser is faster than eval: give some other reason.
- bawolff 6mo agoNot that long, browsers implemented JSON.parse() back in 2009. JSON was only invented back in 2001 and took a while to become popular. It was a very short window more than a decade ago when eval made sense here. Eval for json also lead to other security issues like XSSI.
- creatonez 6mo agoProblem is, it took until around 2016 for IE6 to be fully dead, so people continued to justify these hacks for a long time. Horrifying times.
- creatonez 6mo agoFor IE7 support, yes... https://caniuse.com/mdn-javascript_builtins_json_parse https://caniuse.com/mdn-javascript_builtins_json_parse
- pavel_lishin 6mo agoWhen is an eval not at least a security "code smell"?
- simonreiff 6mo agoOWASP disagrees: See https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Security_Cheat_Sheet.html https://cheatsheetseries.owasp.org/cheatsheets/Nodejs_Securi..., listing `eval()` first in its small list of examples of "JavaScript functions that are dangerous and should only be used where necessary or unavoidable". I'm unaware of any such uses, myself. I can't think of any scenario where I couldn't get what I wanted by using some combination of `vm`, the `Function` constructor, and a safe wrapper around `JSON.parse()` to do anything I might have considered doing unsafely with `eval()`. Yes, `eval()` is a blatant red flag and definitely should be avoided.
- jacquesm 6mo agoWhile there are valid use cases for eval they are so rare that it should be disabled by default and strongly discouraged as a pattern. Only in very rare cases is eval the right choice and even then it will be fraught with risk.
- godelski 6mo agoThe parent didn't say "there's no legitimate uses of eval", they said "using eval should make people pay more attention." A red flag is a warning. An alert. Not a signal saying "this is 100% no doubt malicious code." Yes, it's a red flag. Yes, there's legitimate uses. Yes, you should always interrogate evals more closely. All these are true
- jeltz 6mo agoYeah, I would have loved to see an example where it was not obvious that there is an exploit. Where it would be possible for a reviewer to actually miss it.
- smaudet 6mo agoSee the comment just above.
- whizzter 6mo agoSadly JS has ways around it that is far from obvious since you can chain effects over multiple files that leads to running code. Like the following example (you can paste it into node to verify), could be spread out over multiple source files to make it even harder to follow: // prelude 1, obfuscate the constructor property name to avoid raising simple analyser alarms const prefix = "construction".substring(0,7); const suffix = "tractor".substring(3); const obfuscatedConstructorName = prefix + suffix; // innocent looking, but we have the indexing name. // prelude 2, get the Function class by indexing a function object with our constructor property name (that does not show up in source-code) const existingFunction = ()=>"nothing here"; const InnocentLookingClass = existingFunction[obfuscatedConstructorName]; // payload decoding elsewhere (this is where we decode our nasty source) const nastyPayloadDisguisedAsData = "console.log('sourced string that could be malicious')"; // Unrelated location where payload gets executed const hardToMissFun = new InnocentLookingClass(nastyPayloadDisguisedAsData); hardToMissFun(); // when this function is run somewhere.. the nasty things happen. Unless you have a data-tracing verifier or a sandbox that is continiously run it's going to be very hard to even come close to determining that arbitrary code is being evaluated in this example. Not a single trace of eval or even that the property name constructor is used.
- smaudet 6mo agoAnd this is just the simple obfuscation case... I can easily imagine where we simply disable eval completely, (except in specific scenarios perhaps). At it's core the issue is dynamism itself. I don't think it should be forsaken, but the issue is, yes, JS does a terrible job of making it explicit. If the language itself required a matter of explicitly, then it would make the whole attack generally more difficult to pull off.