Page MenuHomePhabricator

Stored XSS via WikibaseMediaInfo caption fields at commons.wikimedia.org
Closed, ResolvedPublicSecurity

Description

As reported via Aidil Arief to security@ on 2021-10-16:

Description

There appears to be an XSS via the caption fields for a given media file in Special:UploadWizard. I've tested a couple of variations of the provided payload ("><img src=x onerror=prompt()>) which do not seem to execute, but the provided payload definitely does.

Update: This appears to be an issue with WikibaseMediaInfo and how it currently displays media caption data.

Steps to reproduce

  1. Log in to commons.wikimedia.org
  2. Upload your media file at https://commons.wikimedia.org/wiki/Special:UploadWizard, or work with an existing media file
  3. Input "><img src=x onerror=prompt()> or similar as a value for one or more caption fields for the media file
  4. Navigating to the media file will produce the javascript prompt

Event Timeline

sbassett changed Risk Rating from N/A to High.
sbassett removed a project: UploadWizard.
sbassett updated the task description. (Show Details)

Payload appears to be executed like this within the rendered source:

<div class="wbmi-caption-value" dir="ltr" lang="simple"> "&gt;<img src="x" onerror="alert(document.domain)"> </div>

The open html tag that's part of the payload makes it through even though the first html close tag seems to be properly escaped.

Looks like there's some incomplete escaping going on here? The simple solution is to wrap that in htmlspecialchars as is done for the default, empty caption message a couple lines below, but there's likely a better way and we obviously don't want to double-escape any caption data. I suppose this could also be done at the mustache layer, but that makes less sense to me just glancing at the code.

sbassett updated the task description. (Show Details)

Hi,

Thanks for adding me here :)

Awesome, I just reported this vulnerability and it's very crowded here :)

A little additional information that the image file that has been inserted Caption XSS, so when the victim uses the image in the main domain to upload, the XSS will be triggered in the main domain https://id.wikipedia.org

Then when you assume it's Self XSS?

So I will assume when the image I upload and insert the XSS PAYLOAD caption becomes crowded and a lot of it is stored in the user's account, then when the user uses the image to upload, the main domain will trigger XSS :)

Can you add to the title of this report XSS Blind Stored Also triggered on main domain https://en.wikipedia.org ?

I've added the xss video triggered at https://en.wikipedia.org in the email. Please check sir :)

Thanks
Regards,

Aidil Arief

@aidilarf28 - I'm not sure this is anything beyond the issue of WikibaseMediaInfo not properly escaping caption data upon the primary media information page for image files, e.g. https://commons.wikimedia.org/wiki/File:some-file.png. I'm not seeing the XSS render anywhere else, e.g. when the raw media file is viewed, when it is viewed within media viewer, or when it is embedded somewhere else. Do you have documented steps as to how the XSS could be rendered and executed upon id.wikipedia.org or en.wikipedia.org?

Proposed patch below. I'd appreciate some CR from other folks on this task since I'm not incredibly familiar with WikibaseMediaInfo:

Hi @sbassett

I think this is just an additional step towards the behavioral findings in this report.

And when the image containing the caption XSS PAYLOAD is saved by the user, then when the user tries to upload the Wiki with the image, see a pop up appears, and xss is triggered :)

Screenshots:
https://www.dropbox.com/s/xshbbk38pdmbxtx/NEW%202.jpg?dl=0

Videos :
https://www.dropbox.com/s/a5q723mlhxgxgvl/NEW.mp4?dl=0

You don't have to worry about the dropbox url above, because if this bug has been fixed and if you want this video not to spread to the public then I can immediately delete the video from Dropbox :)

Thanks
Regards,

Aidil Arief

