I've found several XSSes in Special:RefreshSpecial while conducting a security review of the extension for a request on the Miraheze issue tracker. One is shown below:
I'm too lazy to list them all, so I'll just submit a patch that fixes them all.
| BlankEclair | |
| Nov 3 2024, 12:53 AM |
| F57672515: T378885.patch | |
| Nov 3 2024, 2:26 AM |
| F57672360: T378885.patch | |
| Nov 3 2024, 1:14 AM |
| F57672303: 2024-11-03_11-48.png | |
| Nov 3 2024, 12:53 AM |
I've found several XSSes in Special:RefreshSpecial while conducting a security review of the extension for a request on the Miraheze issue tracker. One is shown below:
I'm too lazy to list them all, so I'll just submit a patch that fixes them all.
Nice catches, thank you for this! 💯 👍
Some quick thoughts before we go ahead with getting this merged...
diff --git a/src/RefreshSpecial.php b/src/RefreshSpecial.php index 312d649..eb2989c 100644 --- a/src/RefreshSpecial.php +++ b/src/RefreshSpecial.php @@ -58,7 +58,7 @@ class RefreshSpecial extends SpecialPage { ini_set( 'memory_limit', '512M' ); set_time_limit( 240 ); - $out->setPageTitle( $this->msg( 'refreshspecial-title' )->plain() ); + $out->setPageTitle( $this->msg( 'refreshspecial-title' )->escaped() ); $cSF = new RefreshSpecialForm(); $cSF->setContext( $this->getContext() );
Is this actually exploitable? AIUI for the longest time setPageTitle was a nice catch-all method that happily, and safely, accepted whatever garbage you threw at it. You could pass in a string, a Message object, just about anything and it'd work as you'd expect it to. Not sure how it's in MW 1.4x but in 1.39 you can have whatever alerts and such passed to setPageTitle and they won't run.
diff --git a/src/RefreshSpecialForm.php b/src/RefreshSpecialForm.php index ce9fd16..8879059 100644 @@ -1,5 +1,6 @@ <?php +use MediaWiki\Html\Html; use MediaWiki\MediaWikiServices; /**
The problem with this is that this de facto raises the minimum MediaWiki version from the stated 1.32 (sic) to 1.40. I'm okay with sorta crude backwards compatibility hacks to work around the namespace issue, but can we please keep 1.39 support in as it's the current LTS release? (If not, the MW requirement in extension.json would need to be adjusted accordingly.)
@@ -23,7 +24,9 @@ class RefreshSpecialForm extends ContextSource { if ( $err != '' ) { $out->setSubtitle( $this->msg( 'formerror' )->escaped() ); - $out->addHTML( "<p class='error'>{$err}</p>\n" ); + $out->addHTML( Html::element( 'p', [ + 'class' => 'error', + ], $err ) . "\n" ); } $out->addWikiMsg( 'refreshspecial-help' );
Though not immediately obvious, this should fix the case of the method being passed the refreshspecial-fail msg from RefreshSpecial.php. Nice!
--- a/src/RefreshSpecialForm.php +++ b/src/RefreshSpecialForm.php @@ -141,7 +144,7 @@ class RefreshSpecialForm extends ContextSource { } if ( !( isset( $options['only'] ) ) || ( $options['only'] == $queryPage->getName() ) ) { - $out->addHTML( "<b>$special</b>: " ); + $out->addHTML( Html::element( 'b', [], $special ) . ': ' ); if ( $queryPage->isExpensive() ) { $t1 = microtime( true );
Correct me if I'm mistaken here, but this doesn't seem to be an XSS issue; while it may look like one and your code is a definite improvement in code quality over the current code in master, $special comes basically from QueryPage::getPages() and thus isn't "user-controllable" in the sense that an on-wiki person, even an admin, could exploit it. (And if an attacker has SSH access to your server, this isn't likely your #1 concern anyway...)
However, just a few lines above this patched line, is this line:
$out->addWikiTextAsInterface( $this->msg( 'refreshspecial-no-page' )->plain() . ": $special\n" );
While the $special usage may indeed seem dodgy there, the same is true for plain() and yet it seems to be fine. How very particular.
@@ -228,7 +231,7 @@ class RefreshSpecialForm extends ContextSource { $this->getOutput()->setSubtitle( $this->msg( 'refreshspecial-choice', $this->msg( 'refreshspecial-refreshing' )->plain() - )->plain() + )->escaped() ); $this->refreshSpecial();
I tested this and it does work as it should. It's just confusing, but that's not on you, that's (IMO) on MW core. Regardless, a fine reminder that though often plain() is a code smell, sometimes it can be OK even if it still looks dodgy, and this is one of those cases.
For the curious: blame me for the XSS (sorry!), I introduced it in version 1.4.0 (5a1dcd157cbe6a6aecdcd0877be395e015401436) over a decade ago. Aren't rewrites just fun?
diff --git a/src/RefreshSpecial.php b/src/RefreshSpecial.php index 312d649..eb2989c 100644 --- a/src/RefreshSpecial.php +++ b/src/RefreshSpecial.php @@ -58,7 +58,7 @@ class RefreshSpecial extends SpecialPage { ini_set( 'memory_limit', '512M' ); set_time_limit( 240 ); - $out->setPageTitle( $this->msg( 'refreshspecial-title' )->plain() ); + $out->setPageTitle( $this->msg( 'refreshspecial-title' )->escaped() ); $cSF = new RefreshSpecialForm(); $cSF->setContext( $this->getContext() );Is this actually exploitable? AIUI for the longest time setPageTitle was a nice catch-all method that happily, and safely, accepted whatever garbage you threw at it. You could pass in a string, a Message object, just about anything and it'd work as you'd expect it to. Not sure how it's in MW 1.4x but in 1.39 you can have whatever alerts and such passed to setPageTitle and they won't run.
It gets sanitized, which technically (appears to) stop <script>s and the like, but I wouldn't want to chance it.
https://doc.wikimedia.org/mediawiki-core/master/php/OutputPage_8php_source.html#l01221
diff --git a/src/RefreshSpecialForm.php b/src/RefreshSpecialForm.php index ce9fd16..8879059 100644 @@ -1,5 +1,6 @@ <?php +use MediaWiki\Html\Html; use MediaWiki\MediaWikiServices; /**The problem with this is that this de facto raises the minimum MediaWiki version from the stated 1.32 (sic) to 1.40. I'm okay with sorta crude backwards compatibility hacks to work around the namespace issue, but can we please keep 1.39 support in as it's the current LTS release? (If not, the MW requirement in extension.json would need to be adjusted accordingly.)
Whoops, didn't know that it was renamed from 1.32 to 1.42, oops. It appears that there's at least an Html class though: https://doc.wikimedia.org/mediawiki-core/REL1_32/php/classHtml.html
--- a/src/RefreshSpecialForm.php +++ b/src/RefreshSpecialForm.php @@ -141,7 +144,7 @@ class RefreshSpecialForm extends ContextSource { } if ( !( isset( $options['only'] ) ) || ( $options['only'] == $queryPage->getName() ) ) { - $out->addHTML( "<b>$special</b>: " ); + $out->addHTML( Html::element( 'b', [], $special ) . ': ' ); if ( $queryPage->isExpensive() ) { $t1 = microtime( true );Correct me if I'm mistaken here, but this doesn't seem to be an XSS issue; while it may look like one and your code is a definite improvement in code quality over the current code in master, $special comes basically from QueryPage::getPages() and thus isn't "user-controllable" in the sense that an on-wiki person, even an admin, could exploit it. (And if an attacker has SSH access to your server, this isn't likely your #1 concern anyway...)
Yeah, I couldn't immediately think of a way to exploit it, but it's a risk, so why not eliminate it? :p
(Also, mediawikiwiki says to escape everything: https://www.mediawiki.org/wiki/XSS#Stopping_Cross-site_scripting)
However, just a few lines above this patched line, is this line:
$out->addWikiTextAsInterface( $this->msg( 'refreshspecial-no-page' )->plain() . ": $special\n" );While the $special usage may indeed seem dodgy there, the same is true for plain() and yet it seems to be fine. How very particular.
I assume that the input is treated as wikitext and not HTML as per the function name and https://doc.wikimedia.org/mediawiki-core/REL1_32/php/classOutputPage.html#ae442f4b110eb8e56921459881a8e81c9
@@ -228,7 +231,7 @@ class RefreshSpecialForm extends ContextSource { $this->getOutput()->setSubtitle( $this->msg( 'refreshspecial-choice', $this->msg( 'refreshspecial-refreshing' )->plain() - )->plain() + )->escaped() ); $this->refreshSpecial();I tested this and it does work as it should. It's just confusing, but that's not on you, that's (IMO) on MW core. Regardless, a fine reminder that though often plain() is a code smell, sometimes it can be OK even if it still looks dodgy, and this is one of those cases.
Gotta check docs :p
https://doc.wikimedia.org/mediawiki-core/REL1_32/php/classOutputPage.html#af0773075842766e0e288dc76f467219e
For the curious: blame me for the XSS (sorry!), I introduced it in version 1.4.0 (5a1dcd157cbe6a6aecdcd0877be395e015401436) over a decade ago. Aren't rewrites just fun?
Heh, I'm just glad that this time it's "only" 11 XSSes, not 18 like I've found in another extension.
Since this isn't Wikimedia-deployed, I think we'd recommend just pushing this through gerrit for review (assuming it's patched at Miraheze) and then we can include it with the next supplemental security release.