From 264cdbb199eb1131107d4b7c387fa831dcc60e65 Mon Sep 17 00:00:00 2001 From: BlankEclair Date: Thu, 14 Nov 2024 00:15:16 +1100 Subject: [PATCH] SECURITY: Fix various security vulnerabilities Bug: T379749 --- includes/DT_ImportJob.php | 5 +++ includes/DT_Utils.php | 8 +++-- includes/DT_XMLParser.php | 3 +- includes/specials/DT_ImportCSV.php | 40 +++++++++++++++++----- includes/specials/DT_ImportSpreadsheet.php | 1 + includes/specials/DT_ImportXML.php | 34 +++++++++++++++--- includes/specials/DT_ViewXML.php | 12 ++++--- 7 files changed, 81 insertions(+), 22 deletions(-) diff --git a/includes/DT_ImportJob.php b/includes/DT_ImportJob.php index 91ed2d1..6178a70 100644 --- a/includes/DT_ImportJob.php +++ b/includes/DT_ImportJob.php @@ -49,6 +49,11 @@ class DTImportJob extends Job { } $user = MediaWikiServices::getInstance()->getUserFactory()->newFromId( $this->params['user_id'] ); + // Do not let blocked users import pages (in case of TOCTOU) + if ( !$user->definitelyCan( 'edit', $this->title ) ) { + return true; + } + $text = $this->params['text']; if ( $this->title->exists() ) { if ( $for_pages_that_exist == 'append' ) { diff --git a/includes/DT_Utils.php b/includes/DT_Utils.php index 670f972..26e68cf 100644 --- a/includes/DT_Utils.php +++ b/includes/DT_Utils.php @@ -30,7 +30,7 @@ class DTUtils { $radioButtonAttrs['checked'] = true; } $text = "\t" . Html::input( 'pagesThatExist', $option, 'radio', $radioButtonAttrs ) . - "\t" . wfMessage( $optionMsg )->text() . "
" . "\n"; + "\t" . wfMessage( $optionMsg )->escaped() . "
" . "\n"; $text = Html::rawElement( 'label', null, $text ) . "\n"; return $text; } @@ -58,10 +58,14 @@ class DTUtils { ] ) . "\n"; return "\t" . Html::rawElement( 'p', null, - wfMessage( 'dt_import_summarydesc' )->text() . "\n" . + wfMessage( 'dt_import_summarydesc' )->escaped() . "\n" . $importSummaryText ) . "\n"; } + static function printEditTokenInput( $csrfTokenSet ) { + return Html::hidden( 'wpEditToken', $csrfTokenSet->getToken() ); + } + static function printSubmitButton( $buttonMsg = null ) { if ( $buttonMsg == null ) { $buttonMsg = 'import-interwiki-submit'; diff --git a/includes/DT_XMLParser.php b/includes/DT_XMLParser.php index fd47f21..1a84e48 100644 --- a/includes/DT_XMLParser.php +++ b/includes/DT_XMLParser.php @@ -20,6 +20,7 @@ class DTXMLParser { } function debug( $text ) { + // $text = htmlspecialchars( $text, ENT_QUOTES ); // print "$text. "; } @@ -57,7 +58,7 @@ class DTXMLParser { // $this->debug( "in_start $name" ); $pages_str = str_replace( ' ', '_', wfMessage( 'dt_xml_pages' )->inContentLanguage()->text() ); if ( $name != $pages_str ) { - print( "Expected '$pages_str', got '$name'" ); + $this->throwXMLerror( "Expected '$pages_str', got '$name'" ); } xml_set_element_handler( $parser, "in_pages", "out_pages" ); } diff --git a/includes/specials/DT_ImportCSV.php b/includes/specials/DT_ImportCSV.php index 1823980..66caeb7 100644 --- a/includes/specials/DT_ImportCSV.php +++ b/includes/specials/DT_ImportCSV.php @@ -1,6 +1,7 @@ setHeaders(); $out = $this->getOutput(); - $out->enableOOUI(); - $out->addModuleStyles( 'ext.datatransfer' ); - if ( !$this->getUser()->isAllowed( 'datatransferimport' ) ) { - throw new PermissionsError( 'datatransferimport' ); + // Double check if the user has proper permissions (this prevents using the special page + // if the user is blocked, which is not done by default for some reason) + $status = PermissionStatus::newEmpty(); + $this->getAuthority()->isDefinitelyAllowed( 'datatransferimport', $status ); + if ( !$status->isGood() ) { + $out->prepareErrorPage(); + $out->setPageTitleMsg( $this->msg( 'permissionserrors' ) ); + $out->addWikiTextAsInterface( Html::errorBox( + $out->formatPermissionStatus( $status, 'datatransferimport' ) + ) ); + return; } - if ( $this->getRequest()->getCheck( 'import_file' ) ) { + $out->enableOOUI(); + $out->addModuleStyles( 'ext.datatransfer' ); + + if ( $this->getRequest()->wasPosted() && $this->getRequest()->getCheck( 'import_file' ) ) { $text = $this->importFromUploadAndModifyPages(); } else { $text = $this->printForm(); @@ -41,6 +52,16 @@ class DTImportCSV extends SpecialPage { } protected function importFromUploadAndModifyPages() { + $editToken = $this->getRequest()->getVal( 'wpEditToken' ); + if ( !$this->getContext()->getCsrfTokenSet()->matchToken( $editToken ) ) { + // Ideally, we should output a prefilled form with a new edit token + // ready to go for more convenient resubmitting. However, I do not + // want to touch this code more than I already do, and honestly the + // proper solution is to rewrite the form to properly use OOUI or + // HTMLForm instead. I'm not spearheading that effort though. + return $this->msg( 'import-token-mismatch' )->parse(); + } + $text = DTUtils::printImportingMessage(); $uploadResult = ImportStreamSource::newFromUpload( "file_name" ); @@ -57,7 +78,7 @@ class DTImportCSV extends SpecialPage { $error_msg = $this->importFromFile( $source, $encoding, $pages ); if ( $error_msg !== null ) { - $text .= $error_msg; + $text .= htmlspecialchars( $error_msg, ENT_QUOTES ); return $text; } @@ -84,11 +105,12 @@ class DTImportCSV extends SpecialPage { $formText .= "\t" . Html::rawElement( 'p', null, - $this->msg( 'dt_import_encodingtype', 'CSV' )->text() . " " . $encodingSelectText + $this->msg( 'dt_import_encodingtype', 'CSV' )->escaped() . " " . $encodingSelectText ) . "\n"; $formText .= "\t" . '
' . "\n"; $formText .= DTUtils::printExistingPagesHandling(); $formText .= DTUtils::printImportSummaryInput( $this->getFiletype() ); + $formText .= DTUtils::printEditTokenInput( $this->getContext()->getCsrfTokenSet() ); $formText .= DTUtils::printSubmitButton(); $text = "\t" . Html::rawElement( 'form', [ @@ -216,7 +238,7 @@ class DTImportCSV extends SpecialPage { foreach ( $pages as $page ) { $title = Title::newFromText( $page->getName() ); if ( $title === null ) { - $text .= '

' . $this->msg( 'img-auth-badtitle', $page->getName() )->text() . "

\n"; + $text .= '

' . $this->msg( 'img-auth-badtitle', $page->getName() )->escaped() . "

\n"; continue; } $jobParams['text'] = $page->createText(); diff --git a/includes/specials/DT_ImportSpreadsheet.php b/includes/specials/DT_ImportSpreadsheet.php index dcac9bb..dc75594 100644 --- a/includes/specials/DT_ImportSpreadsheet.php +++ b/includes/specials/DT_ImportSpreadsheet.php @@ -22,6 +22,7 @@ class DTImportSpreadsheet extends DTImportCSV { $formText = DTUtils::printFileSelector( $this->getFiletype() ); $formText .= DTUtils::printExistingPagesHandling(); $formText .= DTUtils::printImportSummaryInput( $this->getFiletype() ); + $formText .= DTUtils::printEditTokenInput( $this->getContext()->getCsrfTokenSet() ); $formText .= DTUtils::printSubmitButton(); $text = "\t" . Xml::tags( 'form', [ diff --git a/includes/specials/DT_ImportXML.php b/includes/specials/DT_ImportXML.php index 8682fc4..7f6aa62 100644 --- a/includes/specials/DT_ImportXML.php +++ b/includes/specials/DT_ImportXML.php @@ -1,6 +1,7 @@ setHeaders(); $out = $this->getOutput(); - $out->enableOOUI(); - if ( !$this->getUser()->isAllowed( 'datatransferimport' ) ) { - throw new PermissionsError( 'datatransferimport' ); + // Double check if the user has proper permissions (this prevents using the special page + // if the user is blocked, which is not done by default for some reason) + $status = PermissionStatus::newEmpty(); + $this->getAuthority()->isDefinitelyAllowed( 'datatransferimport', $status ); + if ( !$status->isGood() ) { + $out->prepareErrorPage(); + $out->setPageTitleMsg( $this->msg( 'permissionserrors' ) ); + $out->addWikiTextAsInterface( Html::errorBox( + $out->formatPermissionStatus( $status, 'datatransferimport' ) + ) ); + return; } + $out->enableOOUI(); + $request = $this->getRequest(); - if ( $request->getCheck( 'import_file' ) ) { + if ( $request->wasPosted() && $request->getCheck( 'import_file' ) ) { + $editToken = $request->getVal( 'wpEditToken' ); + if ( !$this->getContext()->getCsrfTokenSet()->matchToken( $editToken ) ) { + // Ideally, we should output a prefilled form with a new edit token + // ready to go for more convenient resubmitting. However, I do not + // want to touch this code more than I already do, and honestly the + // proper solution is to rewrite the form to properly use OOUI or + // HTMLForm instead. I'm not spearheading that effort though. + $text = $this->msg( 'import-token-mismatch' )->parse(); + $out->addHTML( $text ); + return; + } + $text = DTUtils::printImportingMessage(); $uploadResult = ImportStreamSource::newFromUpload( "file_name" ); $source = $uploadResult->value; @@ -42,6 +65,7 @@ class DTImportXML extends SpecialPage { $formText = DTUtils::printFileSelector( $this->msg( 'dt_filetype_xml' )->text() ); $formText .= DTUtils::printExistingPagesHandling(); $formText .= DTUtils::printImportSummaryInput( $this->msg( 'dt_filetype_xml' )->text() ); + $formText .= DTUtils::printEditTokenInput( $this->getContext()->getCsrfTokenSet() ); $formText .= DTUtils::printSubmitButton(); $text = "\t" . Xml::tags( 'form', [ diff --git a/includes/specials/DT_ViewXML.php b/includes/specials/DT_ViewXML.php index 54e8687..ed19196 100644 --- a/includes/specials/DT_ViewXML.php +++ b/includes/specials/DT_ViewXML.php @@ -252,14 +252,16 @@ class DTViewXML extends SpecialPage { } else { // Set 'title' as hidden field, in case there's no URL niceness. $mw_namespace_labels = $contLang->getNamespaces(); - $special_namespace = $mw_namespace_labels[NS_SPECIAL]; + // Hopefully no one sets the special namespace label to an XSS payload, + // but we escape it just in case anyway. + $special_namespace = htmlspecialchars( $mw_namespace_labels[NS_SPECIAL], ENT_QUOTES ); $text = << END; - $text .= "

" . $this->msg( 'dt_viewxml_docu' )->text() . "

\n"; - $text .= "

" . $this->msg( 'dt_viewxml_categories' )->text() . "

\n"; + $text .= "

" . $this->msg( 'dt_viewxml_docu' )->escaped() . "

\n"; + $text .= "

" . $this->msg( 'dt_viewxml_categories' )->escaped() . "

\n"; $categories = self::getCategoriesList(); $linkRenderer = $this->getLinkRenderer(); foreach ( $categories as $category ) { @@ -268,7 +270,7 @@ END; $link = $linkRenderer->makeKnownLink( $title, $title->getText() ); $text .= " $link
\n"; } - $text .= "

" . $this->msg( 'dt_viewxml_namespaces' )->text() . "

\n"; + $text .= "

" . $this->msg( 'dt_viewxml_namespaces' )->escaped() . "

\n"; $namespaces = self::getNamespacesList(); foreach ( $namespaces as $nsCode ) { if ( $nsCode === '0' ) { @@ -282,7 +284,7 @@ END; $text .= Html::input( "namespaces[$nsCode]", null, 'checkbox' ); $text .= ' ' . str_replace( '_', ' ', $nsName ) . "
\n"; } - $text .= "

\n"; + $text .= "

\n"; $text .= DTUtils::printSubmitButton( 'viewxml' ); $text .= "\n"; -- 2.47.0