Digging a bit more, it looks like this has been around for 3 years or so with Ibf6053abb. And phan-taint-check-plugin passed just fine on the change set, at least a couple different times. I might try to get this deployed today as I'd like for it not to sit any longer, but I am concerned about unintended consequences of my patch, so I'll pull to an mwdebug and try to test on test.w.o as best I can, and keep an eye on logstash for a bit. Worst case is that we roll back and someone with more domain knowledge can craft a better patch sooner than later.

Well, my patch didn't seem to work when testing on mwdebug1002/test-commons. So we'll need the project owners to review this tomorrow (hopfeully) and provide any guidance on where this issue might need to be further addressed.

@sbassett Escaping in MediaInfoEntityTermsView.php is not needed – the value is passed to OOUI\Element, which escapes all parameters, unless they're constructed using OOUI\HtmlSnippet. (In fact, the htmlspecialchars() call causes double escaping.)

I think the bug is in CaptionsPanel.mustache+dom, where caption is rendered without HTML escaping using {{{caption}}} (in Mustache, triple brace = no escaping, double brace = escaping). Possibly that should be just {{caption}}, but I'm not familiar with that code. (There are also other triple-brace parameters in that file, some of which might also need to use double-brace instead.)

@sbassett Escaping in MediaInfoEntityTermsView.php is not needed – the value is passed to OOUI\Element, which escapes all parameters, unless they're constructed using OOUI\HtmlSnippet. (In fact, the htmlspecialchars() call causes double escaping.)

I think the bug is in CaptionsPanel.mustache+dom, where caption is rendered without HTML escaping using {{{caption}}} (in Mustache, triple brace = no escaping, double brace = escaping). Possibly that should be just {{caption}}, but I'm not familiar with that code. (There are also other triple-brace parameters in that file, some of which might also need to use double-brace instead.)

Yes, thanks. I was looking at this this morning a bit and thought that it had to be an issue at the mustache layer even though I originally didn't think that was the case. Downgrading the triple-curlies to doubles in templates/filepage/CaptionsPanel.mustache+dom is a pretty trivial fix, but I'd love to get some feedback from folks a bit more familiar with WikibaseMediaInfo before we deploy that change, just to ensure we aren't breaking any assumptions regarding the type of data/content displayed within that template.

Hi @sbassett – I am a former member of the team with some familiarity with the code here, though it has been a while since I’ve worked on this extension.

Unfortunately I am currently traveling and out of office without a laptop, so I can’t really review private patches at the moment. I will be back on Wednesday and can review then if no one else has gotten to it first.

Hi @sbassett – I am a former member of the team with some familiarity with the code here, though it has been a while since I’ve worked on this extension.

Unfortunately I am currently traveling and out of office without a laptop, so I can’t really review private patches at the moment. I will be back on Wednesday and can review then if no one else has gotten to it first.

Ok, thanks for the reply. I'm most likely going to get a patch up for templates/filepage/CaptionsPanel.mustache+dom today and try to deploy it during the security window today.

@Cparle will take a look first thing tomorrow.

Ok, thanks. Per my comment above, I'm likely going to get a patch up and deployed for the caption-related XSS which seems to be occurring within templates/filepage/CaptionsPanel.mustache+dom as we really don't want a stored XSS like that lingering. What would be helpful is if @Cparle and/or other folks could review that patch, post-deployment, and evaluate if there's a better way to handle that, and then audit any related code to ensure we aren't allowing any additional user-inputted values to remain unsanitized.

Proposed patch for templates/filepage/CaptionsPanel.mustache+dom just to deal with the current XSS within the caption fields. I'd like to get this deployed today during the security window unless there are any serious objections. This patch can revisited revised later this week by @Cparle et al, prior to any backports or tracking for the next supplemental security release (T292236).

Mstyles lowered the priority of this task from High to Low.Oct 18 2021, 9:41 PM
Mstyles moved this task from Security Patch To Deploy to Our Part Is Done on the Security-Team board.

Hi @sbassett

After I double checked, it seems this behavior has been fixed.

Can you confirm it?

@sbassett your patch breaks captions editing on the page :( Looking for a solution now

Alternative solution deployed now. Didn't realise I should not put security patches through gerrit, sorry! ... but it's backported and deployed now anyway

@sbassett and @aidilarf28 can you confirm it's ok now?

Hi @Cparle

Thanks for fixing this issue so fast, it's great :)

Yes, I can confirm XSS is not triggered. And both reports have been fixed.

@sbassett Then what next?

Hi @sbassett

Why is XSS STORED risk rating Low?

@Cparle - Ok, thanks for the fixed patch. I figured there was likely some better way to accomplish this, but we needed to get a quick fix deployed to mitigate the stored XSS on commons. And thanks, @Urbanecm, for the wmf.4 and wmf.5 backports. I'll also update T276237 and note the old security patch should no longer be deployed. In the future, please feel free to reference our current documentation on creating and deploying security patches. And the Gitlab version, whenever we get things migrated over there.

@aidilarf28 - The stored XSS was actually mitigated with the patch @Mstyles and I deployed to production yesterday (T293556#7438632). It just wasn't the best way to fix this issue since it broke some other things. @Cparle, who works more closely with this code, created an improved patch this morning which was then deployed in place of the old security patch. This bug is now rated as a low priority and risk since it has been patched within Wikimedia production. Priorities and risks can evolve over time within the context of a Phabricator task.

sbassett renamed this task from Stored XSS in Special:UploadWizard caption fields at commons.wikimedia.org to Stored XSS via WikibaseMediaInfo caption fields at commons.wikimedia.org.Oct 19 2021, 2:53 PM

Change 731990 had a related patch set uploaded (by SBassett; author: SBassett):

[wikimedia/security/landing-page@master] Add Aidil Arief to security hall of fame for reporting two issues

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

Hi @sbassett

I don't understand the reason why it went low sir :(

Aren't you using the OWASP Risk Rating?

In OWASP Risk Rating, XSS STORED is usually HIGH

Let me know if anything is wrong in my understanding :)

I don't understand the reason why it went low sir :(

Aren't you using the OWASP Risk Rating?

In OWASP Risk Rating, XSS STORED is usually HIGH

Phabricator priority and risk levels do not equal OWASP or other risk levels.

Phabricator priority and risk levels signify the current priority and risk of a given issue or issues. So while the risk of a stored XSS might be high, just in describing the vulnerability, that is not how we currently use Phabricator priority and risk. Since the issue has been mitigated within production via deploying security-patched code, it is no longer considered a high priority or risk to the Foundation or Community.

Hi @sbassett

Oh I see, I understand now.

Sorry to have confused you with my question :)

Success always for you all :)

Have a nice day :)

Change 731954 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@REL1_37] Escape captions when writing stored data into js state

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

sbassett assigned this task to Cparle.

@sbassett so can we close this now?

Yes, thanks again for reviewing this and submitting the updated patch. I'll also go ahead and make this public since it's patched within production and most of the backports have merged or will merge soon. Last steps will be to request a CVE and (re-)announce it with the supplemental security release later this December. But I don't believe WikibaseMediaInfo is really used much outside of the Foundation.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 20 2021, 3:21 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from High to Low.

Change 731956 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@REL1_35] Escape captions when writing stored data into js state

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

Change 731955 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@REL1_36] Escape captions when writing stored data into js state

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

Change 731990 merged by jenkins-bot:

[wikimedia/security/landing-page@master] Add Aidil Arief to security hall of fame for reporting two issues

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

Change 734739 had a related patch set uploaded (by SBassett; author: SBassett):

[wikimedia/security/landing-page@master] Changes to content and Gemfile for security.wikimedia.org

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

Change 734739 merged by jenkins-bot:

[wikimedia/security/landing-page@master] Changes to content and Gemfile for security.wikimedia.org

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

Reedy added a parent task: Restricted Task.Nov 3 2021, 6:41 PM