CVE-2023-36675: ⚓ T332889 XSS in BlockLogFormatter due to unsafe message use

**

XSS in BlockLogFormatter due to unsafe message use

Closed, ResolvedPublicSecurity

**

• Edit Task

• Edit Related Tasks…

• Edit Related Objects…

• Mute Notifications

• Protect as security issue

• Award Token

• Flag For Later

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

Risk Rating

Medium

Author Affiliation

WMF Product

• Task Graph

Event Timeline

Comment Actions

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 ) );

Comment Actions

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…

Comment Actions

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…

Comment Actions

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.

Comment Actions

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

Comment Actions

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

I’ll take care of that.

Comment Actions

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.

Comment Actions

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.

Comment Actions

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)".

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL