-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Prevent arbitrary js code execution #16
Conversation
lib/sheet.js
Outdated
@@ -2,8 +2,15 @@ | |||
|
|||
const metavm = require('metavm'); | |||
|
|||
const math = new Proxy(Math, { | |||
get: (target, prop) => { | |||
if (prop === 'constructor') return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it is still possible to do such a thing:
sheet.cells['A1'] = '=Math.ceil.constructor("console.log(process)")()';
console.log(sheet.values['A1']);
And even this:
sheet.cells['A1'] = '=Math.ceil.constructor("console.log(process.mainModule.require(\'fs\').readFileSync(\'./metacalc.js\', \'utf-8\'))")()';
I will take a closer look today at what we can do to overcome this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One pitfall of the implemented fix I can see now is that constant values such as Math.PI
are no longer available. Accessing them leads to an exception. I don't know if they need to be exposed.
Moreover, I would suggest that fields data
, expressions
and context
of class Sheet
should be private and not accessible from the user-land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, the fix looks good! ✅
I will let you know if I find anything later on.
Thank you for the quick response 🙏🏻
Thank to @primeare for reporting this security issue
npm t
)npm run fmt
)