r64375 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r64374‎ | r64375 | r64376 >
Date:03:59, 30 March 2010
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Validate background colour to avoid arbitrary style attribute injection. Leads to XSS in versions of IE that support dynamic properties (IE<8, and IE 8 in quirks mode).
Modified paths:
  • /trunk/extensions/InputBox/InputBox.classes.php (modified) (history)

Diff [purge]

Index: trunk/extensions/InputBox/InputBox.classes.php
@@ -510,7 +510,28 @@
511511 // Validate the width; make sure it's a valid, positive integer
512512 $this->mWidth = intval( $this->mWidth <= 0 ? 50 : $this->mWidth );
513513
 514+ // Validate background color
 515+ if ( !$this->isValidColor( $this->mBGColor ) ) {
 516+ $this->mBGColor = 'transparent';
 517+ }
514518 wfProfileOut( __METHOD__ );
515519 }
516520
 521+ /**
 522+ * Do a security check on the bgcolor parameter
 523+ */
 524+ public function isValidColor( $color ) {
 525+ $regex = <<<REGEX
 526+ /^ (
 527+ [a-zA-Z]* | # color names
 528+ \# [0-9a-f]{3} | # short hexadecimal
 529+ \# [0-9a-f]{6} | # long hexadecimal
 530+ rgb \s* \( \s* (
 531+ \d+ \s* , \s* \d+ \s* , \s* \d+ | # rgb integer
 532+ [0-9.]+% \s* , \s* [0-9.]+% \s* , \s* [0-9.]+% # rgb percent
 533+ ) \s* \)
 534+ ) $ /xi
 535+REGEX;
 536+ return (bool) preg_match( $regex, $color );
 537+ }
517538 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r64376MFT r64375: CSS injectiontstarling04:24, 30 March 2010
r64377MFT r64375: CSS injectiontstarling04:25, 30 March 2010
r64378MFT r64375: CSS injectiontstarling04:25, 30 March 2010

Comments

#Comment by Catrope (talk | contribs)   15:38, 30 March 2010

Shouldn't isValidColor() be a static function in Sanitizer.php?

#Comment by Tim Starling (talk | contribs)   21:53, 30 March 2010

That would introduce an unnecessary dependency on the core version.

#Comment by Bryan (talk | contribs)   20:11, 6 December 2010

[a-zA-Z] is redundant with the i-modifier, but ok.

The x-modifier makes reviewing so much easier; I wish that more people used that.

Status & tagging log