Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

OiCMudkips
Copy link

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.

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/", prematurely closing the script
tag that a Hypernova comment is in, causing the page to
crash.
@ljharb
Copy link
Collaborator

ljharb commented Mar 27, 2020

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

@OiCMudkips
Copy link
Author

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

@OiCMudkips OiCMudkips closed this Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants