Page MenuHomePhabricator

Cargo: HTML/JS injection via unescaped messages
Closed, ResolvedPublicSecurity

Description

While reviewing random patches I found a whole bunch of places in MediaWiki-extensions-Cargo where unescaped output from Message::text() is used as if it is HTML: https://codesearch.wmcloud.org/search/?q=(text%7Cplain)%5C(%5CW*%5C.&repos=Extension:Cargo. This allows injecting arbitrary HTML and JavaScript by anyone who manages to manipulate the messages in questions.

As far as I'm aware of this is one of the more relevant, alive extensions. Luckily WikiApiary is online today: https://wikiapiary.com/wiki/Extension:Cargo. It reports the extension being used on 290 websites.

I'm not sure how to classify this so I will start with this being a restricted Security-Team task.

Details

Risk Rating
Medium
Author Affiliation
Wikimedia Deutschland

Event Timeline

Should parse() or escape() always be used? When should text() ever be used?

Basic documentation is here: https://www.mediawiki.org/wiki/Manual:Messages_API#Output_modes_and_escaping. text() is for situations that don't need escaping, e.g. when the output goes to a text file or console. It's most often used together with the Html class. These lines are all correct and all do (mostly) the same:

$html .= Html::element( 'span', [], $msg->text() );
$html .= Html::rawElement( 'span', [], $msg->escaped() );
$html .= '<span>' . $msg->escaped() . '</span>';
$html .= '<span>' . htmlspecialchars( $msg->text() ) . '</span>';

The very first one is actually the recommended one.

parse() should be very rarely used because it's significantly slower. Only when you need the full power of wikitext.

@thiemowmde - thanks for finding these, and sorry for the long delay in handling this. I just checked in this fix - I don't know if I got all the necessary escaping there, but hopefully I got most of it:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cargo/+/899598

sbassett assigned this task to Yaron_Koren.
sbassett triaged this task as Medium priority.
sbassett subscribed.

@thiemowmde - thanks for finding these, and sorry for the long delay in handling this. I just checked in this fix - I don't know if I got all the necessary escaping there, but hopefully I got most of it:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Cargo/+/899598

Let's resolve for now then. If more are found, this can always be reopened. Also - it looks like @thiemowmde pointed out some instances where data might be getting double-escaped now, on the gerrit change set. It might make sense to file a separate bug to investigate those issues.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 27 2023, 7:14 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Medium.