Page MenuHomePhabricator

CI reports possible XSS vulnerability in SecurePoll
Closed, ResolvedPublicBUG REPORT

Description

CI output: https://integration.wikimedia.org/ci/job/mwext-php74-phan/18524/console

Relevant portion:

15:41:58 includes/Pages/CreatePage.php:123 SecurityCheck-XSS Calling method \MediaWiki\Linker\Linker::makeExternalLink() in \MediaWiki\Extension\SecurePoll\Pages\CreatePage::execute that outputs using tainted argument #2 (`$wiki`). (Caused by: ../../includes/linker/Linker.php +1150; annotations in \HtmlArmor::__construct) (Caused by: includes/Pages/CreatePage.php +118; annotations in \MediaWiki\Message\Message::text)
15:41:58 includes/Pages/TranslatePage.php:101 SecurityCheck-XSS Calling method \MediaWiki\Linker\Linker::makeExternalLink() in \MediaWiki\Extension\SecurePoll\Pages\TranslatePage::execute that outputs using tainted argument #2 (`$wiki`). (Caused by: ../../includes/linker/Linker.php +1150; annotations in \HtmlArmor::__construct) (Caused by: includes/Pages/TranslatePage.php +96; annotations in \MediaWiki\Message\Message::text)
15:41:58 includes/Pages/VoterEligibilityPage.php:127 SecurityCheck-XSS Calling method \MediaWiki\Linker\Linker::makeExternalLink() in \MediaWiki\Extension\SecurePoll\Pages\VoterEligibilityPage::execute that outputs using tainted argument #2 (`$wiki`). (Caused by: ../../includes/linker/Linker.php +1150; annotations in \HtmlArmor::__construct) (Caused by: includes/Pages/VoterEligibilityPage.php +122; annotations in \MediaWiki\Message\Message::text)

Relevant part of the code:

...
				$wiki = $this->election->getProperty( 'main-wiki' );
				if ( $wiki ) {
					$wiki = WikiMap::getWikiName( $wiki );
				} else {
					$wiki = $this->msg( 'securepoll-edit-redirect-otherwiki' )->text();
				}

				$out->addWikiMsg(
					'securepoll-edit-redirect',
					Message::rawParam( Linker::makeExternalLink( $jumpUrl, $wiki ) )
				);
...

Event Timeline

I suspect CI is hitting a false positive here. The $wiki variable is effectively sanitized by the if/else statement above. That said, I'm not sure what the best path forward is.

One option is suppress the CI check for this line. I don't like it because if the "safeguard" in the lines above is modified, then the CI will miss an actual potential XSS vulnerability.

Another option is to modify the test so it can validate that $wiki value is legit but I haven't fully figured that out.

Adding @DannyS712 because the change in question is stuck despite his +2

Reedy renamed this task from CI reports possible XSS vulnarability in Securepoll to CI reports possible XSS vulnerability in SecurePoll.Mon, Jun 10, 10:16 PM

Change #1041252 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Fix Linker::makeExternalLink build failures

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

Change #1041297 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.43.0-wmf.9] Fix Linker::makeExternalLink build failures

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

Change #1041252 merged by jenkins-bot:

[mediawiki/core@master] Fix Linker::makeExternalLink build failures

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

Change #1041297 merged by jenkins-bot:

[mediawiki/core@wmf/1.43.0-wmf.9] Fix Linker::makeExternalLink build failures

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

Mentioned in SAL (#wikimedia-operations) [2024-06-11T21:44:09Z] <ladsgroup@deploy1002> Started scap: Backport for [[gerrit:1041297|Fix Linker::makeExternalLink build failures (T367127)]]

Mentioned in SAL (#wikimedia-operations) [2024-06-11T21:47:33Z] <ladsgroup@deploy1002> matmarex, ladsgroup: Backport for [[gerrit:1041297|Fix Linker::makeExternalLink build failures (T367127)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-06-11T21:56:43Z] <ladsgroup@deploy1002> Finished scap: Backport for [[gerrit:1041297|Fix Linker::makeExternalLink build failures (T367127)]] (duration: 12m 33s)