From 8fd6f769bcbaf8a9338f600a4ea4a3d5ccd37325 Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Fri, 29 Sep 2023 15:57:12 +0100 Subject: [PATCH] SECURITY: Address many XSS vectors via message definitions Why: * Several messages in the CheckUser extension allow users without editsitecss or editsitejs to add CSS and/or JS code that is viewable by users using the CheckUser interfaces. * These interfaces should not allow CSS and/or JS code to be injected in this way and should properly escape HTML unless the use of HTML code is required. What: * Define a few SpecialCheckUserLog result line messages as raw HTML messages by adding them to wgRawHtmlMessages as they contain bidirectional isolation HTML elements. * Update the code in Special:CheckUserLog to pass the parameters as raw parameters and also use ::escaped over ::text if the message key is not in wgRawHtmlMessages. * Update SpecialCheckUserLog to always escape the 'parentheses' message. * Update CheckUserGetUsersPager to escape the 'checkuser-massblock-text' message. * Change ApiQueryCheckUser to escape the 'checkuser-reason-api' message. * Update SpecialInvestigate to escape the Language::userDate output. Bug: T347708 Change-Id: If3ce02cac9c5f2a6f84c42d902b8290eb1fa7250 --- extension.json | 6 +++ src/Api/ApiQueryCheckUser.php | 2 +- .../Pagers/CheckUserGetUsersPager.php | 2 +- src/CheckUser/Pagers/CheckUserLogPager.php | 53 +++++++++++-------- src/Investigate/Pagers/TimelinePager.php | 4 +- 5 files changed, 42 insertions(+), 25 deletions(-) diff --git a/extension.json b/extension.json index 32b85f76..ac7ffeb6 100644 --- a/extension.json +++ b/extension.json @@ -524,6 +524,12 @@ ] } }, + "RawHtmlMessages": [ + "checkuser-log-entry-ipedits", + "checkuser-log-entry-ipusers", + "checkuser-log-entry-ipedits-xff", + "checkuser-log-entry-ipusers-xff" + ], "JobClasses": { "checkuserLogTemporaryAccountAccess": "\\MediaWiki\\CheckUser\\Jobs\\LogTemporaryAccountAccessJob", "checkuserPruneCheckUserDataJob": "\\MediaWiki\\CheckUser\\Jobs\\PruneCheckUserDataJob" diff --git a/src/Api/ApiQueryCheckUser.php b/src/Api/ApiQueryCheckUser.php index 7d578177..86f4c627 100644 --- a/src/Api/ApiQueryCheckUser.php +++ b/src/Api/ApiQueryCheckUser.php @@ -74,7 +74,7 @@ class ApiQueryCheckUser extends ApiQueryBase { $this->dieWithError( 'apierror-checkuser-missingsummary', 'missingdata' ); } - $reason = $this->msg( 'checkuser-reason-api', $reason )->inContentLanguage()->text(); + $reason = $this->msg( 'checkuser-reason-api', $reason )->inContentLanguage()->escaped(); // absolute time $timeCutoff = strtotime( $timecond ); if ( !$timeCutoff || $timeCutoff < 0 || $timeCutoff > time() ) { diff --git a/src/CheckUser/Pagers/CheckUserGetUsersPager.php b/src/CheckUser/Pagers/CheckUserGetUsersPager.php index 01077c50..3adfa3b3 100644 --- a/src/CheckUser/Pagers/CheckUserGetUsersPager.php +++ b/src/CheckUser/Pagers/CheckUserGetUsersPager.php @@ -654,7 +654,7 @@ class CheckUserGetUsersPager extends AbstractCheckUserPager { ->setSubmitTextMsg( 'checkuser-massblock-commit' ) ->setSubmitId( 'checkuserblocksubmit' ) ->setSubmitName( 'checkuserblock' ) - ->setHeaderHtml( $this->msg( 'checkuser-massblock-text' )->text() ); + ->setHeaderHtml( $this->msg( 'checkuser-massblock-text' )->escaped() ); if ( $config->get( 'BlockAllowsUTEdit' ) ) { $fieldset->addFields( [ diff --git a/src/CheckUser/Pagers/CheckUserLogPager.php b/src/CheckUser/Pagers/CheckUserLogPager.php index c08ca294..d89caeab 100644 --- a/src/CheckUser/Pagers/CheckUserLogPager.php +++ b/src/CheckUser/Pagers/CheckUserLogPager.php @@ -151,14 +151,14 @@ class CheckUserLogPager extends RangeChronologicalPager { } $user .= $this->msg( 'word-separator' )->escaped() . Html::rawElement( 'span', [ 'classes' => 'mw-usertoollinks' ], - $this->msg( 'parentheses' )->params( $this->getLinkRenderer()->makeLink( + $this->msg( 'parentheses' )->rawParams( $this->getLinkRenderer()->makeLink( SpecialPage::getTitleFor( 'CheckUserLog' ), $this->msg( 'checkuser-log-checks-by' )->text(), [], [ 'cuInitiator' => $row->actor_name, ] - ) )->text() + ) )->escaped() ); } @@ -192,26 +192,37 @@ class CheckUserLogPager extends RangeChronologicalPager { // Give grep a chance to find the usages: // checkuser-log-entry-userips, checkuser-log-entry-ipedits, // checkuser-log-entry-ipusers, checkuser-log-entry-ipedits-xff - // checkuser-log-entry-ipusers-xff, checkuser-log-entry-useredits - $rowContent = $this->msg( - 'checkuser-log-entry-' . $row->cul_type, - $user, - $target, - $this->generateTimestampLink( - $lang->userTimeAndDate( - wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser + // checkuser-log-entry-ipusers-xff, checkuser-log-entry-useredits, + // checkuser-log-entry-investigate + $messageKey = 'checkuser-log-entry-' . $row->cul_type; + $message = $this->msg( $messageKey ) + ->rawParams( + $user, + $target, + $this->generateTimestampLink( + $lang->userTimeAndDate( + wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser + ), + $row ), - $row - ), - $this->generateTimestampLink( - $lang->userDate( wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser ), - $row - ), - $this->generateTimestampLink( - $lang->userTime( wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser ), - $row - ) - )->text(); + $this->generateTimestampLink( + $lang->userDate( wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser ), + $row + ), + $this->generateTimestampLink( + $lang->userTime( wfTimestamp( TS_MW, $row->cul_timestamp ), $contextUser ), + $row + ) + ); + if ( in_array( $messageKey, $this->getConfig()->get( 'RawHtmlMessages' ) ) ) { + # Allow HTML in log entry messages in wgRawHtmlMessages + $rowContent = $message->text(); + } else { + # Disallow HTML in log entry messages not in wgRawHtmlMessages + # as users without editsitejs or editsitecss could add HTML to + # this message otherwise. + $rowContent = $message->escaped(); + } $rowContent .= $this->commentFormatter->formatBlock( $this->commentStore->getComment( 'cul_reason', $row )->text ); diff --git a/src/Investigate/Pagers/TimelinePager.php b/src/Investigate/Pagers/TimelinePager.php index 345de6c2..aa05b148 100644 --- a/src/Investigate/Pagers/TimelinePager.php +++ b/src/Investigate/Pagers/TimelinePager.php @@ -109,14 +109,14 @@ class TimelinePager extends ReverseChronologicalPager { $dateHeader = $this->getLanguage()->userDate( wfTimestamp( TS_MW, $row->cuc_timestamp ), $this->getUser() ); if ( $this->lastDateHeader === null ) { $this->lastDateHeader = $dateHeader; - $line .= Html::rawElement( 'h4', [], $dateHeader ); + $line .= Html::element( 'h4', [], $dateHeader ); $line .= Html::openElement( 'ul' ); } elseif ( $this->lastDateHeader !== $dateHeader ) { $this->lastDateHeader = $dateHeader; // Start a new list with a new date header $line .= Html::closeElement( 'ul' ); - $line .= Html::rawElement( 'h4', [], $dateHeader ); + $line .= Html::element( 'h4', [], $dateHeader ); $line .= Html::openElement( 'ul' ); } -- 2.25.1