handlebars, php and smarty highlighter flawed by design (modifies output)

[Rob--W]

Code like

prism/components/prism-php.js

Lines 67 to 71 in 25cdd3f

	env.code = env.code.replace(/(?:<\?php|<\?)[\w\W]*?(?:\?>)/ig, function(match) {
	env.tokenStack.push(match);
	return '{{{PHP' + env.tokenStack.length + '}}}';
	});

replaces parts of the input with a predictable sentinel.
After finishing highlighting, code like

prism/components/prism-php.js

Lines 88 to 91 in 25cdd3f

	for (var i = 0, t; t = env.tokenStack[i]; i++) {
	// The replace prevents $$, $&, $`, $', $n, $nn from being interpreted as special patterns
	env.highlightedCode = env.highlightedCode.replace('{{{PHP' + (i + 1) + '}}}', Prism.highlight(t, env.grammar, 'php').replace(/\$/g, '$$$$'));
	}

replaces the sentinel in the HTML with the highlighted version of the original value.

The implementation has a flaw: There is no guarantee that the sentinel is unique. For example:

• Input: {{{PHP1}}}<?php x ?>
• Output: <?php x ?>{{{PHP1}}} (note that the rendered output differs from the input!)

This can be solved by repeating the sentinel generation step until the sentinel is unique (e.g. by repeatedly incrementing a counter, or with a random number).

Another problem with replacing predictable sentinels in the HTML after highlighting is that if someone manages to put the sentinel in a HTML attribute, then the resulting HTML can be unsafe, leading to XSS in the worst case.

[Golmote]

Yes, this is a known flaw. If you have any idea to improve these components, feel free to submit a PR.

[zeitgeist87]

Should be fixed now in all three languages.

[Rob--W]

The following commits solve the first part of my reported issue:
5df26e2 Smarty
36bc560 PHP
7a1a404 Handlebars

I don't see anything that fixes the second part,

Another problem with replacing predictable sentinels in the HTML after highlighting is that if someone manages to put the sentinel in a HTML attribute, then the resulting HTML can be unsafe, leading to XSS in the worst case.

[Golmote]

How could we solve this? Should we generate a different pattern for the sentinel based on the code? Like, we could look for different patterns in the initial code, and keep generating new ones while we find they are already used. That way we would end up with a not (or less) predictable pattern that is safe using for the replacements?

Edit: oh that's actually exactly what you suggested in your initial post... I did not read it again before posting ><

[Rob--W]

I haven't suggested a solution yet. My suggestion and what you wrote there is exactly what was implemented in the three commits that I referencecd in my last comment (which fixed the first of the two reported issues).

To fix the last outstanding issue, you have to detect the difference between content that is inside an attribute, and content that is outside of it. For example, currently " is not escaped unless it is inside an auto-generated HTML attribute -

prism/prism.js

Lines 456 to 460 in 8eb0ab6

	var attributes = Object.keys(env.attributes).map(function(name) {
	return name + '="' + (env.attributes[name] || '').replace(/"/g, '"') + '"';
	}).join(' ');
	return '<' + env.tag + ' class="' + env.classes.join(' ') + '"' + (attributes ? ' ' + attributes : '') + '>' + env.content + '</' + env.tag + '>';

And also, instead of replacing in a loop, I suggest to use a regexp and a function in the replacer. Then you can be certain that the replacement string cannot affect the next replacements.

Here is a quick example for the Smarty highlighter:

prism/components/prism-smarty.js

Line 98 in 8eb0ab6

return '___SMARTY' + env.tokenStack.length + '___';

Replace

				return '___SMARTY' + env.tokenStack.length + '___';

with

				return '___SMARTY"' + env.tokenStack.length + '___';

prism/components/prism-smarty.js

Lines 119 to 122 in 8eb0ab6

	for (var i = 0, t; t = env.tokenStack[i]; i++) {
	// The replace prevents $$, $&, $`, $', $n, $nn from being interpreted as special patterns
	env.highlightedCode = env.highlightedCode.replace('___SMARTY' + (i + 1) + '___', Prism.highlight(t, env.grammar, 'smarty').replace(/\$/g, '$$$$'));
	}

Replace

for (var i = 0, t; t = env.tokenStack[i]; i++) {
	// The replace prevents $$, $&, $`, $', $n, $nn from being interpreted as special patterns
	env.highlightedCode = env.highlightedCode.replace('___SMARTY' + (i + 1) + '___', Prism.highlight(t, env.grammar, 'smarty').replace(/\$/g, '$$$$'));
}

with

if (env.tokenStack.length) {
    env.highlightedCode = env.highlightedCode.replace(/___SMARTY(")([0-9]+)___)/g, function(match, quot, i) {
        if (!(i in env.tokenStack)) return match;
        var text = env.tokenStack[i];
        if (quot === '"') {
            // " was escaped, so we are inside a HTML attribute. Don't highlight.
            return text.replace(/"/g, '"');
        }
        // The replace prevents $$, $&, $`, $', $n, $nn from being interpreted as special patterns
        return Prism.highlight(text, env.grammar, 'smarty').replace(/\$/g, '$$$$'));
    });
}

[zeitgeist87]

Another problem with replacing predictable sentinels in the HTML after highlighting is that if someone manages to put the sentinel in a HTML attribute, then the resulting HTML can be unsafe, leading to XSS in the worst case.

You are correct. The autolinker plugin for example puts a possibly user supplied URL in the href attribute. This URL could easily contain a string like ___SMARTY0___. I hadn't thought of that.

And also, instead of replacing in a loop, I suggest to use a regexp and a function in the replacer. Then you can be certain that the replacement string cannot affect the next replacements.

That is a good idea.

You cannot use a quotation mark in the sentinel, because it would interfere with the highlighting:

<p class="<?php ?>"></p> 

The above would become this for markup highlighting:

<p class="___PHP"0___"></p> 

Then the regex for tag in markup would no longer match.

How about we count the sentinel occurrences? Every sentinel can only occur once in env.highlightedCode. If there are more, then something is fishy and we refuse to do the replacement...

[Rob--W]

How about we count the sentinel occurrences? Every sentinel can only occur once in env.highlightedCode. If there are more, then something is fishy and we refuse to do the replacement...

This is an easy way out, but may cause unexpected behavior when the code has a legitimate need for using the sentinel as a part of the code.

If " cannot be used, you could use another special character and do a safe replacement. E.g. replacing some rarely used unicode character with its &#[code];-equivalent, and using that special unicode character in the sentinel. In the event that the unicode character occurs in the original code, the user doesn't see the difference because the &#[code];-entity renders the original glyph.

[zeitgeist87]

On second thought, if ___PHP0___ was in a URL in a comment, then my code would also find it in env.code and avoid it:

<!-- Exploit: http://example.com/___PHP0___ -->
<?php /* some kind of XSS*/ ?>

You are definitely right, that env.attributes is super dangerous, but I don't see a practical exploit with the current stock plugins. Maybe I am wrong with this assessment. Can you provide a proof of concept exploit?

[zeitgeist87]

This is an easy way out, but may cause unexpected behavior when the code has a legitimate need for using the sentinel as a part of the code.

In that case my recent patches would choose a different sentinel. If for example ___PHP2___ already occurs in the code, then it is simply skipped and not used as a sentinel.

[Golmote]

I don't see what can be done (if there is to do) without an actual proof of concept, so I'm closing this issue in the mean time.

[reyronald]

Hello guys, sorry for commenting on a closed issue. I stumbled upon something similar to what has been discussed here and just wanted to make sure it was the same issue. For a Smarty snippet, after running .highlightElement I'm getting this:

<div style="color: {$color};"></div>

=>

<div style="color: ___SMARTY0___;"></div>

Here's a CodeSandbox with the reproduction in case it's useful: https://codesandbox.io/s/2pk8rlmxwr. Something similar is happening with Erb files too.

Just want to know if I can expect a fix sometime eventually @Golmote

Reference: refined-bitbucket/refined-bitbucket#234