Page MenuHomePhabricator

CVE-2024-23177: WatchAnalytics: classic XSS on Special:PageStatistics with the 'page' URL parameter
Closed, ResolvedPublicSecurity

Description

Minimal test example: Special:PageStatistics?page=<script>alert('XSS')</script>

Expected results: no alert() is shown

Actual results: the alert() is, in fact, shown

Mitigation: wrapping the $requestedPage variable in htmlspecialchars( ..., ENT_QUOTES ) before outputting it, like so:

@@ -54,7 +59,8 @@ class SpecialPageStatistics extends SpecialPage {
                        $this->renderPageStats();
                } elseif ( $requestedPage ) {
                        // @todo FIXME: internationalize
-                       $out->addHTML( "<p>\"$requestedPage\" is either not a page or is not watchable</p>" );
+                       $safeRequestedPage = htmlspecialchars( $requestedPage, ENT_QUOTES );
+                       $out->addHTML( "<p>\"$safeRequestedPage\" is either not a page or is not watchable</p>" );
                } else {
                        $out->addHTML( "<p>No page requested</p>" );
                }

(Excuse the line numbers presumably not matching as I have a lot of changes to this extension locally which I've yet to commit.)

Additionally there are various i18n issues here (and also many other places) and an alternative, even better fix would be to move the i18n string(s) into the i18n file and use $out->addWikiMsg( 'some-message-key-you-would-create', $requestedPage ); instead of $out->addHTML( ... ); to output a definitely known to be safe string without having to manually rely on htmlspecialchars() and the like and having the Parser do the sanitization etc. for you.
But I think the i18n stuff is a separate issue that can and should be fixed separately from the security issue.

Event Timeline

ashley claimed this task.
ashley added a subscriber: sbassett.

Now fixed in git master by the afore-referenced commit. @sbassett, feel free to make this task public now; thanks!

sbassett triaged this task as Medium priority.Oct 17 2023, 3:40 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Medium.

Change 991333 had a related patch set uploaded (by Mmartorana; author: Jack Phoenix):

[mediawiki/extensions/WatchAnalytics@REL1_41] [SECURITY] Escape the 'page' URL parameter value before outputting it

https://gerrit.wikimedia.org/r/991333

Change 991334 had a related patch set uploaded (by Mmartorana; author: Jack Phoenix):

[mediawiki/extensions/WatchAnalytics@REL1_40] [SECURITY] Escape the 'page' URL parameter value before outputting it

https://gerrit.wikimedia.org/r/991334

mmartorana renamed this task from WatchAnalytics: classic XSS on Special:PageStatistics with the 'page' URL parameter to CVE-2024-23177: WatchAnalytics: classic XSS on Special:PageStatistics with the 'page' URL parameter.Jan 17 2024, 4:17 PM

Change 991333 merged by jenkins-bot:

[mediawiki/extensions/WatchAnalytics@REL1_41] [SECURITY] Escape the 'page' URL parameter value before outputting it

https://gerrit.wikimedia.org/r/991333

Change 991334 merged by SBassett:

[mediawiki/extensions/WatchAnalytics@REL1_40] [SECURITY] Escape the 'page' URL parameter value before outputting it

https://gerrit.wikimedia.org/r/991334