Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F4529853
T134931-WIP.patch
Bawolff (Brian Wolff)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Authored By
Bawolff
Sep 27 2016, 2:47 PM
2016-09-27 14:47:43 (UTC+0)
Size
17 KB
Referenced Files
None
Subscribers
None
T134931-WIP.patch
View Options
From 0a32d992c36c30fa1baf14c85d853361d7f0020f Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Tue, 27 Sep 2016 13:13:01 +0000
Subject: [PATCH] [WIP; NOT TESTED] Low severity security-ish issues in
CentralAuth special pages
Things that are not "good" but not exploitable
* Double escaping
* Using raw html i18n messages
* Misleading code comments
* Change implicit __toString() on some messages to explicit ->parse()
* Require POST for changes that edit stuff
Special:MergeAccount has not yet been looked at in this patch.
Bug: T134931
---
includes/specials/SpecialCentralAuth.php | 2 +-
.../specials/SpecialGlobalGroupPermissions.php | 17 +++++++----
includes/specials/SpecialGlobalRenameQueue.php | 2 +-
includes/specials/SpecialGlobalRenameRequest.php | 6 ++--
includes/specials/SpecialGlobalRenameUser.php | 2 +-
includes/specials/SpecialGlobalUsers.php | 2 +-
includes/specials/SpecialMultiLock.php | 24 +++++++--------
includes/specials/SpecialUsersWhoWillBeRenamed.php | 6 ++--
includes/specials/SpecialWikiSets.php | 34 +++++++++++++---------
9 files changed, 54 insertions(+), 41 deletions(-)
diff --git a/includes/specials/SpecialCentralAuth.php b/includes/specials/SpecialCentralAuth.php
index 5a59443..b5749b1 100644
--- a/includes/specials/SpecialCentralAuth.php
+++ b/includes/specials/SpecialCentralAuth.php
@@ -323,7 +323,7 @@ class SpecialCentralAuth extends SpecialPage {
$reg = $globalUser->getRegistration();
$age = $this->prettyTimespan( wfTimestamp( TS_UNIX ) - wfTimestamp( TS_UNIX, $reg ) );
$attribs = array(
- 'username' => $globalUser->getName(),
+ 'username' => htmlspecialchars( $globalUser->getName() ),
'registered' => htmlspecialchars( $this->getLanguage()->timeanddate( $reg, true ) . " ($age)" ),
'editcount' => htmlspecialchars( $this->getLanguage()->formatNum( $this->evaluateTotalEditcount() ) ),
'attached' => htmlspecialchars( $this->getLanguage()->formatNum( count( $this->mAttachedLocalAccounts ) ) ),
diff --git a/includes/specials/SpecialGlobalGroupPermissions.php b/includes/specials/SpecialGlobalGroupPermissions.php
index b002e7f..6d0ca8a 100644
--- a/includes/specials/SpecialGlobalGroupPermissions.php
+++ b/includes/specials/SpecialGlobalGroupPermissions.php
@@ -60,7 +60,11 @@ class SpecialGlobalGroupPermissions extends SpecialPage {
$subpage = $this->getRequest()->getVal( 'wpGroup' );
}
- if ( $subpage != '' && $this->getUser()->matchEditToken( $this->getRequest()->getVal( 'wpEditToken' ) ) ) {
+ if (
+ $subpage != ''
+ && $this->getUser()->matchEditToken( $this->getRequest()->getVal( 'wpEditToken' ) )
+ && $this->getRequest()->wasPosted()
+ ) {
$this->doSubmit( $subpage );
} elseif ( $subpage != '' ) {
$this->buildGroupView( $subpage );
@@ -232,7 +236,7 @@ class SpecialGlobalGroupPermissions extends SpecialPage {
if ( $editable ) {
$fields['centralauth-editgroup-name'] = Xml::input( 'wpGlobalGroupName', 50, $group );
} else {
- $fields['centralauth-editgroup-name'] = $group;
+ $fields['centralauth-editgroup-name'] = htmlspecialchars( $group );
}
if( $this->getUser()->isAllowed( 'editinterface' ) ) {
@@ -240,8 +244,8 @@ class SpecialGlobalGroupPermissions extends SpecialPage {
$fields['centralauth-editgroup-display'] = $this->msg( 'centralauth-editgroup-display-edit', $group, User::getGroupName( $group ) )->parse();
$fields['centralauth-editgroup-member'] = $this->msg( 'centralauth-editgroup-member-edit', $group, User::getGroupMember( $group ) )->parse();
} else {
- $fields['centralauth-editgroup-display'] = User::getGroupName( $group );
- $fields['centralauth-editgroup-member'] = User::getGroupMember( $group );
+ $fields['centralauth-editgroup-display'] = htmlspecialchars( User::getGroupName( $group ) );
+ $fields['centralauth-editgroup-member'] = htmlspecialchars( User::getGroupMember( $group ) );
}
$fields['centralauth-editgroup-members'] = $this->msg( 'centralauth-editgroup-members-link', $group, User::getGroupMember( $group ) )->parse();
$fields['centralauth-editgroup-restrictions'] = $this->buildWikiSetSelector( $group );
@@ -279,7 +283,7 @@ class SpecialGlobalGroupPermissions extends SpecialPage {
htmlspecialchars( $set->getName() )
);
} else {
- return $this->msg( 'centralauth-editgroup-nowikiset' );
+ return $this->msg( 'centralauth-editgroup-nowikiset' )->parse();
}
}
@@ -373,7 +377,8 @@ class SpecialGlobalGroupPermissions extends SpecialPage {
* @param $group string
*/
function doSubmit( $group ) {
- // Paranoia -- the edit token shouldn't match anyway
+ // It is important to check userCanEdit, as otherwise an
+ // unauthorized user could manually construct a POST request.
if ( !$this->userCanEdit( $this->getUser() ) ) {
return;
}
diff --git a/includes/specials/SpecialGlobalRenameQueue.php b/includes/specials/SpecialGlobalRenameQueue.php
index 53b4314..76e17f3 100644
--- a/includes/specials/SpecialGlobalRenameQueue.php
+++ b/includes/specials/SpecialGlobalRenameQueue.php
@@ -431,7 +431,7 @@ class SpecialGlobalRenameQueue extends SpecialPage {
if ( $titleBlacklist instanceof TitleBlacklistEntry ) {
$form->addHeaderText(
$this->msg( 'globalrenamequeue-request-titleblacklist' )
- ->rawParams( $titleBlacklist->getRegex() )->parseAsBlock()
+ ->params( wfEscapeWikiText( $titleBlacklist->getRegex() ) )->parseAsBlock()
);
}
}
diff --git a/includes/specials/SpecialGlobalRenameRequest.php b/includes/specials/SpecialGlobalRenameRequest.php
index ebada93..4f3908c 100644
--- a/includes/specials/SpecialGlobalRenameRequest.php
+++ b/includes/specials/SpecialGlobalRenameRequest.php
@@ -233,7 +233,7 @@ class SpecialGlobalRenameRequest extends FormSpecialPage {
return true;
}
$check = GlobalRenameRequest::isNameAvailable( $value );
- return $check->isGood() ? true : (string) $check->getMessage();
+ return $check->isGood() ? true : $check->getMessage()->parse();
}
/**
@@ -245,9 +245,9 @@ class SpecialGlobalRenameRequest extends FormSpecialPage {
*/
public function validateEmail ( $value, $alldata, HTMLForm $form ) {
if ( $alldata['email'] !== $alldata['email2'] ) {
- return $this->msg( 'globalrenamerequest-email-mismatch' )->text();
+ return $this->msg( 'globalrenamerequest-email-mismatch' )->parse();
} elseif ( !Sanitizer::validateEmail( $alldata['email'] ) ) {
- return $this->msg( 'globalrenamerequest-email-invalid' )->text();
+ return $this->msg( 'globalrenamerequest-email-invalid' )->parse();
}
return true;
}
diff --git a/includes/specials/SpecialGlobalRenameUser.php b/includes/specials/SpecialGlobalRenameUser.php
index ab3b0fc..e2055c0 100644
--- a/includes/specials/SpecialGlobalRenameUser.php
+++ b/includes/specials/SpecialGlobalRenameUser.php
@@ -187,7 +187,7 @@ class SpecialGlobalRenameUser extends FormSpecialPage {
if ( $titleBlacklist instanceof TitleBlacklistEntry ) {
return Status::newFatal(
$this->msg( 'centralauth-rename-titleblacklist-match' )
- ->rawParams( $titleBlacklist->getRegex() )
+ ->params( wfEscapeWikiText( $titleBlacklist->getRegex() ) )
);
}
}
diff --git a/includes/specials/SpecialGlobalUsers.php b/includes/specials/SpecialGlobalUsers.php
index 12e0141..aeec096 100644
--- a/includes/specials/SpecialGlobalUsers.php
+++ b/includes/specials/SpecialGlobalUsers.php
@@ -254,7 +254,7 @@ class GlobalUsersPager extends AlphabeticPager {
foreach ( $this->globalIDGroups[$id] as $group ) {
if ( !in_array( $group, $this->localWikisets ) ) {
// Mark if the group is not applied on this wiki
- $rights[] = Html::element( 'span',
+ $rights[] = Html::rawElement( 'span',
array( 'class' => 'groupnotappliedhere' ),
User::makeGroupLinkWiki( $group, User::getGroupMember( $group ) )
);
diff --git a/includes/specials/SpecialMultiLock.php b/includes/specials/SpecialMultiLock.php
index 416c245..96e3659 100644
--- a/includes/specials/SpecialMultiLock.php
+++ b/includes/specials/SpecialMultiLock.php
@@ -141,28 +141,28 @@ class SpecialMultiLock extends SpecialPage {
$form = '';
$radioLocked =
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-lock-nochange' )->parse(),
+ $this->msg( 'centralauth-admin-action-lock-nochange' )->text(),
'wpActionLock',
'nochange',
'mw-centralauth-status-locked-no',
true ) .
'<br />' .
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-lock-unlock' )->parse(),
+ $this->msg( 'centralauth-admin-action-lock-unlock' )->text(),
'wpActionLock',
'unlock',
'centralauth-admin-action-lock-unlock',
false ) .
'<br />' .
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-lock-lock' )->parse(),
+ $this->msg( 'centralauth-admin-action-lock-lock' )->text(),
'wpActionLock',
'lock',
'centralauth-admin-action-lock-lock',
false );
$radioHidden =
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-hide-nochange' )->parse(),
+ $this->msg( 'centralauth-admin-action-hide-nochange' )->text(),
'wpActionHide',
'nochange',
'mw-centralauth-status-hidden-nochange',
@@ -170,21 +170,21 @@ class SpecialMultiLock extends SpecialPage {
'<br />';
if ( $this->mCanOversight ) {
$radioHidden .= Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-hide-none' )->parse(),
+ $this->msg( 'centralauth-admin-action-hide-none' )->text(),
'wpActionHide',
CentralAuthUser::HIDDEN_NONE,
'mw-centralauth-status-hidden-no',
false ) .
'<br />' .
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-hide-lists' )->parse(),
+ $this->msg( 'centralauth-admin-action-hide-lists' )->text(),
'wpActionHide',
CentralAuthUser::HIDDEN_LISTS,
'mw-centralauth-status-hidden-list',
false ) .
'<br />' .
Xml::radioLabel(
- $this->msg( 'centralauth-admin-action-hide-oversight' )->parse(),
+ $this->msg( 'centralauth-admin-action-hide-oversight' )->text(),
'wpActionHide',
CentralAuthUser::HIDDEN_OVERSIGHT,
'mw-centralauth-status-hidden-oversight',
@@ -198,7 +198,7 @@ class SpecialMultiLock extends SpecialPage {
$this->msg( 'centralauth-admin-reason-other-select' )->inContentLanguage()->text()
);
$reasonField = Xml::input( 'wpReason', 45, false );
- $botField = Xml::check( 'markasbot' ) . $this->msg( 'centralauth-admin-multi-botcheck' );
+ $botField = Xml::check( 'markasbot' ) . $this->msg( 'centralauth-admin-multi-botcheck' )->parse();
$form .= Xml::buildForm(
array(
@@ -333,12 +333,12 @@ class SpecialMultiLock extends SpecialPage {
$guHidden = $sca->formatHiddenLevel( $globalUser->getHiddenLevel() );
$accountAge = time() - wfTimestamp( TS_UNIX, $globalUser->getRegistration() );
$guRegister = $sca->prettyTimespan( $accountAge );
- $guLocked = $this->msg('centralauth-admin-status-locked-no')->escaped();
+ $guLocked = $this->msg('centralauth-admin-status-locked-no')->text();
if ( $globalUser->isLocked() ) {
- $guLocked = $this->msg('centralauth-admin-status-locked-yes')->escaped();
+ $guLocked = $this->msg('centralauth-admin-status-locked-yes')->text();
}
- $guEditCount = htmlspecialchars( $this->getLanguage()->formatNum( $globalUser->getGlobalEditCount() ) );
- $guAttachedLocalAccounts = htmlspecialchars( $this->getLanguage()->formatNum( count( $globalUser->listAttached() ) ) );
+ $guEditCount = $this->getLanguage()->formatNum( $globalUser->getGlobalEditCount() );
+ $guAttachedLocalAccounts = $this->getLanguage()->formatNum( count( $globalUser->listAttached() ) );
$rowHtml .= Html::rawElement( 'td', array(),
Html::input(
'wpActionTarget['.$guName.']',
diff --git a/includes/specials/SpecialUsersWhoWillBeRenamed.php b/includes/specials/SpecialUsersWhoWillBeRenamed.php
index e1a206f..30009fb 100644
--- a/includes/specials/SpecialUsersWhoWillBeRenamed.php
+++ b/includes/specials/SpecialUsersWhoWillBeRenamed.php
@@ -133,7 +133,7 @@ class UsersWhoWillBeRenamedPager extends TablePager {
}
break;
case 'user_editcount':
- $formatted = $this->getLanguage()->formatNum( $user->getEditCount() );
+ $formatted = htmlspecialchars( $this->getLanguage()->formatNum( $user->getEditCount() ) );
break;
}
return $formatted;
@@ -162,8 +162,8 @@ class UsersWhoWillBeRenamedPager extends TablePager {
if ( $this->mFieldNames === null ) {
$this->mFieldNames = array(
'utr_name' => $this->msg( 'centralauth-uwbr-name' )->text(),
- 'user_registration' => $this->msg( 'centralauth-uwbr-registration' ),
- 'user_editcount' => $this->msg( 'centralauth-uwbr-editcount' ),
+ 'user_registration' => $this->msg( 'centralauth-uwbr-registration' )->text(),
+ 'user_editcount' => $this->msg( 'centralauth-uwbr-editcount' )->text(),
);
}
return $this->mFieldNames;
diff --git a/includes/specials/SpecialWikiSets.php b/includes/specials/SpecialWikiSets.php
index 78090bd..d6fa74e 100644
--- a/includes/specials/SpecialWikiSets.php
+++ b/includes/specials/SpecialWikiSets.php
@@ -28,13 +28,16 @@ class SpecialWikiSets extends SpecialPage {
function execute( $subpage ) {
$this->mCanEdit = $this->getUser()->isAllowed( 'globalgrouppermissions' );
+ $req = $this->getRequest();
+ $tokenOk = $req->wasPosted()
+ && $this->getUser()->matchEditToken( $req->getVal( 'wpEditToken' ) );
$this->setHeaders();
if ( strpos( $subpage, 'delete/' ) === 0 && $this->mCanEdit ) {
$subpage = substr( $subpage, 7 ); // Remove delete/ part
if ( is_numeric( $subpage ) ) {
- if ( $this->getUser()->matchEditToken( $this->getRequest()->getVal( 'wpEditToken' ) ) ) {
+ if ( $tokenOk ) {
$this->doDelete( $subpage );
} else {
$this->buildDeleteView( $subpage );
@@ -56,7 +59,7 @@ class SpecialWikiSets extends SpecialPage {
}
}
- if ( ( $subpage || $newPage ) && $this->mCanEdit && $this->getUser()->matchEditToken( $this->getRequest()->getVal( 'wpEditToken' ) ) ) {
+ if ( ( $subpage || $newPage ) && $this->mCanEdit && $tokenOk ) {
$this->doSubmit( $subpage );
} elseif ( ( $subpage || $newPage ) && is_numeric( $subpage ) ) {
$this->buildSetView( $subpage );
@@ -67,12 +70,12 @@ class SpecialWikiSets extends SpecialPage {
}
/**
- * @param string $msg
+ * @param string $msg Output directly as HTML. Caller must escape.
*/
function buildMainView( $msg = '' ) {
// Give grep a chance to find the usages: centralauth-editset-legend-rw, centralauth-editset-legend-ro
$msgPostfix = $this->mCanEdit ? 'rw' : 'ro';
- $legend = $this->msg( "centralauth-editset-legend-{$msgPostfix}" )->text();
+ $legend = $this->msg( "centralauth-editset-legend-{$msgPostfix}" )->escaped();
$this->getOutput()->addHTML( "<fieldset><legend>{$legend}</legend>" );
if ( $msg )
$this->getOutput()->addHTML( $msg );
@@ -100,12 +103,12 @@ class SpecialWikiSets extends SpecialPage {
}
/**
- * @param $subpage
- * @param $error bool
- * @param $name
- * @param $type
- * @param $wikis
- * @param $reason
+ * @param $subpage integer ID of WikiSet
+ * @param $error bool|string False or raw html to output as error
+ * @param $name String|null (Optional) Name of WikiSet
+ * @param $type String|null WikiSet::OPTIN or WikiSet::OPTOUT
+ * @param $wikis Array|null
+ * @param $reason String
*/
function buildSetView( $subpage, $error = false, $name = null, $type = null, $wikis = null, $reason = null ) {
global $wgLocalDatabases;
@@ -168,7 +171,12 @@ class SpecialWikiSets extends SpecialPage {
if ( $error ) {
$this->getOutput()->addHTML( "<strong class='error'>{$error}</strong>" );
}
- $this->getOutput()->addHTML( "<form action='{$url}' method='post'>" );
+ $this->getOutput()->addHTML(
+ Html::openElement(
+ 'form',
+ [ 'action' => $url, 'method' => 'POST' ]
+ )
+ );
$form = array();
$form['centralauth-editset-name'] = Xml::input( 'wpName', false, $name );
@@ -190,7 +198,7 @@ class SpecialWikiSets extends SpecialPage {
$form = array();
$form['centralauth-editset-name'] = htmlspecialchars( $name );
$form['centralauth-editset-usage'] = $usage;
- $form['centralauth-editset-type'] = $this->msg( "centralauth-editset-{$type}" )->text();
+ $form['centralauth-editset-type'] = $this->msg( "centralauth-editset-{$type}" )->escaped();
$form['centralauth-editset-wikis'] = self::buildTableByList( $sortedWikis, 3, array( 'width' => '100%' ) );
$form['centralauth-editset-restwikis'] = self::buildTableByList( $restWikis, 3, array( 'width' => '100%' ) );
@@ -275,7 +283,7 @@ class SpecialWikiSets extends SpecialPage {
$url = htmlspecialchars( SpecialPage::getTitleFor( 'WikiSets', "delete/{$subpage}" )->getLocalUrl() );
$edittoken = Html::hidden( 'wpEditToken', $this->getUser()->getEditToken() );
- $this->getOutput()->addHTML( "<fieldset><legend>{$legend}</legend><form action='{$url}' method='post'>" );
+ $this->getOutput()->addHTML( "<fieldset><legend>{$legend}</legend><form action=\"{$url}\" method='post'>" );
$this->getOutput()->addHTML( Xml::buildForm( $form, 'centralauth-editset-submit-delete' ) );
$this->getOutput()->addHTML( "<p>{$edittoken}</p></form></fieldset>" );
}
--
1.9.5 (Apple Git-50.3)
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
4017223
Default Alt Text
T134931-WIP.patch (17 KB)
Attached To
Mode
T134931: Security review of GlobalGroupPermissions
Attached
Detach File
Event Timeline
Log In to Comment