[wp-trac] [WordPress Trac] #33848: Protect against vulnerability in Netscape 4?
WordPress Trac
noreply at wordpress.org
Mon Dec 7 18:32:37 UTC 2015
#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
More information about the wp-trac
mailing list