[wp-trac] [WordPress Trac] #33848: Protect against vulnerability in Netscape 4?

[WordPress Trac]

#33848: Protect against vulnerability in Netscape 4?
-------------------------+------------------------------
 Reporter:  dmsnell      |       Owner:
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  Awaiting Review
Component:  Security     |     Version:  4.4
 Severity:  normal       |  Resolution:
 Keywords:               |     Focuses:  performance
-------------------------+------------------------------

Comment (by dmsnell):

 As best as I can tell, we have further issues regarding this ticket.
 Here's the basic breakdown:

  - Removing the call to `wp_kses_js_entities()` breaks the kses unit
 tests: `phpunit -c phpunit.xml.dist --group uses`
  - That unit test is in
 [https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/kses.php#L197,
 line 197 of tests/phpunit/tests/kses.php]
  - The goal of that test appears to be checking a XSS vuln that is still
 at large but `wp_kses_js_entities()` is ''specifically'' for a deprecated
 bug that never really made it big.
  - I ''think'' that the unit test has been passing due to an irregularity
 with the code, PHP's handling of entities, XML's representation of the
 entities, and a bug in the regular expression. Because this has been
 complicated, I'm honestly having trouble pinning it down.

 '''A bug in the regex?'''

 Kind of, not really. The regex tested with a star when a plus would have
 been more specific to the original vulnerability.

 {{{%&\s*\{[^}]*(\}\s*;?|$)%}}}

 For reference, the original vulnerability looks like this...

 {{{<IMG SRC="&{ badCode() }"> }}}

 if there's nothing between those angle brackets, there is thus no security
 issue because the vulnerability is that someone ''could'' run code between
 those brackets, not that their inherent existence is a problem.

 Here you can see that the regex is matching between the brackets with
 {{{\{[^}]*}}}, which matches zero or more occurrences of any character
 before the closing bracket or end of string. This is important. Fixing the
 "regex bug" would mean we do this instead, {{{\{[^}]+}}}

 When the
 [https://core.trac.wordpress.org/browser/trunk/tests/phpunit/data/formatting/xssAttacks.xml#L5
 input string] runs through `wp_kses()` in the unit test, we get the
 following end of the string {{{&{} }}}. Now we should note that other
 filters in `wp_kses()`are getting rid of the nasty {{{<SCRIPT>}}} tags and
 that this `js_entities` regex doesn't do a thing to protect against it.
 Ironically, an ampersand followed by curly brackets with zero characters
 between them ''does'' match the regex and gets replaced with a blank, the
 expected result according to the test.

 Ironically, when I attempted to look at the history of the test I hit a
 dead end, and of all the possible links, this was imported from version
 1337 in a previous repository, now served by a 404. I suspect that this
 oddity in the `${}` was never caught when the test was written but I could
 be wrong and I could be missing something big.

 '''XML, HTML entities, and PHP is crazy - does the filter work?'''
 If we aren't testing for the vulnerability the filter was designed for, is
 our unit test still protecting us here against XSS attacks? That was my
 question, so I made a site to test it.

 {{{#!php
 <form action="<? echo $_SERVER['SCRIPT_NAME'] ?>" method="POST">
 <textarea name="text"></textarea>
 <button>Submit</button>
 <button name="sanitize">Sanitize</button>
 </form>

 <?php

 function wp_kses_js_entities( $string ) {
         return preg_replace('%&\s*\{[^}]*(\}\s*;?|$)%', '', $string);
 }

 if ( isset( $_POST['sanitize'] ) ) {
         echo "<p>Sanitized</p>";
         echo wp_kses_js_entities( $_POST['text'] );
 } else {
         echo $_POST['text'];
 }
 }}}

 I have this running at [http://dmsnell.wpsandbox.me/xss.php] but you will
 need to disable the XSS auditor in your browser, otherwise it will
 automatically prevent the attack.

 {{{
 chrome --disable-xss-auditor
 // mac
 /Applications/Google Chrome.app/Contents/MacOS/Google Chrome --disable-
 xss-auditor
 }}}

 You can load that page and then paste in the string from the
 [https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#XSS_Locator
 owasp site] for XSS locator, then click on submit or sanitize to see the
 output simply echoed versus going through `wp_kses_js_entities()`

 {{{
 ';alert(String.fromCharCode(88,83,83))//';alert(String.fromCharCode(88,83,83))//";
 alert(String.fromCharCode(88,83,83))//";alert(String.fromCharCode(88,83,83))//--
 ></SCRIPT>">'><SCRIPT>alert(String.fromCharCode(88,83,83))</SCRIPT>
 }}}

 If I understand this properly, the XSS attack comes through in both cases,
 meaning this filter is irrelevant and the unit test is offering no
 protection. I am still unconvinced that I understand everything 100%.
 There's a lot going on with string encodings that I really want to be
 certain of before pushing this out there, but I believe we could safely
 update the expected string in the unit test to add back the irregularity
 that left us without the final `${}`, wipe out `wp_kses_js_entities()`,
 and be just as protected as we have been.

 Any help or guidance would be greatly appreciated. I'll go ahead and
 submit a patch when I get the chance.

 cc: @nacin

--
Ticket URL: <https://core.trac.wordpress.org/ticket/33848#comment:7>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform