Escape blame popup log messages before embedding in inline JS - #243
Open
michael-o wants to merge 1 commit into
Open
Escape blame popup log messages before embedding in inline JS#243michael-o wants to merge 1 commit into
michael-o wants to merge 1 commit into
Conversation
blame.php builds a per-revision hover-popup message for blame-popup.js by embedding each commit's log message directly into an inline <script> block, then blame-popup.js assigns it via innerHTML. The message was only passed through addslashes(), which escapes quote/backslash characters for the surrounding JS string literal but does nothing for HTML-significant characters. A log message containing "</script><script>..." could close the legitimate <script> block early and inject an arbitrary script, and a message containing markup like "<img src=x onerror=...>" would execute once assigned to innerHTML, even without breaking out of the script tag. This is the same class of stored XSS as d4de770/#228, but in a sink neither of those commits touched. Apply xml_entities() before addslashes(): entity-encoding neutralizes real HTML metacharacters in the message, and addslashes() is still needed separately, since a message containing a raw "\uXXXX"-style sequence would otherwise be decoded into a real character by the browser's JS string-literal parser before the value ever reaches innerHTML, after entity-encoding has already run. Also fix the line-flattening regex, which only matched a bare "\n" and left a stray "\r" in messages using CRLF line endings, causing display artifacts in the popup.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
blame.php builds a per-revision hover-popup message for blame-popup.js by embedding each commit's log message directly into an inline <script> block, then blame-popup.js assigns it via innerHTML. The message was only passed through addslashes(), which escapes quote/backslash characters for the surrounding JS string literal but does nothing for HTML-significant characters.
A log message containing "</script><script>..." could close the legitimate <script> block early and inject an arbitrary script, and a message containing markup like "
" would execute once assigned to innerHTML, even without breaking out of the script tag. This is the same class of stored XSS as d4de770/#228, but in a sink neither of those commits touched.
Apply xml_entities() before addslashes(): entity-encoding neutralizes real HTML metacharacters in the message, and addslashes() is still needed separately, since a message containing a raw "\uXXXX"-style sequence would otherwise be decoded into a real character by the browser's JS string-literal parser before the value ever reaches innerHTML, after entity-encoding has already run.
Also fix the line-flattening regex, which only matched a bare "\n" and left a stray "\r" in messages using CRLF line endings, causing display artifacts in the popup.