homebrewery
homebrewery copied to clipboard
naturalcrit
Add DOM Purify to BrewRenderer
This PR adds DOM Purify to the BrewRenderer, and uses it to sanitize all Page and Style content immediately before it is added to the component output (using dangerouslySetInnerHTML).
This PR extends the work done should close #3293.
From #3293:
We should remove any possibility of Javascript execution inside brews, that means removing inline event handlers like onClick, attributes that allow the javascript: protocol, clean svg files, and clean iframes from any javascript.
Testing:
- [x] element
onmethods (onClick,onError, etc) - removed - [x] JavaScript in HREF property - property is removed
- [x]
<iframe>- tag and content is completely removed - [x]
<script>inside<svg>-<script>removed,<svg>remains
So this appears to resolve the issues raised in #3293, although as suggested in that issue, a Content Security Policy may also be needed for defense in depth. I found a really good slide set from Cure53 which discusses the issue here (https://www.usenix.org/sites/default/files/conference/protected-files/enigma16_slides_heiderich.pdf), or if academic papers are your thing, this conference paper (https://link.springer.com/chapter/10.1007/978-3-319-66399-9_7).
JavaScript in HREF property - property is removed
Does this mean that the href prop is removed only when javascript inside is detected?
JavaScript in HREF property - property is removed
Does this mean that the href prop is removed only when javascript inside is detected?
That is my recollection from testing. Looks like there's a deployment up now so you should be able to test.
This piece of code is what i used to test this:
<a href="javascript:alert('This is a JavaScript injection via href attribute')">Click me</a>
<img src="javascript:alert('This is a JavaScript injection via src attribute')">
<form action="javascript:alert('This is a JavaScript injection via action attribute')">
<input type="submit" value="Submit">
</form>
<div style="background-color: red; color: white; width: 100px; height: 100px;" onclick="alert('This is a JavaScript injection via inline event handler')">
Click me
</div>
<div onmouseover="alert('This is a JavaScript injection via inline event handler')">Hover over me</div>
<style>
.test { background-image: url('javascript:alert("This is a JavaScript injection via CSS background-image")'); }
</style>
<div class="test">Test</div>
<div data-code="javascript:alert('This is a JavaScript injection via data attribute')">Test</div>
<div id="parent">
<button id="child">Click me</button>
</div>
<script>
document.getElementById('parent').addEventListener('click', function(event) {
if (event.target.id === 'child') {
console.log('This is JavaScript executed via event delegation');
}
});
</script>
Using this code in the live site results in a complete crash, but in this deployment seems to work just fine.
#3446 might add some cases we want to check.
Is there any Js injection method we haven't tested yet?
Added a small change to template.js to stop </script> tags from breaking everything.
Related to this, as mentioned, </script> can be input in the title of a brew. If the only content of a brew or the starting content of the brew is that string, it will be used as title, therefore, the entire userpage is broken, and the entire editpage is broken too, which makes it very difficult to fix. the only fix i have found is to go into this deployment as it shares the db, and login manually, then deleting the brew from userpage or editpage.
I have removed the </script> sanitization from this PR, it was becoming a much bigger job than just a single additional line of code - it will require it's own PR.
Related to this, as mentioned,
</script>can be input in the title of a brew. If the only content of a brew or the starting content of the brew is that string, it will be used as title, therefore, the entire userpage is broken, and the entire editpage is broken too, which makes it very difficult to fix. the only fix i have found is to go into this deployment as it shares the db, and login manually, then deleting the brew from userpage or editpage.
This is a different issue though — we are emitting strings from brews without proper encoding, and they are being treated as html elements by the browser's html parser.
For example, this line in template.js:
<title>${title.length ?${title} - The Homebrewery: 'The Homebrewery - NaturalCrit'}</title>
Here, if the title is the string A < B && TRUE then that emits as literally that in the html source .. but it should be A < B && TRUE. The browser will then render that as the string A < B && TRUE. The same should happen if the title string is <script>..</script>. It should not be delivered to the browser as if it is actual html code to be parsed. We do the encoding properly in other places (e.g. brew cards on userpage).
The </script> issue is a separate issue, as recorded in #546. This PR, while similar, addresses the issue of active content being parsed by the BrewRenderer, not the HTML template. As such, this PR can be merged as is and a second PR raised to resolve the issues with template.js.
Ok, I think this is working fine. Going to just poke a little more and then merge it. 👍
