Make WordPress Core

Opened 19 years ago

Closed 19 years ago

Last modified 13 months ago

#2023 closed defect (bug) (fixed)

XSS Vulnerability Fix v1.0.1

Reported by: shovel's profile shovel Owned by: shovel's profile shovel
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)

classes.php (39.1 KB) - added by shovel 19 years ago.
The new classes.php with appended fix.

Download all attachments as: .zip

Change History (14)

@shovel
19 years ago

The new classes.php with appended fix.

#1 @masquerade
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.

#2 @shovel
19 years ago

  • Owner changed from anonymous to shovel

#3 @shovel
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 @shovel
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 @masquerade
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 @masquerade
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 @shovel
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.

#8 @markjaquith
19 years ago

  • Milestone changed from 1.5.2 to 2.0.1

Is any action needed on this ticket?

#9 @ryan
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 @davidhouse
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.

#11 @(none)
18 years ago

  • Milestone 2.0.1 deleted

Milestone 2.0.1 deleted

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

@ryelle commented on PR #562:


13 months ago
#13

I'm going to merge this, since it doesn't require a deploy & isn't affected by a site's Gutenberg version.

Note: See TracTickets for help on using tickets.