-
Notifications
You must be signed in to change notification settings - Fork 17
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
Filtrex fails CSP #54
Comments
Hey, Danny! Internally, filtrex works by parsing the expression and immediately translating it to the corresponding JavaScript code. However, there is no way for the user to directly enter any JavaScript code at all – the resulting JS code is always composed of pre-made chunks of code (eg. this) and properly escaped user input (eg. this). The only thing that passes right into the JS code unescaped are numbers, but the allowed syntax there has been chosen very conservatively. Therefore, personally I wouldn't be too worried about an XSS. That being said, everybody makes mistakes, and even if I didn't, that wouldn't change company policies :) There are several ways I can think of that would allow filtrex not to use
import { compileExpression } from 'filtrex';
import { safelyCompileJS } from 'my-favorite-js-sandbox';
let options = {
compileJavaScript = (args, src) => {
const vm = safelyCompileJS(`function result(${args}){${src}}`)
return vm.getVariable('result')
}
};
const f = compileExpression(`category == "meal"`, options)
I'd love to hear your take on this :) |
Yeah, it is complicated indeed. I don't have a solution either. It's just that people look at their CSP evaluators and say: 'hey, this piece of code is not safe so we cannot allow it.' We have some software using filtrex and a government agency ran this check and complains that our software is not safe enough blablabla. All because somewhere in the code there is a |
@dannybloe Curious question: do your clients check whether |
I think they just did a simple search. We ship code with a piece that could theoretically be harmful. It is very hard to prove that a certain piece of code is ever executed of course. Same that it is almost impossible to prove some piece of code ends. At least that's what I learned in some computer science class millenia ago 😉 |
Which means that in order for Filtrex to pass CSP, it can't have |
Well, and things should definitly not become slower of course so that would also rule out option 1. |
I think it would be possible to do both option 3 and also do issue #40 at once by rewriting to peggy, and write the grammar such that it builds a series of nested functions that can be passed the arbitrary object I haven't actually tested it yet, but at least theoretically there shouldn't be much of a performance hit thanks to automatic inlining performed by modern javascript engines I have had a lot of fun using pegjs in the past, so I'll try and take a stab at this on my own fork in the next weeks/month or two and let you know how it goes
|
Update on my previous comment having now taken a stab at this, as I unfortunately won't be able to follow through on an implementation - This approach would pretty much entail a ground-up re-write of the library, which is unfortunately more than I've got in me at this time. |
Yeah. I totally understand. |
Hi folks,
We are happily using filtrex for quite some time but we see some of our clients complain that the filtrex code is unsecure becuse it uses
new Function
.See https://stackoverflow.com/questions/52573756/what-are-the-eval-related-functions-to-be-avoided-when-csp-is-enabled for instance.
Is that something that can be fixed? I guess more and more people will run into this with filtrex eventually as more organisations start to use more strict CSP rules.
Cheers,
Danny
The text was updated successfully, but these errors were encountered: