r79561 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r79560‎ | r79561 | r79562 >
Date:06:12, 4 January 2011
Author:tstarling
Status:ok
Tags:
Comment:
Fix for bug 26561: clickjacking attacks. See the bug report for full documentation.
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/HTMLForm.php (modified) (history)
  • /trunk/phase3/includes/HistoryPage.php (modified) (history)
  • /trunk/phase3/includes/ImagePage.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/Skin.php (modified) (history)
  • /trunk/phase3/includes/diff/DifferenceEngine.php (modified) (history)
  • /trunk/phase3/includes/installer/WebInstallerOutput.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderStartUpModule.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialAllpages.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialCategories.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialLinkSearch.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialSearch.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialSpecialpages.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialVersion.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/diff/DifferenceEngine.php
@@ -115,6 +115,8 @@
116116 global $wgUser, $wgOut, $wgUseExternalEditor, $wgUseRCPatrol;
117117 wfProfileIn( __METHOD__ );
118118
 119+ # Allow frames except in certain special cases
 120+ $wgOut->allowClickjacking();
119121
120122 # If external diffs are enabled both globally and for the user,
121123 # we'll use the application/x-external-editor interface to call
@@ -206,6 +208,7 @@
207209 // Check if page is editable
208210 $editable = $this->mNewRev->getTitle()->userCan( 'edit' );
209211 if ( $editable && $this->mNewRev->isCurrent() && $wgUser->isAllowed( 'rollback' ) ) {
 212+ $wgOut->preventClickjacking();
210213 $rollback = '   ' . $sk->generateRollback( $this->mNewRev );
211214 } else {
212215 $rollback = '';
@@ -243,6 +246,7 @@
244247 }
245248 // Build the link
246249 if ( $rcid ) {
 250+ $wgOut->preventClickjacking();
247251 $token = $wgUser->editToken( $rcid );
248252 $patrol = ' <span class="patrollink">[' . $sk->link(
249253 $this->mTitle,
@@ -471,6 +475,7 @@
472476 if ( $this->mRcidMarkPatrolled && $this->mTitle->quickUserCan( 'patrol' ) ) {
473477 $sk = $wgUser->getSkin();
474478 $token = $wgUser->editToken( $this->mRcidMarkPatrolled );
 479+ $wgOut->preventClickjacking();
475480 $wgOut->addHTML(
476481 "<div class='patrollink'>[" . $sk->link(
477482 $this->mTitle,
Index: trunk/phase3/includes/Article.php
@@ -886,6 +886,9 @@
887887 return;
888888 }
889889
 890+ # Allow frames by default
 891+ $wgOut->allowClickjacking();
 892+
890893 if ( !$wgUseETag && !$this->mTitle->quickUserCan( 'edit' ) ) {
891894 $parserOptions->setEditSection( false );
892895 }
@@ -1304,6 +1307,7 @@
13051308
13061309 $sk = $wgUser->getSkin();
13071310 $token = $wgUser->editToken( $rcid );
 1311+ $wgOut->preventClickjacking();
13081312
13091313 $wgOut->addHTML(
13101314 "<div class='patrollink'>" .
@@ -1584,6 +1588,8 @@
15851589 return;
15861590 }
15871591
 1592+ $wgOut->preventClickjacking();
 1593+
15881594 $tbtext = "";
15891595 foreach ( $tbs as $o ) {
15901596 $rmvtxt = "";
Index: trunk/phase3/includes/ImagePage.php
@@ -601,6 +601,7 @@
602602 $this->loadFile();
603603 $pager = new ImageHistoryPseudoPager( $this );
604604 $wgOut->addHTML( $pager->getBody() );
 605+ $wgOut->preventClickjacking( $pager->getPreventClickjacking() );
605606
606607 $this->img->resetHistory(); // free db resources
607608
@@ -828,6 +829,7 @@
829830 class ImageHistoryList {
830831
831832 protected $imagePage, $img, $skin, $title, $repo, $showThumb;
 833+ protected $preventClickjacking = false;
832834
833835 public function __construct( $imagePage ) {
834836 global $wgUser, $wgShowArchiveThumbnails;
@@ -954,6 +956,7 @@
955957 # Don't link to unviewable files
956958 $row .= '<span class="history-deleted">' . $wgLang->timeAndDate( $timestamp, true ) . '</span>';
957959 } elseif ( $file->isDeleted( File::DELETED_FILE ) ) {
 960+ $this->preventClickjacking();
958961 $revdel = SpecialPage::getTitleFor( 'Revisiondelete' );
959962 # Make a link to review the image
960963 $url = $this->skin->link(
@@ -1041,9 +1044,19 @@
10421045 return wfMsgHtml( 'filehist-nothumb' );
10431046 }
10441047 }
 1048+
 1049+ protected function preventClickjacking( $enable = true ) {
 1050+ $this->preventClickjacking = $enable;
 1051+ }
 1052+
 1053+ public function getPreventClickjacking() {
 1054+ return $this->preventClickjacking;
 1055+ }
10451056 }
10461057
10471058 class ImageHistoryPseudoPager extends ReverseChronologicalPager {
 1059+ protected $preventClickjacking = false;
 1060+
10481061 function __construct( $imagePage ) {
10491062 parent::__construct();
10501063 $this->mImagePage = $imagePage;
@@ -1084,6 +1097,10 @@
10851098 $s .= $list->imageHistoryLine( !$file->isOld(), $file );
10861099 }
10871100 $s .= $list->endImageHistoryList( $navLink );
 1101+
 1102+ if ( $list->getPreventClickjacking() ) {
 1103+ $this->preventClickjacking();
 1104+ }
10881105 }
10891106 return $s;
10901107 }
@@ -1166,4 +1183,13 @@
11671184 }
11681185 $this->mQueryDone = true;
11691186 }
 1187+
 1188+ protected function preventClickjacking( $enable = true ) {
 1189+ $this->preventClickjacking = $enable;
 1190+ }
 1191+
 1192+ public function getPreventClickjacking() {
 1193+ return $this->preventClickjacking;
 1194+ }
 1195+
11701196 }
Index: trunk/phase3/includes/HTMLForm.php
@@ -342,6 +342,9 @@
343343 function displayForm( $submitResult ) {
344344 global $wgOut;
345345
 346+ # For good measure (it is the default)
 347+ $wgOut->preventClickjacking();
 348+
346349 $html = ''
347350 . $this->getErrors( $submitResult )
348351 . $this->mHeader
Index: trunk/phase3/includes/OutputPage.php
@@ -48,6 +48,7 @@
4949 var $mPageTitleActionText = '';
5050 var $mParseWarnings = array();
5151 var $mSquidMaxage = 0;
 52+ var $mPreventClickjacking = true;
5253 var $mRevisionId = null;
5354 protected $mTitle = null;
5455
@@ -1443,6 +1444,41 @@
14441445 }
14451446
14461447 /**
 1448+ * Set a flag which will cause an X-Frame-Options header appropriate for
 1449+ * edit pages to be sent. The header value is controlled by
 1450+ * $wgEditPageFrameOptions.
 1451+ *
 1452+ * This is the default for special pages. If you display a CSRF-protected
 1453+ * form on an ordinary view page, then you need to call this function.
 1454+ */
 1455+ public function preventClickjacking( $enable = true ) {
 1456+ $this->mPreventClickjacking = $enable;
 1457+ }
 1458+
 1459+ /**
 1460+ * Turn off frame-breaking. Alias for $this->preventClickjacking(false).
 1461+ * This can be called from pages which do not contain any CSRF-protected
 1462+ * HTML form.
 1463+ */
 1464+ public function allowClickjacking() {
 1465+ $this->mPreventClickjacking = false;
 1466+ }
 1467+
 1468+ /**
 1469+ * Get the X-Frame-Options header value (without the name part), or false
 1470+ * if there isn't one. This is used by Skin to determine whether to enable
 1471+ * JavaScript frame-breaking, for clients that don't support X-Frame-Options.
 1472+ */
 1473+ public function getFrameOptions() {
 1474+ global $wgBreakFrames, $wgEditPageFrameOptions;
 1475+ if ( $wgBreakFrames ) {
 1476+ return 'DENY';
 1477+ } elseif ( $this->mPreventClickjacking && $wgEditPageFrameOptions ) {
 1478+ return $wgEditPageFrameOptions;
 1479+ }
 1480+ }
 1481+
 1482+ /**
14471483 * Send cache control HTTP headers
14481484 */
14491485 public function sendCacheControl() {
@@ -1665,6 +1701,12 @@
16661702 $wgRequest->response()->header( "Content-type: $wgMimeType; charset={$wgOutputEncoding}" );
16671703 $wgRequest->response()->header( 'Content-language: ' . $wgLanguageCode );
16681704
 1705+ // Prevent framing, if requested
 1706+ $frameOptions = $this->getFrameOptions();
 1707+ if ( $frameOptions ) {
 1708+ $wgRequest->response()->header( "X-Frame-Options: $frameOptions" );
 1709+ }
 1710+
16691711 if ( $this->mArticleBodyOnly ) {
16701712 $this->out( $this->mBodytext );
16711713 } else {
Index: trunk/phase3/includes/HistoryPage.php
@@ -171,6 +171,7 @@
172172 $pager->getBody() .
173173 $pager->getNavigationBar()
174174 );
 175+ $wgOut->preventClickjacking( $pager->getPreventClickjacking() );
175176
176177 wfProfileOut( __METHOD__ );
177178 }
@@ -309,6 +310,7 @@
310311 class HistoryPager extends ReverseChronologicalPager {
311312 public $lastRow = false, $counter, $historyPage, $title, $buttons, $conds;
312313 protected $oldIdChecked;
 314+ protected $preventClickjacking = false;
313315
314316 function __construct( $historyPage, $year = '', $month = '', $tagFilter = '', $conds = array() ) {
315317 parent::__construct();
@@ -399,6 +401,7 @@
400402 ) . "\n";
401403
402404 if ( $wgUser->isAllowed( 'deleterevision' ) ) {
 405+ $this->preventClickjacking();
403406 $float = $wgContLang->alignEnd();
404407 # Note bug #20966, <button> is non-standard in IE<8
405408 $element = Html::element( 'button',
@@ -415,6 +418,7 @@
416419 $this->buttons .= $element;
417420 }
418421 if ( $wgUser->isAllowed( 'revisionmove' ) ) {
 422+ $this->preventClickjacking();
419423 $float = $wgContLang->alignEnd();
420424 # Note bug #20966, <button> is non-standard in IE<8
421425 $element = Html::element( 'button',
@@ -516,6 +520,7 @@
517521 $del = '';
518522 // Show checkboxes for each revision
519523 if ( $wgUser->isAllowed( 'deleterevision' ) || $wgUser->isAllowed( 'revisionmove' ) ) {
 524+ $this->preventClickjacking();
520525 // If revision was hidden from sysops, disable the checkbox
521526 // However, if the user has revisionmove rights, we cannot disable the checkbox
522527 if ( !$rev->userCan( Revision::DELETED_RESTRICTED ) && !$wgUser->isAllowed( 'revisionmove' ) ) {
@@ -565,6 +570,7 @@
566571 # Rollback and undo links
567572 if ( !is_null( $next ) && is_object( $next ) ) {
568573 if ( $latest && $this->title->userCan( 'rollback' ) && $this->title->userCan( 'edit' ) ) {
 574+ $this->preventClickjacking();
569575 $tools[] = '<span class="mw-rollback-link">' .
570576 $this->getSkin()->buildRollbackLink( $rev ) . '</span>';
571577 }
@@ -754,6 +760,20 @@
755761 return '';
756762 }
757763 }
 764+
 765+ /**
 766+ * This is called if a write operation is possible from the generated HTML
 767+ */
 768+ function preventClickjacking( $enable = true ) {
 769+ $this->preventClickjacking = $enable;
 770+ }
 771+
 772+ /**
 773+ * Get the "prevent clickjacking" flag
 774+ */
 775+ function getPreventClickjacking() {
 776+ return $this->preventClickjacking;
 777+ }
758778 }
759779
760780 /**
Index: trunk/phase3/includes/installer/WebInstallerOutput.php
@@ -162,7 +162,8 @@
163163 $this->headerDone = true;
164164 $dbTypes = $this->parent->getDBTypes();
165165
166 - $this->parent->request->response()->header("Content-Type: text/html; charset=utf-8");
 166+ $this->parent->request->response()->header( 'Content-Type: text/html; charset=utf-8' );
 167+ $this->parent->request->response()->header( 'X-Frame-Options: DENY' );
167168 if ( $this->redirectTarget ) {
168169 $this->parent->request->response()->header( 'Location: '.$this->redirectTarget );
169170 return;
Index: trunk/phase3/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -30,7 +30,7 @@
3131
3232 protected function getConfig( $context ) {
3333 global $wgLoadScript, $wgScript, $wgStylePath, $wgScriptExtension,
34 - $wgArticlePath, $wgScriptPath, $wgServer, $wgContLang, $wgBreakFrames,
 34+ $wgArticlePath, $wgScriptPath, $wgServer, $wgContLang,
3535 $wgVariantArticlePath, $wgActionPaths, $wgUseAjax, $wgVersion,
3636 $wgEnableAPI, $wgEnableWriteAPI, $wgDBname, $wgEnableMWSuggest,
3737 $wgSitename, $wgFileExtensions;
@@ -66,7 +66,6 @@
6767 'wgServer' => $wgServer,
6868 'wgUserLanguage' => $context->getLanguage(),
6969 'wgContentLanguage' => $wgContLang->getCode(),
70 - 'wgBreakFrames' => $wgBreakFrames,
7170 'wgVersion' => $wgVersion,
7271 'wgEnableAPI' => $wgEnableAPI,
7372 'wgEnableWriteAPI' => $wgEnableWriteAPI,
Index: trunk/phase3/includes/DefaultSettings.php
@@ -2261,12 +2261,33 @@
22622262 $wgEnableTooltipsAndAccesskeys = true;
22632263
22642264 /**
2265 - * Break out of framesets. This can be used to prevent external sites from
2266 - * framing your site with ads.
 2265+ * Break out of framesets. This can be used to prevent clickjacking attacks,
 2266+ * or to prevent external sites from framing your site with ads.
22672267 */
22682268 $wgBreakFrames = false;
22692269
22702270 /**
 2271+ * The X-Frame-Options header to send on pages sensitive to clickjacking
 2272+ * attacks, such as edit pages. This prevents those pages from being displayed
 2273+ * in a frame or iframe. The options are:
 2274+ *
 2275+ * - 'DENY': Do not allow framing. This is recommended for most wikis.
 2276+ *
 2277+ * - 'SAMEORIGIN': Allow framing by pages on the same domain. This can be used
 2278+ * to allow framing within a trusted domain. This is insecure if there
 2279+ * is a page on the same domain which allows framing of arbitrary URLs.
 2280+ *
 2281+ * - false: Allow all framing. This opens up the wiki to XSS attacks and thus
 2282+ * full compromise of local user accounts. Private wikis behind a
 2283+ * corporate firewall are especially vulnerable. This is not
 2284+ * recommended.
 2285+ *
 2286+ * For extra safety, set $wgBreakFrames = true, to prevent framing on all pages,
 2287+ * not just edit pages.
 2288+ */
 2289+$wgEditPageFrameOptions = 'DENY';
 2290+
 2291+/**
22712292 * Disable output compression (enabled by default if zlib is available)
22722293 */
22732294 $wgDisableOutputCompression = false;
Index: trunk/phase3/includes/specials/SpecialAllpages.php
@@ -62,6 +62,7 @@
6363
6464 $this->setHeaders();
6565 $this->outputHeader();
 66+ $wgOut->allowClickjacking();
6667
6768 # GET values
6869 $from = $wgRequest->getVal( 'from', null );
Index: trunk/phase3/includes/specials/SpecialCategories.php
@@ -35,6 +35,7 @@
3636
3737 $this->setHeaders();
3838 $this->outputHeader();
 39+ $wgOut->allowClickjacking();
3940
4041 $from = $wgRequest->getText( 'from', $par );
4142
Index: trunk/phase3/includes/specials/SpecialSpecialpages.php
@@ -33,8 +33,10 @@
3434 }
3535
3636 function execute( $par ) {
 37+ global $wgOut;
3738 $this->setHeaders();
3839 $this->outputHeader();
 40+ $wgOut->allowClickjacking();
3941
4042 $groups = $this->getPageGroups();
4143
Index: trunk/phase3/includes/specials/SpecialContributions.php
@@ -143,6 +143,7 @@
144144 '<p>' . $pager->getNavigationBar() . '</p>'
145145 );
146146 }
 147+ $wgOut->preventClickjacking( $pager->getPreventClickjacking() );
147148
148149
149150 # Show the appropriate "footer" message - WHOIS tools, etc.
@@ -485,6 +486,7 @@
486487 public $mDefaultDirection = true;
487488 var $messages, $target;
488489 var $namespace = '', $mDb;
 490+ var $preventClickjacking = false;
489491
490492 function __construct( $options ) {
491493 parent::__construct();
@@ -633,6 +635,7 @@
634636 if( !$row->page_is_new && $page->quickUserCan( 'rollback' )
635637 && $page->quickUserCan( 'edit' ) )
636638 {
 639+ $this->preventClickjacking();
637640 $topmarktext .= ' '.$sk->generateRollback( $rev );
638641 }
639642 }
@@ -753,4 +756,11 @@
754757 }
755758 }
756759
 760+ protected function preventClickjacking() {
 761+ $this->preventClickjacking = true;
 762+ }
 763+
 764+ public function getPreventClickjacking() {
 765+ return $this->preventClickjacking;
 766+ }
757767 }
Index: trunk/phase3/includes/specials/SpecialVersion.php
@@ -53,6 +53,7 @@
5454
5555 $this->setHeaders();
5656 $this->outputHeader();
 57+ $wgOut->allowClickjacking();
