search

airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 211 forks source link

Add "<" (&lt) to the ENCODE list #182

Closed OiCMudkips closed 4 years ago

OiCMudkips commented 4 years ago

What?

Also encode the < character in src/index.js.

Why?

We have found that browsers (at least Chrome and Firefox) can interpret "/" to close a tag in a HTML comment in a script tag.

This means that a malicious attacker could submit a payload like "</script/", get it into hydration data, and prematurely close the script tag that a Hypernova comment is in, causing the page to crash.

Here's a POC (please feel free to inspect and rename it to .html if you believe I'm not giving you malware): whoops.txt

<!DOCTYPE html5>

<body>
  <h1 id="h1">Script didn't run (bad)</h1>
  <!-- if you remove the comment line in the script the script runs as expected -->
  <script>
    <!-- {"text": "</script/"} -->
    document.getElementById("h1").innerText = "Script ran (good)"
  </script>
</body>

Alternatives

Maybe we should only include < and drop > from the list if performance is a concern. Replacing the comment-in-script with <!-- {"text": "&lt;/script>"} --> correctly runs the script for me in Chrome, so I think escaping only < is acceptable.

ljharb commented 4 years ago

167 seems to cover this; it's not all opening tags, it's just </script.

OiCMudkips commented 4 years ago

Whoops, didn't see that PR. Looking forward to seeing it merged!