-
Notifications
You must be signed in to change notification settings - Fork 35
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
Possible injection because of sanitize cache #34
Comments
While I can reproduce this with the 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') ( 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:
Given the above, is there a real-world exploit anyone can think of for this? |
@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 const secondTry = html`<div><script>alert("hello")</script></div>` This doesn't inject either because quotes are rendered as 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
|
@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 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 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 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; |
Basically it's possible to inject dirty html:
This is the output:
Expected output:
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.The text was updated successfully, but these errors were encountered: