Page MenuHomePhabricator

CVE-2023-36675: XSS in BlockLogFormatter due to unsafe message use
Closed, ResolvedPublicSecurity

Description

Found with r902363 which annotates some methods as unsafe. There are some genuine vulnerabilities in BlockLogFormatter, specifically in the code for partial blocks:

BlockLogFormatter.php#110-135
$actions = $params[6]['actions'] ?? [];
$actions = array_map( function ( $actions ) {
	return $this->msg( 'ipb-action-' . $actions )->text();
}, $actions );

$restrictions = [];
if ( $pages ) {
	$restrictions[] = $this->msg( 'logentry-partialblock-block-page' )
		->numParams( count( $pages ) )
		->rawParams( $this->context->getLanguage()->listToText( $pages ) )->text();
}

if ( $namespaces ) {
	$restrictions[] = $this->msg( 'logentry-partialblock-block-ns' )
		->numParams( count( $namespaces ) )
		->rawParams( $this->context->getLanguage()->listToText( $namespaces ) )->text();
}
$enablePartialActionBlocks = $this->context->getConfig()
	->get( MainConfigNames::EnablePartialActionBlocks );
if ( $actions && $enablePartialActionBlocks ) {
	$restrictions[] = $this->msg( 'logentry-partialblock-block-action' )
		->numParams( count( $actions ) )
		->rawParams( $this->context->getLanguage()->listToText( $actions ) )->text(); // <-- Unsafe use of rawParams because $actions is built with Message::text()
}

$params[6] = Message::rawParam( $this->context->getLanguage()->listToText( $restrictions ) );// <-- Unsafe use of rawParam because $restrictons is built with Message::text()

These can be reproduced by:

  • Adding <script>alert()</script> or something like that to ipb-action-* and logentry-partialblock-block-* messages
  • Enabling action blocks with $wgEnablePartialActionBlocks = true;
  • Blocking someone selecting all options for partial blocks
  • Going to their contributions page

Event Timeline

So for:

->rawParams( $this->context->getLanguage()->listToText( $actions ) )->text(); // <-- Unsafe use of rawParams because $actions is built with Message::text()

that should be resolved with an s/text()/parse()/, no? Which we'd also want to change for the message construction under the if ( $pages ) and if ( $namespaces ) conditionals, right? Unless that would somehow lead to double-escaping. Assuming it doesn't, those mitigations should ensure that $restrictions shouldn't be tainted within:

$params[6] = Message::rawParam( $this->context->getLanguage()->listToText( $restrictions ) );

So for:

->rawParams( $this->context->getLanguage()->listToText( $actions ) )->text(); // <-- Unsafe use of rawParams because $actions is built with Message::text()

that should be resolved with an s/text()/parse()/, no?

When using the ipb-action-* messages, yes, I think so. Possibly escaped() instead of parse(), unless there's a good reason to use the latter.

Which we'd also want to change for the message construction under the if ( $pages ) and if ( $namespaces ) conditionals, right? Unless that would somehow lead to double-escaping. Assuming it doesn't, those mitigations should ensure that $restrictions shouldn't be tainted within:

$params[6] = Message::rawParam( $this->context->getLanguage()->listToText( $restrictions ) );

Yes, I think replacing text() with escaped() in all the 3 conditionals should fix this, BUT I have no idea if this will break anything. I'm not familiar with LogFormatter and I've always found it to be a huge mess, particularly for how the escaping is controlled ($this>plaintext etc.). Maybe we need to manually vary the escaping on the value of $this->plaintext. I don't really know...

mmartorana changed the task status from Open to In Progress.Mar 31 2023, 3:26 PM
mmartorana triaged this task as Medium priority.
mmartorana changed Risk Rating from N/A to Medium.

Yes, I think replacing text() with escaped() in all the 3 conditionals should fix this, BUT I have no idea if this will break anything. I'm not familiar with LogFormatter and I've always found it to be a huge mess, particularly for how the escaping is controlled ($this>plaintext etc.). Maybe we need to manually vary the escaping on the value of $this->plaintext. I don't really know...

