Skip to content
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

Open
remziatay opened this issue Jan 21, 2022 · 4 comments
Open

Possible injection because of sanitize cache #34

remziatay opened this issue Jan 21, 2022 · 4 comments

Comments

@remziatay
Copy link

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>&lt;strike&gt;test&lt;/strike&gt;</div>
<div><strike>test</strike></div>
<div><strike>test</strike></div>

Expected output:

<div>&lt;strike&gt;test&lt;/strike&gt;</div>
<div><strike>test</strike></div>
<div>&lt;strike&gt;test&lt;/strike&gt;</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.

@aral
Copy link

aral commented Mar 3, 2023

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>&lt;script&gt;alert(&quot;hello&quot;)&lt;/script&gt;</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>&lt;script&gt;alert(&quot;hello&quot;)&lt;/script&gt;</div>
<div>&lt;script&gt;alert(&quot;hello&quot;)&lt;/script&gt;</div>
<div>&lt;script&gt;alert(&quot;hello&quot;)&lt;/script&gt;</div

Given the above, is there a real-world exploit anyone can think of for this?

CC @remziatay @developit

@remziatay
Copy link
Author

@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 &quot;. 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.

@aral
Copy link

aral commented Mar 6, 2023

@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 :)

@remziatay
Copy link
Author

@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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants