Page MenuHomePhabricator

CVE-2023-22910: XSS in Wikibase date formatting
Closed, ResolvedPublicSecurity

Description

Impact: Users with the editinterface right (not limited to editsitejs) can cause Wikibase to emit arbitrary HTML in certain situations, including in APIs where callers might treat the result as trusted HTML and use it in their own sites. On Wikidata, the editinterface right is granted not only to interface administrators (who also have editsitejs, so they can already run arbitrary JS on Wikidata, though they still shouldn’t be able to break API users in this way), but also to regular administrators as well as Wikidata staff.

Description: Wikibase formats time values for HTML using two classes, MwTimeIsoFormatter and HtmlTimeFormatter. MwTimeIsoFormatter is responsible for formatting the timestamp depending on the precision of the time value, using various interface messages; HtmlTimeFormatter wraps it to add the calendar model, if necessary. An example result looks like this:

14 March 1879<sup class="wb-calendar-name">Gregorian</sup>
|MwTimeIsoF…||.............HtmlTimeFormatter.............|

HtmlTimeFormatter assumes that its inner formatter (which can actually be any formatter, as far as it’s concerned) emits HTML:

/**
 * @param FormatterOptions|null $options
 * @param ValueFormatter $dateTimeFormatter A value formatter that accepts TimeValue objects and
 *  returns the formatted date and time, but not the calendar model. Must return HTML.
 */
public function __construct(
HtmlTimeFormatter::format()
$formatted = $this->dateTimeFormatter->format( $value );

if ( $this->calendarNameNeeded( $value ) ) {
	$formatted .= '<sup class="wb-calendar-name">'
		. $this->formatCalendarName( $value->getCalendarModel() )
		. '</sup>';
}

However, MwTimeIsoFormatter returns plain text:

/**
 * @param TimeValue $timeValue
 *
 * @return string Text
 */
private function formatTimeValue( TimeValue $timeValue ) {
MwTimeIsoFormatter::getLocalizedYear()
return wfMessage(
	'wikibase-time-precision-' . ( $isBCE ? 'BCE-' : '' ) . $msg,
	$year
)
->inLanguage( $this->language )
->text();

The two formatters are wired up without any escaping layer between them:

WikibaseValueFormatterBuilders::newTimeFormatter()
return new HtmlTimeFormatter( $options, new MwTimeIsoFormatter( $options ) );

So if a user with the editinterface right edits one of the relevant interface messages (wikibase-time-precision-*) to include HTML (e.g. <script>), it will be returned unchanged by $message->text() and be added directly to the resulting HTML; this HTML can then turn up in the Wikibase UI (such as on item pages and diffs), but also in third parties who use the wbformatvalue API with the generate parameter set to one of the available HTML formats.

Event Timeline

Noticed while working on T323568; I’ll try to think of a way to patch this (secretly) that won’t merge-conflict too much with the work I want to do in that task (publicly)…

that won’t merge-conflict too much with the work I want to do in that task

Shouldn’t be too bad after all:

$formatted = $this->dateTimeFormatter->format( $value );

if ( $this->calendarNameNeeded( $value ) ) {
	$formatted .= '<sup class="wb-calendar-name">'
		. $this->formatCalendarName( $value->getCalendarModel() )
		. '</sup>';
}

For this task, we need to fix the first line; in T323568, I’ll need to touch the if condition; thanks to the blank line between them, Git should be able to merge the changes without conflict.

Proposed patch:

That change looks sensible as a stop-gap for the particular issue described here. We can think of better ways after the immediate security threat is resolved. (I still need to try it out)

Do we have any cases where that message is maybe already overridden with (non-malicious) HTML?

Not as far as I can tell – all the translations in Wikibase.git don’t contain any <, and the only overridden translation on Wikidata is plain text as well.

I confirmed that this task's issue exists in the first place and that Lucas' patch above (in T323592#8413371) fixes it. From my side, this is ready to be deployed.

I also checked that the messages for "Gregorian" and "Julian" calendar models do not have the same issue. They do not.

Fix should be deployed in production (SAL); I don’t think it can be verified without revealing the issue (you’d need to create a malicious MediaWiki: page).

Backporting note: I expect this should apply cleanly to all supported REL_* branches. (I don’t see any changes in git log -p lib/includes/Formatters/HtmlTimeFormatter.php that look like they should conflict, only some routine maintenance touching other lines.)

mmartorana changed the task status from Open to In Progress.Nov 28 2022, 5:26 PM
mmartorana triaged this task as Medium priority.
mmartorana changed Risk Rating from N/A to Medium.

@Lucas_Werkmeister_WMDE Can you coordinate with the security team to arrange for an updated version of the security patch? The one that was created for wmf.10 doesn't apply cleanly to wmf.12.

Hey @dancy @Lucas_Werkmeister_WMDE et al -

The patch above seems to conflict with wmf.12 in lib/includes/Formatters/HtmlTimeFormatter.php at lines 41 - 51, which is just the comment block for format(). I assume that, given @Jdforrester-WMF's comment at T276237#8429638 and the mentioned conflicting change set, that we do still want this patch to be applied, as the $formatted value wouldn't be sanitized otherwise. Of course @Lucas_Werkmeister_WMDE or @Michael would need to confirm. The updated patch below should apply to wmf.12 with the security functionality in place:

Hey @sbassett @dancy: Yes, we still want that patch to be applied. I looked at your patch, and as far as I can tell, it does fix the problem. We can move forward with that.

The comment part is not ideal yet, but we can look into that when we make this task public.

I'm sorry for not catching this issue in code review. 😔

/me writes note for our next retrospective to figure out how to prevent something like that in the future

Sorry, I hoped that it wouldn’t conflict but didn’t think about the phpdoc :/ @sbassett’s update looks good to me, though it drops a @param comment – the following would be slightly better for the eventual publish on Gerrit etc., but it’s only a doc comment change, so it doesn’t need to be deployed immediately:

Confirmed that 01-T323592-rev3.patch applies cleanly to wmf.12. Thanks for your help everyone.

Mstyles changed the visibility from "Custom Policy" to "Public (No Login Required)".Jan 12 2023, 3:06 AM
Mstyles changed the edit policy from "Custom Policy" to "All Users".

Change 879171 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879454 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879455 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_38] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879456 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879455 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_38] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879454 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Change 879456 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_35] SECURITY: HTML-escape inner formatter in HtmlTimeFormatter

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

Alright, the REL1_35 branch was a bit tricky but the fix should now be merged on all the relevant branches. Should we still keep this task open for something or can we close it?

Mstyles claimed this task.
Mstyles subscribed.

I'm closing it since everything has been merged. Thanks so much!

mmartorana renamed this task from XSS in Wikibase date formatting to CVE-2023-22910: XSS in Wikibase date formatting.Jan 12 2023, 6:26 PM