I think we should just get a patch up on gerrit for this and see how it tests. That's the most reasonable approach IMO, especially since we've fixed many of these MW message output issues in the open. Unless there are strong objections...

Change 921452 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Fix escaping in BlockLogFormatter

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

Yes, I think replacing text() with escaped() in all the 3 conditionals should fix this, BUT I have no idea if this will break anything. I'm not familiar with LogFormatter and I've always found it to be a huge mess, particularly for how the escaping is controlled ($this>plaintext etc.). Maybe we need to manually vary the escaping on the value of $this->plaintext. I don't really know...

I think we should just get a patch up on gerrit for this and see how it tests. That's the most reasonable approach IMO, especially since we've fixed many of these MW message output issues in the open. Unless there are strong objections...

No objections in the meantime, so I pushed the fix to gerrit to get the ball rolling.

Change 921452 merged by jenkins-bot:

[mediawiki/core@master] Fix escaping in BlockLogFormatter

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

Change 921165 had a related patch set uploaded (by Legoktm; author: Daimona Eaytoy):

[mediawiki/core@REL1_40] Fix escaping in BlockLogFormatter

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

Change 921546 had a related patch set uploaded (by Legoktm; author: Daimona Eaytoy):

[mediawiki/core@REL1_39] Fix escaping in BlockLogFormatter

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

Change 921547 had a related patch set uploaded (by Legoktm; author: Daimona Eaytoy):

[mediawiki/core@REL1_38] Fix escaping in BlockLogFormatter

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

There's a merge conflict when backporting to REL1_35 that I didn't investigate yet.

There's a merge conflict when backporting to REL1_35 that I didn't investigate yet.

I'll take care of that.

Change 921547 merged by jenkins-bot:

[mediawiki/core@REL1_38] Fix escaping in BlockLogFormatter

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

Change 921546 merged by jenkins-bot:

[mediawiki/core@REL1_39] Fix escaping in BlockLogFormatter

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

Change 921165 merged by jenkins-bot:

[mediawiki/core@REL1_40] Fix escaping in BlockLogFormatter

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

There's a merge conflict when backporting to REL1_35 that I didn't investigate yet.

I'll take care of that.

Or maybe not? It's rejecting my push for some reason. The conflict itself is easy to fix, a couple of places that use text() in master did not exist in 1.35.

Change 921545 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@REL1_35] Fix escaping in BlockLogFormatter

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

Change 921545 merged by jenkins-bot:

[mediawiki/core@REL1_35] Fix escaping in BlockLogFormatter

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

Thanks all, I'm leaving it up to the Security team to close this task and make it public when they're comfortable with that.

Thanks all, I'm leaving it up to the Security team to close this task and make it public when they're comfortable with that.

Thanks for getting this done. I'll make this task public now since all of the work was done in gerrit and the issue is now being tracked for the next release as well: T333617.

sbassett changed Author Affiliation from N/A to WMF Product.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett moved this task from Incoming to Our Part Is Done on the Security-Team board.
sbassett added a project: SecTeam-Processed.
MoritzMuehlenhoff renamed this task from XSS in BlockLogFormatter due to unsafe message use to XSS in BlockLogFormatter due to unsafe message use (CVE-2023-36675).Jun 26 2023, 9:32 AM
Reedy renamed this task from XSS in BlockLogFormatter due to unsafe message use (CVE-2023-36675) to CVE-2023-36675: XSS in BlockLogFormatter due to unsafe message use (CVE-2023-36675).Jun 26 2023, 10:58 AM
Reedy renamed this task from CVE-2023-36675: XSS in BlockLogFormatter due to unsafe message use (CVE-2023-36675) to CVE-2023-36675: XSS in BlockLogFormatter due to unsafe message use.Jun 29 2023, 7:46 PM