Note: most of Wikidata’s badge items are not protected at all. (I would’ve expected all of them to have at least some weak page protection, to be honest.)

Not sure at what layer this needs to happen, but I would assume a simple call to Sanitizer::safeEncodeAttribute() or even htmlentities( $badgeLabelText, ENT_QUOTES ) would be all that was needed here?

Yeah, I expect the fix will be simple, we just need to find the code in Wikibase.

This seems to do the trick:

diff --git a/view/src/SiteLinksView.php b/view/src/SiteLinksView.php
index 5864120618..249313783e 100644
--- a/view/src/SiteLinksView.php
+++ b/view/src/SiteLinksView.php
@@ -351,18 +351,18 @@ private function getHtmlForBadges( array $badges ) {
 		foreach ( $badges as $badge ) {
 			$serialization = $badge->getSerialization();
 			$classes = Sanitizer::escapeClass( $serialization );
 			if ( !empty( $this->badgeItems[$serialization] ) ) {
 				$classes .= ' ' . Sanitizer::escapeClass( $this->badgeItems[$serialization] );
 			}
 
 			$html .= $this->templateFactory->render( 'wb-badge',
 				$classes,
-				$this->entityIdFormatter->formatEntityId( $badge ),
+				Sanitizer::safeEncodeAttribute( $this->entityIdFormatter->formatEntityId( $badge ) ),
 				$badge
 			);
 		}
 
 		return $this->templateFactory->render( 'wikibase-badgeselector', $html );
 	}
 
 }

I’ll try to put together a test as well. And thanks for the pointer to Sanitizer::safeEncodeAttribute(), I didn’t know that method.

Patch with test added:

{F37103553}

Same patch but this time with perhaps better permissions (I think I accidentally attached the previous file to T250720 instead of this task):

The patch isn’t enough, @Michael found another place that is missing escaping when editing badges:

I can’t seem to trigger the onmouseenter= handler in this case (probably because the element in question has 0 width?), but the style injection is still a problem.

Also, when a sitelink doesn’t have a badge yet, it uses an empty badge as a button:

This button has an i18n message as the title= (wikibase-badgeselector-badge-placeholder-title), and that message also isn’t escaped, so if you set it to e.g. Click to assign a badge." onmouseenter="alert('i18n xss') then you have another HTML injection.

I think this is the 10.5-year-old change that introduced the js issues: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/38288
I'm still trying to wrap my head around what problem it tried to actually fix.

Ah, gotcha: T44956

The following change complements the change @Lucas_Werkmeister_WMDE shared above. It should fix the other places about badges mentioned in T339111#8934513, though I have not yet looked into the i18n issue mentioned there.

However, this change modifies code that was last touched/written a decade ago and is used in a lot of code paths in Wikibases legacy js code. I think it would be great if that could get a second and maybe third look.

Also, we should probably be extra on the lookout for community members reporting problems with the js/editing functionality on Item/Property(/Lexeme?) pages.

In T339111#8935513, @Michael wrote:

The following change complements the change @Lucas_Werkmeister_WMDE shared above. It should fix the other places about badges mentioned in T339111#8934513, though I have not yet looked into the i18n issue mentioned there.

Complements as in should be applied as a second patch dependent upon the refactor from T339111#8932224?

However, this change modifies code that was last touched/written a decade ago and is used in a lot of code paths in Wikibases legacy js code. I think it would be great if that could get a second and maybe third look.

Any other folks from the git history, etc. who we could sub to this task?

Also, we should probably be extra on the lookout for community members reporting problems with the js/editing functionality on Item/Property(/Lexeme?) pages.

Once the patch(es) are deployed to Wikimedia production, we could make this task public since Wikibase isn't bundled with the MediaWiki core releases.

In T339111#8935513, @Michael wrote:

The following change complements the change @Lucas_Werkmeister_WMDE shared above. It should fix the other places about badges mentioned in T339111#8934513, though I have not yet looked into the i18n issue mentioned there.

0001-SECURITY-Escape-quotes-in-js-templates.patch2 KBDownload

LGTM, +1. I can confirm it also fixes the i18n issue I found.

In T339111#8936683, @sbassett wrote:

Complements as in should be applied as a second patch dependent upon the refactor from T339111#8932224?

Yes, both should be applied. And while they could be squashed into one, to me they look separate enough that I think it also makes sense to keep them separate, if that’s okay with you.

However, this change modifies code that was last touched/written a decade ago and is used in a lot of code paths in Wikibases legacy js code. I think it would be great if that could get a second and maybe third look.

Any other folks from the git history, etc. who we could sub to this task?

git shortlog -- lib/resources/templates.js repo/resources/templates.js view/resources/wikibase/templates.js has a lot of people who are no longer with us :/ but CC @thiemowmde and @Krinkle who made nontrivial changes.

Sorry, I think I can't help. I joined February 2014. But this here is quite a bit older.

I kind of remember poking at this custom template system. I always wondered why it's even necessary. At one point I even tried to get rid of it: https://gerrit.wikimedia.org/r/252008. Unfortunately that never happened.

I think back then @Tobi_WMDE_SW and @daniel were around, though maybe not working on that particular part of the code. But maybe looking at the changes, especially the Javascript one, does trigger any memories or immediate thoughts?

FTR, I plan to deploy both fixes tomorrow (today’s supposedly a no-deploy day, though there’s some confusion about that) unless I hear any objections.

In T339111#8931931, @Lucas_Werkmeister_WMDE wrote:

This seems to do the trick:

diff --git a/view/src/SiteLinksView.php b/view/src/SiteLinksView.php
index 5864120618..249313783e 100644
--- a/view/src/SiteLinksView.php
+++ b/view/src/SiteLinksView.php
@@ -351,18 +351,18 @@ private function getHtmlForBadges( array $badges ) {
 		foreach ( $badges as $badge ) {
 			$serialization = $badge->getSerialization();
 			$classes = Sanitizer::escapeClass( $serialization );
 			if ( !empty( $this->badgeItems[$serialization] ) ) {
 				$classes .= ' ' . Sanitizer::escapeClass( $this->badgeItems[$serialization] );
 			}
 
 			$html .= $this->templateFactory->render( 'wb-badge',
 				$classes,
-				$this->entityIdFormatter->formatEntityId( $badge ),
+				Sanitizer::safeEncodeAttribute( $this->entityIdFormatter->formatEntityId( $badge ) ),
 				$badge
 			);
 		}
 
 		return $this->templateFactory->render( 'wikibase-badgeselector', $html );
 	}
 
 }

I’ll try to put together a test as well. And thanks for the pointer to Sanitizer::safeEncodeAttribute(), I didn’t know that method.

Is the result of this rendered HTML template at some point passed to a wikitext parser?

If not, I suggest using htmlspecialchars and not Sanitizer::safeEncodeAttribute as the latter is very specific to escaping user input for use in wikitext Parser context. For example, it escapes underscores and slashes that have no meaning in HTML, but would otherwise produce magic words, and anchor tags for bare URLs, etc.

Or better yet, update the Mustache template to use {{ instead of {{{ as it should not be rendering this as raw HTML in the first place.

It maybe be prudent to audit the other Wikibase HTML templates and confirm they also don't place any text as raw HTML.

In T339111#8946884, @Krinkle wrote:

Is the result of this rendered HTML template at some point passed to a wikitext parser?

Doesn’t look like it, it goes into ParserOutput::setText() (very confusingly named, but takes HTML) in FullEntityParserOutputGenerator, and into OutputPage::addHTML() in WikibaseMediaInfoHooks.

If not, I suggest using htmlspecialchars and not Sanitizer::safeEncodeAttribute as the latter is very specific to escaping user input for use in wikitext Parser context. For example, it escapes underscores and slashes that have no meaning in HTML, but would otherwise produce magic words, and anchor tags for bare URLs, etc.

Okay, new patches:

Or better yet, update the Mustache template to use {{ instead of {{{ as it should not be rendering this as raw HTML in the first place.

It’s not a Mustache template, it’s a Wikibase-specific thing, predating Mustache support in core by a few years. (It’s actually implemented based on Message, but bypassing almost all Message functionality – AFAICT there’s little reason why we don’t just replace $1, $2 etc. directly.) Replacing it with Mustache might be a good idea (we need to process these templates server- and client-side, and MediaWiki gives us that), but that should be done outside of a security task.

It maybe be prudent to audit the other Wikibase HTML templates and confirm they also don't place any text as raw HTML.

I did that, T339260 was the only other issue I found.

Patches deployed to wmf.13. (I skipped wmf.12 because the train is already done; wmf.14 doesn’t exist yet on deploy1002.) I don’t think we can really test that the issue doesn’t occur on production without making it quite obvious (since the badge items are reasonably prominent); let’s just hope that it’s fixed now, I think. (And if we hear any user reports about errors that might be due to the changed templating in JS, investigate those.)

It’s not a Mustache template, it’s a Wikibase-specific thing, predating Mustache support in core by a few years. (It’s actually implemented based on Message, but bypassing almost all Message functionality – AFAICT there’s little reason why we don’t just replace $1, $2 etc. directly.) Replacing it with Mustache might be a good idea (we need to process these templates server- and client-side, and MediaWiki gives us that), but that should be done outside of a security task.

I put together a quick proof of concept for decoupling Wikibase’s Template class from core’s Message, and it seems to work well enough in local testing. I’ll push it for CI checks once this task is public. (Migrating to Mustache needs a bit more work because it’ll require updating all the templates.)

@roti_WMDE @adee_wmde @lojo_wmde FYI when considering the next security release of Wikibase

@Tarrow is there a list of Wikibase versions you could point me towards?

@Mstyles Wikibase versions are currently described in https://github.com/wmde/wikibase-release-pipeline/blob/main/CHANGES.md

Change 933649 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Escape badge title

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

Change 933650 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Escape quotes in js templates

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

In T339111#8969994, @Mstyles wrote:

@Tarrow is there a list of Wikibase versions you could point me towards?

For the upcoming supplemental release, I think we'll only need to care about currently supported versions of MediaWiki for Wikibase.

In T339111#8949436, @Lucas_Werkmeister_WMDE wrote:

It’s not a Mustache template, it’s a Wikibase-specific thing, predating Mustache support in core by a few years. (It’s actually implemented based on Message, but bypassing almost all Message functionality – AFAICT there’s little reason why we don’t just replace $1, $2 etc. directly.) Replacing it with Mustache might be a good idea (we need to process these templates server- and client-side, and MediaWiki gives us that), but that should be done outside of a security task.

I put together a quick proof of concept for decoupling Wikibase’s Template class from core’s Message, and it seems to work well enough in local testing. I’ll push it for CI checks once this task is public. (Migrating to Mustache needs a bit more work because it’ll require updating all the templates.)

⇒ POC: Decouple Template from Message

Also, it turns out we’ve wanted to move to Mustache for a long time already (but never prioritized it): T91067: Port wikibase templates to Mustache