#2023 closed defect (bug) (fixed)
XSS Vulnerability Fix v1.0.1
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Priority: | highest omg bbq | |
Severity: | critical | Version: | 1.5.2 |
Component: | Administration | Keywords: | xss problem vulnerability fix has-patch |
Focuses: | Cc: |
Description
This affects the /wp-includes/classes.php file. A new preg_replace() with an array will stop malicious XSS attacks in the Search functionality from the index.php.
Find in /wp-includes/classes.php
// If a search pattern is specified, load the posts that match if (!empty($q['s'])) { $q['s'] = addslashes_gpc($q['s']); $search = ' AND ('; $q['s'] = preg_replace('/, +/', ' ', $q['s']); $q['s'] = str_replace(',', ' ', $q['s']); $q['s'] = str_replace('"', ' ', $q['s']); $q['s'] = trim($q['s']);
Replace with this code.
// If a search pattern is specified, load the posts that match if (!empty($q['s'])) { $q['s'] = addslashes_gpc($q['s']); $search = ' AND ('; //$q['s'] = preg_replace('#(&\#x*)([0-9A-F]+);*#iu', ' ', $q['s']); //$q['s'] = str_replace(',', ' ', $q['s']); //$q['s'] = str_replace('"', ' ', $q['s']); //$q['s'] = trim($q['s']); // -- BEGIN Anthony White's (firespade@gmail.com) XSS Patch v1.0.1 -- // $tonystr = array ('@<script[^>]*?>.*?</script>@si', // kill js '@<[\/\!]*?[^<>]*?>@si', // kill html '@([\r\n])[\s]+@', // trim whitespace '@&(quot|#34);@i', '@&(amp|#38);@i', '@&(lt|#60);@i', '@&(gt|#62);@i', '@&(nbsp|#160);@i', '@&(iexcl|#161);@i', '@&(cent|#162);@i', '@&(pound|#163);@i', '@&(copy|#169);@i', '@&#(\d+);@e'); // -- replacements $replace = array ('', '', '\1', '"', '&', '<', '>', ' ', chr(161), chr(162), chr(163), chr(169), 'chr(\1)'); // -- new preg_replace() $q['s'] = preg_replace($tonystr, $replace, $q['s']); // -- END Anthony White's (firespade@gmail.com) XSS Patch v1.0.1 -- //
Enjoyed making this contribution. :)
Attachments (1)
Change History (14)
#1
@
19 years ago
This is by far one of the worst examples of a patch to fix a simple problem I have ever seen.
There is only one point in the code where the search results might be echo'd out onto the page, and that is when you're in the next and previous URIs. Instead of silly regexps, let's just use urlencode in the spots where this may be echo'd.
#3
@
19 years ago
Okay so you're gonna add a urlencode to every single field out there? Do it then, I'm just trying to be helpful. I haven't had this software but a single day and already discovered problems, you've probably been here for months. That's pretty pathetic down talking someone who's trying to save WordPress users from having XSS attacks. If you don't like it, don't fix it. Simple as that. If you think you're so smart, how come it took me one day find this problem and it took you up until this point to figure it out? Don't get mouthy with me because you think you're high and mighty, save that for your friends. The things I do to help.. ridiculous.
#4
@
19 years ago
forgot to go through your post. so i'm adding more input on what you've said.
'silly regexps'
first off, regexps are highly regarded in web development community. if you don't believe me, go ask Jelsoft makers of vBulletin. and then tell them to switch all their regexps to urlencodes for their 100+ input fields...yeah, you'll just get laughed at.
'There is only one point in the code where the search results might be echo'd out onto the page, and that is when you're in the next and previous URIs'
that's incorrect. it affects the index as well as anything else i need to maliciously attack via XSS. if you aren't familiar with XSS why did post in here?
'let's just use urlencode in the spots where this may be echo'd'
and this?... well, this obviously shows your lack of skills in the simplest of software design.
and not only is it a good fix, but you all should start filtering ALL GPC (GET POST COOKIE) bits. you take care of it with a single function, make it run in every class (consistant with GPC) and you won't have to - what was it you said? "use urlencode in the spots where this may be echo'd"
if you can't program, don't bitch. peace.
Anthony White aka shovel
#5
@
19 years ago
The only time that unsanitized HTML is echo'd out in the standard templates is:
1) Next or Previous pages when the search results are paginated. This should be urlencoded anyways, because it is going in a link as the search string
We do not need to unnecessarily filter what may otherwise be perfectly good search keywords. Just because somebody puts a character that may be maliciously in, doesn't mean we should remove it. Instead, we make sure that the output is encoded in a form that cannot be read as normal (X)HTML. We should use urlencode for this instead, because not only will it make sure that the output won't be run as XHTML, it will ensure that the output is valid HTML because the URI will be cleansed of any wrong input characters.
And please, leave your personal bouts and profanity off of trac. If you want to bring up arguements, do it on a mailing list or in an e-mail to me, as this is hardly the place.
#6
@
19 years ago
Er, a mistake in what I pointed out above was pointed out, its also output in the title tag for the page.
Normally, I'd agree that input needs to be sanitized as close to the input as possible, but this is one of the cases where it is a bit different. By far the worst thing we can do is alter the search content, as perfectly legitimate search may contain characters that would be removed or altered. The alternative would normally be to just run htmlspecialchars on all input we recieve. This however, would break the Next and Previous page links because they wouldn't have the original search content in them, and they really should be urlencoded anyways to ensure XHTML validity as WordPress preaches. So, instead, we should deal with this on a case by case basis. The output in the title tag should be run through htmlspecialchars, and the links should be run through urlencode, both to fix XSS and XHTML validity.
I do believe those are the only place where the output permitted, and if you can correct me and would like to further discuss this, I invite you to hop on WordPress' IRC channel and ping me.
#7
@
19 years ago
My appologies for being rude and under-estimating your abilites. I agree with you. It seems as though the problem lies within the skins themselves, but you can fix the problem from the backend, ignoring the skins Search functionality. I'll see what I can do. Thanks.
#9
@
19 years ago
We already fixed the page links. The default template echoes $s is one place, but it is run through wp_specialchars() first.
#10
@
19 years ago
- Resolution set to fixed
- Status changed from new to closed
fixed/invalid/wontfix: We seemed to have covered the two areas masquerade mentioned earlier. As this is a theme-level problem then it's down to themes to make sure that they don't let any unescaped strings through.
This ticket was mentioned in PR #562 on WordPress/wporg-mu-plugins by @ryelle.
13 months ago
#12
- Keywords has-patch added
The Interactivity API is now only available as a JS module, not a plain script, so we need to build and register our view
code differently. As of v27, @wordpress/scripts
supports module builds, but that also breaks compat with Node <18. So this PR updates the node version to match core, and updates the rest of the JS packages.
I've also changed the build process to call wp-scripts directly for each block, rather than using webpack + importing the wp-scripts webpack config. The block build step is now equivalent to running:
npx wp-scripts build --experimental-modules --webpack-src-dir=./mu-plugins/blocks/[block]/src --output-path=./mu-plugins/blocks/[block]/build
Unfortunately due to the folder structure, we still need the custom script (wp-scripts doesn't understand how to find [block]/src/block.json
, it only understands src/[block]/block.json
).
See https://github.com/WordPress/wporg-parent-2021/issues/122, https://github.com/WordPress/gutenberg/pull/56143, https://github.com/WordPress/gutenberg/pull/57461
The new classes.php with appended fix.