From ccfe5ad97b2d4dce3f6214cc8826ae9a2c0d80fb Mon Sep 17 00:00:00 2001
From: Tim Starling <tstarling@users.mediawiki.org>
Date: Tue, 4 Jan 2011 06:12:33 +0000
Subject: [PATCH] Fix for bug 26561: clickjacking attacks. See the bug report
 for full documentation.

---
 includes/Article.php                          |  6 +++
 includes/DefaultSettings.php                  | 25 ++++++++++-
 includes/HTMLForm.php                         |  3 ++
 includes/HistoryPage.php                      | 20 +++++++++
 includes/ImagePage.php                        | 26 ++++++++++++
 includes/OutputPage.php                       | 42 +++++++++++++++++++
 includes/Skin.php                             |  1 +
 includes/diff/DifferenceEngine.php            |  5 +++
 includes/installer/WebInstallerOutput.php     |  3 +-
 .../ResourceLoaderStartUpModule.php           |  3 +-
 includes/specials/SpecialAllpages.php         |  1 +
 includes/specials/SpecialCategories.php       |  1 +
 includes/specials/SpecialContributions.php    | 10 +++++
 includes/specials/SpecialLinkSearch.php       |  1 +
 includes/specials/SpecialSearch.php           |  3 +-
 includes/specials/SpecialSpecialpages.php     |  2 +
 includes/specials/SpecialVersion.php          |  1 +
 17 files changed, 147 insertions(+), 6 deletions(-)

diff --git a/includes/Article.php b/includes/Article.php
index 3355b0138d..3b7780fe5a 100644
--- a/includes/Article.php
+++ b/includes/Article.php
@@ -886,6 +886,9 @@ class Article {
 			return;
 		}
 
+		# Allow frames by default
+		$wgOut->allowClickjacking();
+
 		if ( !$wgUseETag && !$this->mTitle->quickUserCan( 'edit' ) ) {
 			$parserOptions->setEditSection( false );
 		}