5758
5859 $wgOut->addHTML( Xml::openElement( 'div',
5960 array( 'dir' => $wgContLang->getDir() ) ) );
Index: trunk/phase3/includes/specials/SpecialSearch.php
@@ -39,10 +39,11 @@
4040 * @param $par String or null
4141 */
4242 public function execute( $par ) {
43 - global $wgRequest, $wgUser;
 43+ global $wgRequest, $wgUser, $wgOut;
4444
4545 $this->setHeaders();
4646 $this->outputHeader();
 47+ $wgOut->allowClickjacking();
4748
4849 // Strip underscores from title parameter; most of the time we'll want
4950 // text form here. But don't strip underscores from actual text params!
Index: trunk/phase3/includes/specials/SpecialLinkSearch.php
@@ -45,6 +45,7 @@
4646 function execute( $par ) {
4747 global $wgOut, $wgRequest, $wgUrlProtocols, $wgMiserMode, $wgLang;
4848 $this->setHeaders();
 49+ $wgOut->allowClickjacking();
4950
5051 $target = $wgRequest->getVal( 'target', $par );
5152 $namespace = $wgRequest->getIntorNull( 'namespace', null );
Index: trunk/phase3/includes/Skin.php
@@ -502,6 +502,7 @@
503503 'wgUserGroups' => $wgUser->getEffectiveGroups(),
504504 'wgCurRevisionId' => isset( $wgArticle ) ? $wgArticle->getLatest() : 0,
505505 'wgCategories' => $wgOut->getCategories(),
 506+ 'wgBreakFrames' => $wgOut->getFrameOptions() == 'DENY',
506507 );
507508 foreach ( $wgRestrictionTypes as $type ) {
508509 $vars['wgRestriction' . ucfirst( $type )] = $wgTitle->getRestrictions( $type );

Follow-up revisions

RevisionCommit summaryAuthorDate
r79562MFT r79561, bug 26561: fix clickjacking vulnerabilities.tstarling06:15, 4 January 2011
r79563Merge r79562 from REL1_16: bug 26561, clickjacking defences.tstarling06:26, 4 January 2011
r79566(bug 26561) Simplified clickjacking patch.tstarling07:06, 4 January 2011
r797211.17: MFT r79561 (clickjacking fix). SpecialSearch.php and SpecialLinkSearch....catrope15:02, 6 January 2011

Status & tagging log