GetIntrinsic.js uses unsafe eval
s3curitybug opened this issue · 18 comments
https://github.com/ljharb/es-abstract/blob/v1.18.0-next.1/GetIntrinsic.js#L21
This line uses Function(), which gets blocked when used with Content Security Policy unless unsafe-eval is used.
It uses it safely, however, since it’s not evaluating user input and it’s not using actual eval.
There’s no other way to detect or conditionally use syntax, and this is not a security bug, so I’m not sure what’s actionable here.
I'm sorry, I did not mean that this automatically represents a security vulnerability. However, es-abstract is a transitive dependency of many big projects, which means that many projects will stop working if they use Content Security Policy when they upgrade to version 1.18.0.
I'm not familiarized with this project, I just upgraded my project and got that problem.
We have always used the function-bind dependency, which uses Function as well, so this wouldn’t be new with v1.18. Have you aliased that out in your bundle so as not to run into the problem?
I was using @babel/preset-env@7.10.4 in my project and it was working fine. Then I upgraded to @babel/preset-env@7.12.1, which uses some dependencies that end up using es-abstract@1.18.0-next.1.
Then, my project bundle started getting blocked (I use Content Security Policy). So I clicked in the console error and I got that the line blocking the script was this one: return Function('"use strict"; return (' + expressionSyntax + ').constructor;')();
So I searched it in my node_modules, and it only showed up in es-abstract -> GetIntrinsic.js. This line was not present in version 1.17.7 (which was the one used by @babel/preset-env@7.10.4): https://github.com/ljharb/es-abstract/blob/v1.17.7/GetIntrinsic.js
Probably many people is going to upgrade to @babel/preset-env@7.12.1 in the following days / weeks.
Using Function does not cause a security vulnerability atomatically, but it means that the script will be blocked in any website using Content Security Policy. For instance, if you try to open the browser console here in GitHub and execute this, it gets blocked:
expressionSyntax = 'async function* () {}';
Function('"use strict"; return (' + expressionSyntax + ').constructor;')()
In general, using eval and Function in production environments is not a good idea.
Would it be possible to use this instead?
(async function dummy1() {}).constructor
(async function* dummy2() {}).constructor
No, because that's syntax, so it will cause a parse error in older browsers/engines.
There is nothing wrong with using Function (and even, with using eval) when it's not run on user input - that's the only place the risk lies. CSP is a useful but very blunt tool to solve that limited security problem. I believe you can configure CSP to allow eval on your own code (which includes your bundles), but disallow it on third-party code (ie, ads) - that's what I'd suggest, if possible.
I'm sorry but I cannot agree with that. CSP is not a "blunt tool to solve a limited security problem". Javascript projects typically use hundreds of dependencies, and any of them may present an unsafe eval, a XSS, etc. (I don't mean this is the case with es-abstract). In my opinion, not using CSP in a large project nowadays is an irresponsibility. CSP is not just to disallow malicious code in ads. It's actually an effetive way to protect your web application.
Maybe es-abstract does not present any vulnerability, but if it is preventing the projects that use it from using CSP (or forcing them to allow unsafe-eval, which pretty much defeats most of the CSP purpose), it is even worse. So yes, there is something very wrong with using Function or eval.
In my case, my project performs critical operations in the front-end, so client side security is extremely important for me. Basically, you are making me choose between heavily downgrading the security of my whole project, or not using es-abstract (and thus, babel, and thus, react), which would force me to start my project again from scratch.
If you search in the internet, you can find that a lot of effort is being put into avoiding eval and Function code for production in webpack, babel, and many other projects, just to allow the usage of CSP without unsafe-eval:
webpack/webpack#4899
webpack/webpack#6461
webpack/webpack#5627
https://stackoverflow.com/questions/39163748/why-is-webpack-creating-output-js-in-form-of-string-and-using-eval-function-to-d
https://stackoverflow.com/questions/50358773/webpack-babel-loader-transpiles-code-with-eval
https://stackoverflow.com/questions/59878880/remove-eval-for-production-code-through-webpack-because-of-csp-issues
https://stackoverflow.com/questions/59053103/webpack-babel-loader-produces-output-containing-eval
ipfs/ipfs-companion#269
...
Browsers are also putting a lot of effor into developing CSP directives.
es-abstract is used in too many projects. It is not wise forcing all of them not to use CSP (or to use it allowing unsafe-eval). I understand that compatibility with older browsers is also important (specially for a library that is used so much). But currently, the code using Function() gets inmediately executed when the script is loaded, which means that the script gets inmediately blocked by CSP. For projects that bundle everything into just one js (like my case), this represents a big problem just because of one line in one transitive dependency.
Would it be possible at least to use syntax, and if it is not supported, falling back to Function?
Don't get me wrong - I like CSP and I want to enable its use.
The JS language itself does not permit any way to get at the value i need except using eval or Function. Syntax errors instantly end the program, they can not be recovered from.
As I said though, function-bind has long been a dependency of es-abstract, which uses Function - it seems like this is a new problem because babel has started using es-abstract.
If there's a way to detect CSP, I could consider only providing these intrinsics in non-CSP environments, which will likely not present a problem for most users of the library - do you know of one?
The next version of es-abstract will use this functionality which has been extracted into a get-intrinsic package, so I've transferred that issue there/here, since that's/here's where it'll need to be solved.
I see the problem. It's about the "async" keyword right? Even if you only execute it inside an if that checks that it is supported, the browser won't even parse the script, right?
I'm afraid there isn't a "clean" way to detect CSP. Basically you can either cause a CSP violation in a try-catch (which already would cause a violation), or do an ajax request to / and check the response headers (which is not clean because it implies doing a request, and it won't detect CSP meta tag configurations, only response header ones).
Any other idea?
It's about the async keyword, and also the * for generators. Yes, exactly - syntax can't be conditionally parsed without eval.
The only thing I can think of is that apps that use CSP, and know their supported syntax level, can manually alias out individual packages with their bundler.
I am pinning my dependencies at older versions that use the older, non-unsafe-eval version of es-abstract. I do not have the option of turning off this CSP directive.
@ivanjonas that won’t allow you to use a lot of bugfixes, or any new polyfills/shims, so i don’t think that’s a sustainable approach.
@ljharb would you consider creating a new version non-compatible with old browsers? maybe not right now, but in the future...
I just came across this problem when introducing CSP to our webapp.
The main problem for me is that the new Function() call happens at module import time. No code in our app actually calls GetIntrinsic with an argument related to async functions/generators (AsyncIteratorPrototype, AsyncFunction, AsyncGenerator, AsyncGeneratorFunction).
Would it be possible to delay it, so that it only happens when one of these intrinsics is actually requested?
For people coming across the issue, for now I've solved it by patching out the problematic part using string-replace-loader during our webpack build.
{
test: /\.(js|mjs|jsx|ts|tsx)$/,
use: {
loader: 'string-replace-loader',
options: {
search: `return Function('"use strict"; return (' + expressionSyntax + ').constructor;')();`,
replace: 'return undefined;',
}
}
}@s3curitybug that would defeat the purpose of the entire es-shims ecosystem, which is to be compatible with every possible JavaScript engine.
@tstehr this is true - the reason it does this is to try to cache the constructor as soon as possible. However, given that these are reachable from syntax and thus impossible to prevent, there’s no reason to eagerly cache them. It would, indeed, be reasonable to defer caching these alone until they’re requested. I’ll look into making that change. Would your CSP settings complain as long as the eval code is never exercised?
The next release should hopefully address this, by deferring Function eval until the particular intrinsics are requested.