yireo / Yireo_CspWhitelistInlineJs

22 stars 0 forks source link

Security Improvement #1

Open rommelfreddy opened 2 months ago

rommelfreddy commented 2 months ago

Hello @jissereitsma,

I like the idea of the module! I also thought about developing a similar module.

However, there's one big issue for which I haven't found a good solution.

The problem is that if a hacker gains access to the administration, they could insert custom JavaScript into a configuration that gets output by a block. In this case, your module would also add a nonce to it, thus allowing the script to run.

Another thought: In larger organizations, multiple users might add content through the administration. There might be a "black sheep" who tries to add JavaScript to introduce some functionalities to the website, like a polyfill for Promises (see CVE) ;-) This script would also be modified by your script, and a nonce would be added.

So, while the idea is great, it doesn't fully align with the concept of CSP in a CMS system.


One approach could be to scan all phtml files for scripts and add nonces to scripts found within those files. However, it might be challenging to accurately identify the correct script from the templates since some are generated dynamically.

Another approach could be to implement a script that modifies the generated view_processed files, but this might not be a viable solution in some environments.

Alternatively, we could modify all phtml templates during deployment (using GitHub Actions/GitLab CI/Bitbucket). After making changes to the files, the module could be removed from the project again. However, this would involve modifying the Magento core directly, which is also not ideal.


Do you understand the pain? Do you have another idea we could work on it together?

jissereitsma commented 2 months ago

I actually have a blog lined up about this extension, with exactly this caveat mentioned as well.

However, I just did a little testing: I entered a script-tag with alert into the database directly and went to see if this module would add a nonce or not. It does not - which addresses exactly your security concern. When I look at the CMS block and CMS page functionality, they both use the _toHtml method to output things. However, my module overrides the fetchView method. So this is good.

Still, your point is valid, we simply just need to proof it: For instance, if a widget allows for outputting some text value with HTML in it, this could still lead into an issue (with widgets still using fetchView because they rely upon templates).

Automatically fixing PHTML-templates is an option for your own code, not for an reusable extension. To me, this also counts to the view_processed files (plus, that doesn't work in the Developer Mode).

The only other fix I can see is that block aspects are being inspected - what kind of PHTML template, which block, etc. But still, if HTML is inserted into a template variable it will lead to issues.

Yet one more thing is that we can log handled scripts with this extension, so that it creates a listing of scripts that need to be modified manually.

jissereitsma commented 2 months ago

Version 0.0.3 now ships with a logger.