vhtml
vhtml copied to clipboard
developit
Possible injection because of sanitize cache
Basically it's possible to inject dirty html:
const striked = '<strike>test</strike>';
console.log(<div>{striked}</div>);
console.log(<div><strike>test</strike></div>);
console.log(<div>{striked}</div>);
This is the output:
<div><strike>test</strike></div>
<div><strike>test</strike></div>
<div><strike>test</strike></div>
Expected output:
<div><strike>test</strike></div>
<div><strike>test</strike></div>
<div><strike>test</strike></div>
After rendering <div><strike>test</strike></div>, it caches <strike>test</strike> and doesn't sanitize it anymore. It can be seen live here as well. Just because something was rendered before, it shouldn't mean that it's sanitized.
While I can reproduce this with the <strike> example, above, I can’t reproduce it in my fork with a script injection attempt
The following tape tests pass in my fork:
const scriptInjectionAttempt = '<script>alert("hello")</script>'
const firstTry = html`<div>${scriptInjectionAttempt}</div>`
const secondTry = html`<div>${'<script>alert("hello")</script>'}</div>`
const thirdTry = html`<div>${scriptInjectionAttempt}</div>`
const expectedResult = '<div><script>alert("hello")</script></div>'
t.equal(firstTry, expectedResult, 'should sanitise uncached script injection attempt')
t.equal(secondTry, expectedResult, 'should sanitise script injection attempt on initial cache hit')
t.equal(thirdTry, expectedResult, 'should sanitise script injection attempt subsequence cache hits')
(html is xhtm.bind(vhtml) where vhtml is my fork.)
Update: same results using original htm and vhtml:
import htm from 'htm'
import vhtml from 'vhtml'
const html = htm.bind(vhtml)
const scriptInjectionAttempt = '<script>alert("hello")</script>'
const firstTry = html`<div>${scriptInjectionAttempt}</div>`
const secondTry = html`<div>${'<script>alert("hello")</script>'}</div>`
const thirdTry = html`<div>${scriptInjectionAttempt}</div>`
console.log(firstTry)
console.log(secondTry)
console.log(thirdTry)
Outputs:
<div><script>alert("hello")</script></div>
<div><script>alert("hello")</script></div>
<div><script>alert("hello")</script></div
Given the above, is there a real-world exploit anyone can think of for this?
CC @remziatay @developit
@aral One thing wrong with this test scenario is that the script is injected as string. This is only an issue if the markup was already successfully rendered before. So secondTry should be changed as:
const secondTry = html`<div><script>alert("hello")</script></div>`
This doesn't inject either because quotes are rendered as ". So we can change the script to something like this and then it will be injected:
const scriptInjectionAttempt = '<script>alert(123)</script>'
const secondTry = html`<div><script>alert(123)</script></div>`
const thirdTry = html`<div>${scriptInjectionAttempt}</div>` // Injected
Real world exploit idea: We have a social media app with a logout button, some ad iframes on the side and we render users' posts. We expect them to be sanitised but
- Users could post one of our ad's iframe, then the ad will be rendered in their post, won't be sanitised.
- Users could set their name as the html of logout button, then their name will be rendered as logout button.
@remziatay Thanks, Remzi. I was clearly confused while writing that :)
The more I think about this, the more I feel there isn’t really a satisfactory way to fix this given how vhtml works at vhtml’s level. That said, I’m also not even sure vhtml is the right place to be carrying out sanitisation of HTML. vhtml should be rendering the hyperscript it’s passed and outputting valid HTML.
Unless I’m missing something, sanitisation should be carried out at a higher level (either at the tagged template level or even higher, at point of use in a framework). For Kitten, I’m thinking of introducing a global sanitiseUntrustedContent() method that you call on… umm… untrusted content that you want to interpolate into your pages.
Update: I’ve removed the sanitisation and changed the name of my fork (so folks aren’t confused by the difference in behaviour). The expectation in Kitten is that you call sanitise() on any untrusted content. Otherwise, all interpolations are raw and you can embed styles and scripts and HTML entities, etc., work. Which makes for a much nicer authoring experience. (https://codeberg.org/small-tech/hyperscript-to-html-string).
PS. Thanks again for talking this through with me; it really helped :)
@aral I'm glad :) IMO escaping is a fundamental part of what vhtml offers. And it does it well, this is just a bug due to limitations of returning a primitive and traversing the tree starting with the deepest child (hence the need for cache). If we allow vhtml to change its API a little (even returning a String object, instead of primitive string), this issue is preventable.
Something like this instead of the cache object would solve the problem but I understand it may not be desired to change the API and break compatibility
s += child.sanitized ? child : esc(child);
s = new String(s);
s.sanitized = true;
return s;