@@ -1304,6 +1307,7 @@ class Article {
 
 		$sk = $wgUser->getSkin();
 		$token = $wgUser->editToken( $rcid );
+		$wgOut->preventClickjacking();
 
 		$wgOut->addHTML(
 			"<div class='patrollink'>" .
@@ -1584,6 +1588,8 @@ class Article {
 			return;
 		}
 
+		$wgOut->preventClickjacking();
+
 		$tbtext = "";
 		foreach ( $tbs as $o ) {
 			$rmvtxt = "";
diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 02cc4eb342..b35e775642 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -2261,11 +2261,32 @@ $wgUseSiteCss = true;
 $wgEnableTooltipsAndAccesskeys = true;
 
 /**
- * Break out of framesets. This can be used to prevent external sites from
- * framing your site with ads.
+ * Break out of framesets. This can be used to prevent clickjacking attacks,
+ * or to prevent external sites from framing your site with ads.
  */
 $wgBreakFrames = false;
 
+/**
+ * The X-Frame-Options header to send on pages sensitive to clickjacking 
+ * attacks, such as edit pages. This prevents those pages from being displayed
+ * in a frame or iframe. The options are:
+ *
+ *   - 'DENY': Do not allow framing. This is recommended for most wikis.
+ *
+ *   - 'SAMEORIGIN': Allow framing by pages on the same domain. This can be used
+ *         to allow framing within a trusted domain. This is insecure if there
+ *         is a page on the same domain which allows framing of arbitrary URLs.
+ *
+ *   - false: Allow all framing. This opens up the wiki to XSS attacks and thus 
+ *         full compromise of local user accounts. Private wikis behind a 
+ *         corporate firewall are especially vulnerable. This is not 
+ *         recommended.
+ *
+ * For extra safety, set $wgBreakFrames = true, to prevent framing on all pages,
+ * not just edit pages.
+ */
+$wgEditPageFrameOptions = 'DENY';
+
 /**
  * Disable output compression (enabled by default if zlib is available)
  */
diff --git a/includes/HTMLForm.php b/includes/HTMLForm.php
index 8bed092998..d46275481e 100644
--- a/includes/HTMLForm.php
+++ b/includes/HTMLForm.php
@@ -342,6 +342,9 @@ class HTMLForm {
 	function displayForm( $submitResult ) {
 		global $wgOut;
 
+		# For good measure (it is the default)
+		$wgOut->preventClickjacking();
+
 		$html = ''
 			. $this->getErrors( $submitResult )
 			. $this->mHeader
diff --git a/includes/HistoryPage.php b/includes/HistoryPage.php
index 88a8ee44fd..fed09ae1d1 100644
--- a/includes/HistoryPage.php
+++ b/includes/HistoryPage.php
@@ -171,6 +171,7 @@ class HistoryPage {
 			$pager->getBody() .
 			$pager->getNavigationBar()
 		);
+		$wgOut->preventClickjacking( $pager->getPreventClickjacking() );
 
 		wfProfileOut( __METHOD__ );
 	}
@@ -309,6 +310,7 @@ class HistoryPage {
 class HistoryPager extends ReverseChronologicalPager {
 	public $lastRow = false, $counter, $historyPage, $title, $buttons, $conds;
 	protected $oldIdChecked;
+	protected $preventClickjacking = false;
 
 	function __construct( $historyPage, $year = '', $month = '', $tagFilter = '', $conds = array() ) {
 		parent::__construct();
@@ -399,6 +401,7 @@ class HistoryPager extends ReverseChronologicalPager {
 		) . "\n";
 
 		if ( $wgUser->isAllowed( 'deleterevision' ) ) {
+			$this->preventClickjacking();
 			$float = $wgContLang->alignEnd();
 			# Note bug #20966, <button> is non-standard in IE<8
 			$element = Html::element( 'button',
@@ -415,6 +418,7 @@ class HistoryPager extends ReverseChronologicalPager {
 			$this->buttons .= $element;
 		}
 		if ( $wgUser->isAllowed( 'revisionmove' ) ) {
+			$this->preventClickjacking();
 			$float = $wgContLang->alignEnd();
 			# Note bug #20966, <button> is non-standard in IE<8
 			$element = Html::element( 'button',
@@ -516,6 +520,7 @@ class HistoryPager extends ReverseChronologicalPager {
 		$del = '';
 		// Show checkboxes for each revision
 		if ( $wgUser->isAllowed( 'deleterevision' ) || $wgUser->isAllowed( 'revisionmove' ) ) {
+			$this->preventClickjacking();
 			// If revision was hidden from sysops, disable the checkbox
 			// However, if the user has revisionmove rights, we cannot disable the checkbox
 			if ( !$rev->userCan( Revision::DELETED_RESTRICTED ) && !$wgUser->isAllowed( 'revisionmove' ) ) {
@@ -565,6 +570,7 @@ class HistoryPager extends ReverseChronologicalPager {
 		# Rollback and undo links
 		if ( !is_null( $next ) && is_object( $next ) ) {
 			if ( $latest && $this->title->userCan( 'rollback' ) && $this->title->userCan( 'edit' ) ) {
+				$this->preventClickjacking();
 				$tools[] = '<span class="mw-rollback-link">' .
 					$this->getSkin()->buildRollbackLink( $rev ) . '</span>';
 			}
@@ -754,6 +760,20 @@ class HistoryPager extends ReverseChronologicalPager {
 			return '';
 		}
 	}
+
+	/**
+	 * This is called if a write operation is possible from the generated HTML
+	 */
+	function preventClickjacking( $enable = true ) {
+		$this->preventClickjacking = $enable;
+	}
+
+	/**
+	 * Get the "prevent clickjacking" flag
+	 */
+	function getPreventClickjacking() {
+		return $this->preventClickjacking;
+	}
 }
 
 /**
diff --git a/includes/ImagePage.php b/includes/ImagePage.php
index 6676ea83ad..686d3a8ff5 100644
--- a/includes/ImagePage.php
+++ b/includes/ImagePage.php
@@ -601,6 +601,7 @@ EOT
 		$this->loadFile();
 		$pager = new ImageHistoryPseudoPager( $this );
 		$wgOut->addHTML( $pager->getBody() );
+		$wgOut->preventClickjacking( $pager->getPreventClickjacking() );
 
 		$this->img->resetHistory(); // free db resources
 
@@ -828,6 +829,7 @@ EOT
 class ImageHistoryList {
 
 	protected $imagePage, $img, $skin, $title, $repo, $showThumb;
+	protected $preventClickjacking = false;
 
 	public function __construct( $imagePage ) {
 		global $wgUser, $wgShowArchiveThumbnails;
@@ -954,6 +956,7 @@ class ImageHistoryList {
 			# Don't link to unviewable files
 			$row .= '<span class="history-deleted">' . $wgLang->timeAndDate( $timestamp, true ) . '</span>';
 		} elseif ( $file->isDeleted( File::DELETED_FILE ) ) {
+			$this->preventClickjacking();
 			$revdel = SpecialPage::getTitleFor( 'Revisiondelete' );
 			# Make a link to review the image
 			$url = $this->skin->link(
@@ -1041,9 +1044,19 @@ class ImageHistoryList {
 			return wfMsgHtml( 'filehist-nothumb' );
 		}
 	}
+
+	protected function preventClickjacking( $enable = true ) {
+		$this->preventClickjacking = $enable;
+	}
+
+	public function getPreventClickjacking() {
+		return $this->preventClickjacking;
+	}
 }
 
 class ImageHistoryPseudoPager extends ReverseChronologicalPager {
+	protected $preventClickjacking = false;
+
 	function __construct( $imagePage ) {
 		parent::__construct();
 		$this->mImagePage = $imagePage;
@@ -1084,6 +1097,10 @@ class ImageHistoryPseudoPager extends ReverseChronologicalPager {
 				$s .= $list->imageHistoryLine( !$file->isOld(), $file );
 			}
 			$s .= $list->endImageHistoryList( $navLink );
+
+			if ( $list->getPreventClickjacking() ) {
+				$this->preventClickjacking();
+			}
 		}
 		return $s;
 	}
@@ -1166,4 +1183,13 @@ class ImageHistoryPseudoPager extends ReverseChronologicalPager {
 		}
 		$this->mQueryDone = true;
 	}
+	
+	protected function preventClickjacking( $enable = true ) {
+		$this->preventClickjacking = $enable;
+	}
+
+	public function getPreventClickjacking() {
+		return $this->preventClickjacking;
+	}
+
 }
diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 47c560544c..83f6d45b35 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -48,6 +48,7 @@ class OutputPage {
 	var $mPageTitleActionText = '';
 	var $mParseWarnings = array();
 	var $mSquidMaxage = 0;
+	var $mPreventClickjacking = true;
 	var $mRevisionId = null;
 	protected $mTitle = null;
 
@@ -1442,6 +1443,41 @@ class OutputPage {
 		}
 	}
 
+	/**
+	 * Set a flag which will cause an X-Frame-Options header appropriate for 
+	 * edit pages to be sent. The header value is controlled by 
+	 * $wgEditPageFrameOptions.
+	 *
+	 * This is the default for special pages. If you display a CSRF-protected 
+	 * form on an ordinary view page, then you need to call this function.
+	 */
+	public function preventClickjacking( $enable = true ) {
+		$this->mPreventClickjacking = $enable;
+	}
+
+	/**
+	 * Turn off frame-breaking. Alias for $this->preventClickjacking(false).
+	 * This can be called from pages which do not contain any CSRF-protected
+	 * HTML form.
+	 */
+	public function allowClickjacking() {
+		$this->mPreventClickjacking = false;
+	}
+
+	/**
+	 * Get the X-Frame-Options header value (without the name part), or false 
+	 * if there isn't one. This is used by Skin to determine whether to enable 
+	 * JavaScript frame-breaking, for clients that don't support X-Frame-Options.
+	 */
+	public function getFrameOptions() {
+		global $wgBreakFrames, $wgEditPageFrameOptions;
+		if ( $wgBreakFrames ) {
+			return 'DENY';
+		} elseif ( $this->mPreventClickjacking && $wgEditPageFrameOptions ) {
+			return $wgEditPageFrameOptions;
+		}
+	}
+
 	/**
 	 * Send cache control HTTP headers
 	 */
@@ -1665,6 +1701,12 @@ class OutputPage {
 		$wgRequest->response()->header( "Content-type: $wgMimeType; charset={$wgOutputEncoding}" );
 		$wgRequest->response()->header( 'Content-language: ' . $wgLanguageCode );
 
+		// Prevent framing, if requested
+		$frameOptions = $this->getFrameOptions();
+		if ( $frameOptions ) {
+			$wgRequest->response()->header( "X-Frame-Options: $frameOptions" );
+		}
+
 		if ( $this->mArticleBodyOnly ) {
 			$this->out( $this->mBodytext );
 		} else {
diff --git a/includes/Skin.php b/includes/Skin.php
index 8f6937d6da..f94903c84c 100644
--- a/includes/Skin.php
+++ b/includes/Skin.php
@@ -502,6 +502,7 @@ class Skin extends Linker {
 			'wgUserGroups' => $wgUser->getEffectiveGroups(),
 			'wgCurRevisionId' => isset( $wgArticle ) ? $wgArticle->getLatest() : 0,
 			'wgCategories' => $wgOut->getCategories(),
+			'wgBreakFrames' => $wgOut->getFrameOptions() == 'DENY',
 		);
 		foreach ( $wgRestrictionTypes as $type ) {
 			$vars['wgRestriction' . ucfirst( $type )] = $wgTitle->getRestrictions( $type );
diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 83318ddcb6..1dabc782f8 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -115,6 +115,8 @@ class DifferenceEngine {
 		global $wgUser, $wgOut, $wgUseExternalEditor, $wgUseRCPatrol;
 		wfProfileIn( __METHOD__ );
 
+		# Allow frames except in certain special cases
+		$wgOut->allowClickjacking();
 
 		# If external diffs are enabled both globally and for the user,
 		# we'll use the application/x-external-editor interface to call
@@ -206,6 +208,7 @@ CONTROL;
 		// Check if page is editable
 		$editable = $this->mNewRev->getTitle()->userCan( 'edit' );
 		if ( $editable && $this->mNewRev->isCurrent() && $wgUser->isAllowed( 'rollback' ) ) {
+			$wgOut->preventClickjacking();
 			$rollback = '&#160;&#160;&#160;' . $sk->generateRollback( $this->mNewRev );
 		} else {
 			$rollback = '';
@@ -243,6 +246,7 @@ CONTROL;
 			}
 			// Build the link
 			if ( $rcid ) {
+				$wgOut->preventClickjacking();
 				$token = $wgUser->editToken( $rcid );
 				$patrol = ' <span class="patrollink">[' . $sk->link(
 					$this->mTitle,
@@ -471,6 +475,7 @@ CONTROL;
 		if ( $this->mRcidMarkPatrolled && $this->mTitle->quickUserCan( 'patrol' ) ) {
 			$sk = $wgUser->getSkin();
 			$token = $wgUser->editToken( $this->mRcidMarkPatrolled );
+			$wgOut->preventClickjacking();
 			$wgOut->addHTML(
 				"<div class='patrollink'>[" . $sk->link(
 					$this->mTitle,
diff --git a/includes/installer/WebInstallerOutput.php b/includes/installer/WebInstallerOutput.php
index de909a5412..4f68b4f6c1 100644
--- a/includes/installer/WebInstallerOutput.php
+++ b/includes/installer/WebInstallerOutput.php
@@ -162,7 +162,8 @@ class WebInstallerOutput {
 		$this->headerDone = true;
 		$dbTypes = $this->parent->getDBTypes();
 
-		$this->parent->request->response()->header("Content-Type: text/html; charset=utf-8");
+		$this->parent->request->response()->header( 'Content-Type: text/html; charset=utf-8' );
+		$this->parent->request->response()->header( 'X-Frame-Options: DENY' );
 		if ( $this->redirectTarget ) {
 			$this->parent->request->response()->header( 'Location: '.$this->redirectTarget );
 			return;
diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php
index 4d504e57c6..15a68307f5 100644
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -30,7 +30,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
 	
 	protected function getConfig( $context ) {
 		global $wgLoadScript, $wgScript, $wgStylePath, $wgScriptExtension, 
-			$wgArticlePath, $wgScriptPath, $wgServer, $wgContLang, $wgBreakFrames, 
+			$wgArticlePath, $wgScriptPath, $wgServer, $wgContLang, 
 			$wgVariantArticlePath, $wgActionPaths, $wgUseAjax, $wgVersion, 
 			$wgEnableAPI, $wgEnableWriteAPI, $wgDBname, $wgEnableMWSuggest, 
 			$wgSitename, $wgFileExtensions;
@@ -66,7 +66,6 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
 			'wgServer' => $wgServer,
 			'wgUserLanguage' => $context->getLanguage(),
 			'wgContentLanguage' => $wgContLang->getCode(),
-			'wgBreakFrames' => $wgBreakFrames,
 			'wgVersion' => $wgVersion,
 			'wgEnableAPI' => $wgEnableAPI,
 			'wgEnableWriteAPI' => $wgEnableWriteAPI,
diff --git a/includes/specials/SpecialAllpages.php b/includes/specials/SpecialAllpages.php
index 7bb9cddbfe..5fa1aa474d 100644
--- a/includes/specials/SpecialAllpages.php
+++ b/includes/specials/SpecialAllpages.php
@@ -62,6 +62,7 @@ class SpecialAllpages extends IncludableSpecialPage {
 
 		$this->setHeaders();
 		$this->outputHeader();
+		$wgOut->allowClickjacking();
 
 		# GET values
 		$from = $wgRequest->getVal( 'from', null );
diff --git a/includes/specials/SpecialCategories.php b/includes/specials/SpecialCategories.php
index c4104745d8..c2dd40cd46 100644
--- a/includes/specials/SpecialCategories.php
+++ b/includes/specials/SpecialCategories.php
@@ -35,6 +35,7 @@ class SpecialCategories extends SpecialPage {
 
 		$this->setHeaders();
 		$this->outputHeader();
+		$wgOut->allowClickjacking();
 
 		$from = $wgRequest->getText( 'from', $par );
 
diff --git a/includes/specials/SpecialContributions.php b/includes/specials/SpecialContributions.php
index fb9d394ff2..e3a4c71b0b 100644
--- a/includes/specials/SpecialContributions.php
+++ b/includes/specials/SpecialContributions.php
@@ -143,6 +143,7 @@ class SpecialContributions extends SpecialPage {
 					'<p>' . $pager->getNavigationBar() . '</p>'
 				);
 			}
+			$wgOut->preventClickjacking( $pager->getPreventClickjacking() );
 
 
 			# Show the appropriate "footer" message - WHOIS tools, etc.
@@ -485,6 +486,7 @@ class ContribsPager extends ReverseChronologicalPager {
 	public $mDefaultDirection = true;
 	var $messages, $target;
 	var $namespace = '', $mDb;
+	var $preventClickjacking = false;
 
 	function __construct( $options ) {
 		parent::__construct();
@@ -633,6 +635,7 @@ class ContribsPager extends ReverseChronologicalPager {
 			if( !$row->page_is_new && $page->quickUserCan( 'rollback' )
 				&& $page->quickUserCan( 'edit' ) )
 			{
+				$this->preventClickjacking();
 				$topmarktext .= ' '.$sk->generateRollback( $rev );
 			}
 		}
@@ -753,4 +756,11 @@ class ContribsPager extends ReverseChronologicalPager {
 		}
 	}
 
+	protected function preventClickjacking() {
+		$this->preventClickjacking = true;
+	}
+
+	public function getPreventClickjacking() {
+		return $this->preventClickjacking;
+	}
 }
diff --git a/includes/specials/SpecialLinkSearch.php b/includes/specials/SpecialLinkSearch.php
index 6bfcdda3af..e3783d3f6b 100644
--- a/includes/specials/SpecialLinkSearch.php
+++ b/includes/specials/SpecialLinkSearch.php
@@ -45,6 +45,7 @@ class LinkSearchPage extends QueryPage {
 	function execute( $par ) {
 		global $wgOut, $wgRequest, $wgUrlProtocols, $wgMiserMode, $wgLang;
 		$this->setHeaders();
+		$wgOut->allowClickjacking();
 		
 		$target = $wgRequest->getVal( 'target', $par );
 		$namespace = $wgRequest->getIntorNull( 'namespace', null );
diff --git a/includes/specials/SpecialSearch.php b/includes/specials/SpecialSearch.php
index eafbc44cff..fafcf0df96 100644
--- a/includes/specials/SpecialSearch.php
+++ b/includes/specials/SpecialSearch.php
@@ -39,10 +39,11 @@ class SpecialSearch extends SpecialPage {
 	 * @param $par String or null
 	 */
 	public function execute( $par ) {
-		global $wgRequest, $wgUser;
+		global $wgRequest, $wgUser, $wgOut;
 
 		$this->setHeaders();
 		$this->outputHeader();
+		$wgOut->allowClickjacking();
 
 		// Strip underscores from title parameter; most of the time we'll want
 		// text form here. But don't strip underscores from actual text params!
diff --git a/includes/specials/SpecialSpecialpages.php b/includes/specials/SpecialSpecialpages.php
index 42ccbb014b..19bc6b003b 100644
--- a/includes/specials/SpecialSpecialpages.php
+++ b/includes/specials/SpecialSpecialpages.php
@@ -33,8 +33,10 @@ class SpecialSpecialpages extends UnlistedSpecialPage {
 	}
 
 	function execute( $par ) {
+		global $wgOut;
 		$this->setHeaders();
 		$this->outputHeader();
+		$wgOut->allowClickjacking();
 
 		$groups = $this->getPageGroups();
 
diff --git a/includes/specials/SpecialVersion.php b/includes/specials/SpecialVersion.php
index 4ac5888d51..14ea0e56e0 100644
--- a/includes/specials/SpecialVersion.php
+++ b/includes/specials/SpecialVersion.php
@@ -53,6 +53,7 @@ class SpecialVersion extends SpecialPage {
 		
 		$this->setHeaders();
 		$this->outputHeader();
+		$wgOut->allowClickjacking();
 
 		$wgOut->addHTML( Xml::openElement( 'div',
 			array( 'dir' => $wgContLang->getDir() ) ) );
-- 
2.20.1