Protect against vulnerability in Netscape 4?

[dmsnell]

While scanning through wp_kses() trying to understand some performance issues, I jumped into wp_kses_js_entities(), which wouldn't have stood out to me if I hadn't seen its description...

Removes the HTML JavaScript entities found in early versions of Netscape 4.

This strips out JavaScript that was allowed to be written inside of HTML attributes in a non-standard way but was later removed:

<br size="&{ get_br_size() }" />

The Stack Exchange ​page describing this vulnerability is pretty informative and suggests that the problem was mostly being taken care of circa 2000. I also couldn't find any information on modern browsers supporting this at all. It appears to maybe have only ever affected Netscape 4.

I looked into the commit history and realized that we have never touched this line of code! It appears to have come over from the original kses library that ​still lives on SourceForge, though it was only added in version 0.2.0, a month and a half after the original 0.1.0 release in 2003.

My guess is that at the time some people were still using vulnerable browsers which led Ulf to add it into his library of protection, but we never thought about when we originally included it into core. Nowadays I wonder if we couldn't get rid of it, thinking that this isn't a vulnerability for any browser in the market today.

Interestingly, Ilearned why it's called what it is - from the original author in the README:

Finally, the name kses comes from the terms XSS and access.

Proposing to pull out security checks frightens me, but this is running preg_replace() on so much that it's not even funny, and it probably never catches anything because no one even knows that this was an exploit long ago. Therefore, I would like to ask for some feedback on whether we actually need this, whether it serves any purpose, and whether or not we could help the project by removing it from wp-includes/kses.php.

cc: @nbachiyski

[SergeyBiryukov]

Previously: #20421

[kitchin]

For performance, strpos( $string, '{' ) ? preg_replace( ..., ..., $string ) : $string; ?

[dmsnell]

So today I did something I never thought I'd do: install Netscape Navigator 4 inside Windows 95 inside Virtual Box. I was able to verify the behavior of this vulnerability and that it seems sterile in today's market.

<img src="&{alert('Vuln!')};">

<script language="javascript">
alert('Normal');
</script>

The idea here is that the if the browser is vulnerable we will get the "Vuln!' alert and it will arrive first. Netscape Navigator 4.0 was the only browser I could get to even show the "Vuln!" alert.

A scattering of modern browsers can be seen at ​http://browsershots.org/http://dmsnell.wpsandbox.me/old_xss_vuln.html#

Not even IE 3 was impacted by this.

cc: @nacin

[dmsnell]

I've been trying to measure the performance impact of this check and having trouble getting reliable tests. So far, the statistical variation has been broader than any impact I've been able to measure.

Perhaps later I will try and extract a list of real-world strings and only run wp_kses() over those strings and get some real numbers.

In the meantime, this may indicate that this patch would accomplish mostly an aesthetic/maintenance improvement to the codebase.

@kitchin: I'm not sure how much of a performance impact that code would have here. It appears like the regex is performing quite well, and I noticed in the PHP docs that the PCRE library keeps a global list of compiled expressions, meaning that this probably only calculates the regex once and is quick to apply repeatedly. This might also be why I haven't yet seen a big performance impact.

[pento]

Note that ​a lot of plugins use wp_kses_js_entities(), so a patch can only remove calls to the function, the function itself needs to stay (probably in kses.php, too, for anything that loads KSES directly).

[dmsnell]

Stop scanning for js_entities in wp_kses, empty wp_kses_js_entities()

[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 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 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.

<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 ​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

[dmsnell]

Version 2 of the patch to remove wp_kses_js_entities()

[dmsnell]

Version 3 of patch to remove wp_kses_js_entities() (version 2 left unit tests failing)

[dmsnell]

@nacin or @nbachiyski - any thoughts on this ticket? are we able to move forward on it or is there anything we need from it before it would be ready to merge?

[chriscct7]

It should be noted the last version this appears to affect would be the last stable of Netscape 4.x which was 4.0.8 released on November 9th, 1998 (17 years ago).

[chriscct7]

@dmsnell do you have any performance benchmarks before and after patch to demonstrate the performance improvement?

[chriscct7]

Setting version to implemented in version.

[dmsnell]

Replying to chriscct7:

@dmsnell do you have any performance benchmarks before and after patch to demonstrate the performance improvement?

Unfortunately I don't @chriscct7. Earlier in the process I had attempted to benchmark it, but the difference wasn't that significant on my tests. The variation, likely due to external effects on the server, was higher than the performance impact and so I wasn't able to nail anything down.

Maybe we could build a blog with millions of posts and get some valuable data there. I haven't spent the time to get these numbers because I simply wasn't sure how valuable they are. We know that there is some level of overhead here and we know that the code isn't protecting us against anything, so it seems like it should disappear.

Thanks for the review!

[pento]

This is one of my favourite tickets.

The idea has been tumbling around in my head for a while, I'm leaning towards deprecating the function. The security benefit it provides is negligible, it's nice to improve performance, however slight, and it's a small step towards making wp_kses() fast enough to be usable.

For no_js_entities.3.diff​, instead of changing the behaviour of wp_kses_js_entities(), we can just stop calling it, and move it to deprecated.php. It also needs the addition of a _deprecated_function() call. I like the additional explanation in the docblock, that should stay.

@dmsnell, would you mind updating the patch?

[desrosj]

Adjusted the patch per @pento's feedback.

In addition to removing all calls to wp_kses_js_entities(), the function is now moved to deprecated.php and has a _deprecated_function() call. I restored the original functionality of the function, and the original documentation description, but left @dmsnell's doc additions.

This should now be ready to go in after a look over by @pento.

[pento]

In 38785:

KSES: Deprecate wp_kses_js_entities().

This function was originally introduced to fix an XSS attack in Netscape 4, which never affected any other browsers, or later versions of Netscape.

I'm willing to go out on a limb, and say that we've officially dropped security support for Netscape 4.

Props dmsnell, desrosj.
Fixes #33848.