From 0a32d992c36c30fa1baf14c85d853361d7f0020f Mon Sep 17 00:00:00 2001 From: Brian Wolff 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 ) . '
' . 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 ) . '
' . 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 { '
'; 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 ) . '
' . 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 ) . '
' . 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( "
{$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( "{$error}" ); } - $this->getOutput()->addHTML( "
" ); + $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( "
{$legend}" ); + $this->getOutput()->addHTML( "
{$legend}" ); $this->getOutput()->addHTML( Xml::buildForm( $form, 'centralauth-editset-submit-delete' ) ); $this->getOutput()->addHTML( "

{$edittoken}

" ); } -- 1.9.5 (Apple Git-50.3)