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.

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

In T293556#7436539, @matmarex wrote:

@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.

@Cparle will take a look first thing tomorrow.

In T293556#7437111, @egardner wrote:

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.

In T293556#7437407, @CBogen wrote:

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

Deployed to production on 10/18 (https://sal.toolforge.org/log/AQpJlXwB1jz_IcWust7f).

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.

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

In T293556#7441013, @aidilarf28 wrote:

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 so can we close this now?

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