Page MenuHomePhabricator

XSS in Wikidata Query Service UI
Open, NormalPublic

Description

TL;DR: arbitrary JS execution on query.wikidata.org (not CORS-whitelisted)

The Wikidata Query Service UI has a convenient feature (implemented in I6c8a7ebac1, T137784) where mathematical expressions in results are displayed directly, for example:

Unfortunately, it does this by directly pasting the contents of MathML literals into the HTML:

case DATATYPE_MATHML:
    $html.append( $( data.value ) );
    break;

I think this is fine for MathML from the Wikibase RDF output, which is generated by Extension:Math and hopefully trustworthy at the markup level regardless of what the underlying TeX value on Wikidata is. However, nothing stops a query author from tagging any other string as MathML:

SELECT * WHERE {
  BIND("<script>alert('XSS')</script>"^^<http://www.w3.org/1998/Math/MathML> AS ?xss)
}

This means that if an attacker can trick a victim to open a link like this one, they can run arbitrary JavaScript on query.wikidata.org in the user’s browser. (Clicking a link to query results is a pretty common operation for Wikidata users.)

Fortunately, query.wikidata.org is not on the CORS whitelist ($wgCrossSiteAJAXdomains) for production wikis (I had proposed this in T218568, but it was declined), so I don’t think this is enough to let the attacker steal the victim’s Wikimedia accounts.


Steps to fix (from T233213#5584298):

  • Turn P9315 into a patch against wikidata/query/gui-deploy
  • On the deployment server (is that deployment.eqiad.wmnet or a different one, btw?), apply that patch in the gui submodule of the wikidata/query/deploy checkout (/srv/deployment/wdqs/wdqs?)
  • Commit that patch to the gui submodule, and then commit the submodule update in the main deploy repository? Not sure if this is necessary
  • Deploy the current tree (scap deploy?)
  • Verify that the vulnerability is fixed

So tomorrow EU time (2pm GMT) we will...

Get the on gerrit code in shape:

Announce and update final things:

Details

Related Gerrit Patches:
wikidata/query/gui : masterAdd security task to Gruntfile

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

What we need to render in the query service UI is MathML (whatever is in the RDF), not TeX. That’s not what the REST API is for, is it?

That Mathjax 3 can not render MathML generated by MathJax 2 is interesting.

I’ve reported it upstream as mathjax/MathJax#2214, FWIW.

Physikerwelt added a comment.EditedOct 11 2019, 3:48 PM

@Lucas_Werkmeister_WMDE this is implemented but not enabled. Mathoid can handle MathML as input type, but the value 'mathml' must be added to restbase.

See
https://github.com/wikimedia/restbase/commit/86f93a823f8447c777269610768d62c4f9d06a9e
for the corresponding patch for the mhchem format.

So you’re recommending that the MathML input type is enabled in the REST API, and then the workflow is something like this?

  1. Send MathML to REST API
  2. Get hash back
  3. Send hash to REST API
  4. Get… something back? SVG? PNG? Something else?
  5. Embed whatever-it-is in the result

That’s going to take a lot of network requests, though, and making the value formatting asynchronous would also take some work.

Sounds like that flow could also result in many requests to that API in a single second (2x the number of things to be formatted)? Unless the API provides batching.

@Lucas_Werkmeister_WMDE @Addshore - while the MathML/MathJax stuff is being discussed and sorted, could we work towards deploying a more immediate mitigation for this issue - either what was suggested in T233213#5502971 or perhaps an (imperfect) sanitization patch? I think that would be preferable to having a live XSS on a Wikimedia production site (reported almost a month ago) continue to remain live. Please let us know if there's anything the Security-Team can do to help get such a mitigation deployed.

@sbassett deployments for the query service UI are currently somewhat blocked, as I understand it – we’ll hopefully unblock this tomorrow.

I’ve updated P9315 after a MathJax developer commented on the MathML incompatibility issue (it seems to be very limited in scope and can be worked around); I think it should be good enough to deploy as a first version that will fix the XSS.

@Mathew.onipe, @Gehel: I think the process to deploy this without making the fix public prematurely would be:

  1. Turn P9315 into a patch against wikidata/query/gui-deploy
  2. On the deployment server (is that deployment.eqiad.wmnet or a different one, btw?), apply that patch in the gui submodule of the wikidata/query/deploy checkout (/src/deployment/wdqs/wdqs?)
  3. Commit that patch to the gui submodule, and then commit the submodule update in the main deploy repository? Not sure if this is necessary
  4. Deploy the current tree (scap deploy?)
  5. Verify that the vulnerability is fixed
  6. Coordinate pre-announcements?
  7. Upload the fix to Gerrit to the wikidata/query/gui repository
  8. Merge it, causing a corresponding change to wikidata/query/gui-deploy to be built
  9. Merge that change
  10. Update gui submodule in wikidata/query/deploy repository, bringing it in line with the deployment server’s version
  11. Announce the fix
  12. Discuss any further changes to math rendering in T214980: Support mathematical formulae in Wikidata Query Service UI on all browsers (public)

Does that sound right to you?

  1. Turn P9315 into a patch against wikidata/query/gui-deploy
rm -rf build/
grunt shell:cloneDeploy # clones wikidata/query/gui-deploy into build/
grunt clean:deploy
grunt only_build # builds in build/
grunt shell:commitDeploy # commits result with appropriate commit message
git -C build/ format-patch @^ # generate patch

The result is a bit large for a Phabricator paste (14 MB), so maybe I can just scp it to the deployment server directly once you’re ready (assuming it’s deployment.eqiad.wmnet or another one that I have access to).

@LucasWerkmeister

How is the patch coming along? When do we expect this to be deployed?

Thanks

We can generate the patch, or the deployer can generate the patch.
instructions in T233213#5584314, or we need to know where to send it.
We are unsure about what the deployment process is for this.

In contact with discovery on IRC, so will see if we can get this moving along

I’ve updated P9315 after a MathJax developer commented on the MathML incompatibility issue (it seems to be very limited in scope and can be worked around); I think it should be good enough to deploy as a first version that will fix the XSS.
@Mathew.onipe, @Gehel: I think the process to deploy this without making the fix public prematurely would be:

  1. Turn P9315 into a patch against wikidata/query/gui-deploy
  2. On the deployment server (is that deployment.eqiad.wmnet or a different one, btw?), apply that patch in the gui submodule of the wikidata/query/deploy checkout (/src/deployment/wdqs/wdqs?)
  3. Commit that patch to the gui submodule, and then commit the submodule update in the main deploy repository? Not sure if this is necessary
  4. Deploy the current tree (scap deploy?)
  5. Verify that the vulnerability is fixed
  6. Coordinate pre-announcements?
  7. Upload the fix to Gerrit to the wikidata/query/gui repository
  8. Merge it, causing a corresponding change to wikidata/query/gui-deploy to be built
  9. Merge that change
  10. Update gui submodule in wikidata/query/deploy repository, bringing it in line with the deployment server’s version
  11. Announce the fix
  12. Discuss any further changes to math rendering in T214980: Support mathematical formulae in Wikidata Query Service UI on all browsers (public)

Does that sound right to you?

I'm probably missing some context here. What do you mean by '..without making the fix public prematurely'?
The fix is still going to be public from step 7.
Why not do this the normal way like you specified from step 7 to 12?

The fix is still going to be public from step 7.

Yes, but by that point we’ve already fixed the vulnerability on query.wikidata.org (step 4+5).

I’ve just rebuilt the patch and copied it to the deployment server, you can find it at /home/lucaswerkmeister-wmde/T233213.patch. It should apply cleanly to /srv/deployment/wdqs/wdqs/gui/.

Thanks for providing the patch @Lucas_Werkmeister_WMDE

As far as I can tell we have to:

  • Do those steps 1-5 to fix in production without exposing the bug before having it fixed.
  • Also fix our other labs deployments of this GUI? (or maybe we just do this immediately after publishing the fix?)
  • Prepare patches for the public places, so the gerrit repos and prepare to rebuild the docker images that people use
  • Step 6, coordinate announcements
  • There is sometimes also a pre release to parties that we know run the code, normally for mediawiki that is wikia etc? (documented on wikitech somewhere) Does this still happen? We have some people we could tell that we know use the query service GUI.
  • Do steps 7-11

Thoughts?

@Gehel @Mathew.onipe How can we coordinate further to get this deployed on the service?
How does the patch mentioned in T233213#5614669 look to you?

I am still very confused by the bug. How does adding a new method to render mathematical expressions (which might cause new problems and contradicts previous efforts to reduce the number of math tending options) resolve a XSS vulnerablyty? I would suggest to spit the patch, disable XSS in step one and fix the rendering problem in step two.

The XSS vulnerability is inherent in the old implementation of math markup. I don’t think there’s any way to fix it without completely removing that version of math rendering, and I didn’t want to do that without providing an alternative, safer method. (I nevertheless provided a patch for that in T233213#5502971, but nobody deployed it 🤷)

Since I implemented the 'old implemention for math markup' I would like to know how this has to do with XSS. It does not use JS at all. I think the XSS problem has nothing to do with Math rendering. IMHO Math rendering for non MathML enabled rendering should be implemented as on all other Wikimedia Sites using the API, which provides fallback images.

Since I implemented the 'old implemention for math markup' I would like to know how this has to do with XSS. It does not use JS at all. I think the XSS problem has nothing to do with Math rendering. IMHO Math rendering for non MathML enabled rendering should be implemented as on all other Wikimedia Sites using the API, which provides fallback images.

That's likely not possible here as discussed around T233213#5573393

As far as I can tell we are waiting feedback from the QS team, so going to assign one of them to make sure we stay as on top of this as we can

Sounds like that flow could also result in many requests to that API in a single second (2x the number of things to be formatted)? Unless the API provides batching.

The API was designed for a very high number of requests. The API was introduced to replace a database lookup and should not be seen as something slow but very fast. If this is not the case the whole concept of RESTification should be questioned. I can not see a reason why this case is special. Wikidata uses the same input syntax for formulae as within wikitext. If one needs to implement two tending methods this seems to be somehow wrong.
My experience from the past is that it is better to think twice before deploying suboptimal workarounds.

Hi @Gehel @Mathew.onipe: is there anything which we could do at WMDE side that would help with deploying the the provided fix?

The API was designed for a very high number of requests. The API was introduced to replace a database lookup and should not be seen as something slow but very fast. If this is not the case the whole concept of RESTification should be questioned. I can not see a reason why this case is special. Wikidata uses the same input syntax for formulae as within wikitext. If one needs to implement two tending methods this seems to be somehow wrong.
My experience from the past is that it is better to think twice before deploying suboptimal workarounds.

If we want to discuss this we should discuss it in another ticket.
More than anything this is a product decision about what to expect from the UI of the query service. Make a bunch of requests for images when loading a result, or do it in the client.

I still am very concerned about this procedure.
That is how the example provided in the ticket description currently looks

it the patch would be deployed it would look completely different.
Thus I consider the title "XSS in Wikidata Query Service UI" as misleading. The patch does much more. It introduces a new Math rendering engine. I think this should be discussed and not just be deployed and then test in production what happens if users interact with it.

The XSS is the important part, because your implementation injects arbitrary HTML into the page. You’re welcome to discuss alternative implementations after we’ve deployed a fix for this.

@hoo you mentioned you still have access to the WDQS servers, do you know how to deploy a change to the UI? (T233213#5584298 is probably the most important comment to catch up on.)

hoo added a comment.Tue, Nov 5, 9:20 PM

@hoo you mentioned you still have access to the WDQS servers, do you know how to deploy a change to the UI? (T233213#5584298 is probably the most important comment to catch up on.)

I haven't done a WDQS deploy in quite a while, but the steps in there sound right to me. If you want, we can (internally) fix a time for the scap deploy(s)… maybe as soon as tomorrow.

Despite me at least @Gehel is able to deploy WDQS.

I've deployed @Lucas_Werkmeister_WMDE patch. But I can still run:

SELECT * WHERE {
  BIND("<script>alert('XSS')</script>"^^<http://www.w3.org/1998/Math/MathML> AS ?xss)
}

successfully.
I might be doing something wrong. Please can someone check? @Addshore

Thanks @Mathew.onipe !
For my understanding, have you deployed the bandaid suggested in T233213#5502971 or P9315 ?

I just checked the live site and I still see the old code deployed when looking at the minified JS.

hoo added a comment.Wed, Nov 6, 9:07 AM

I just checked the live site and I still see the old code deployed when looking at the minified JS.

The new minified versions are present (alongside the current versions), but index.html was not updated on the individual hosts (so the state on deploy1001 differs from the state on the actual wdqs servers).

hoo@deploy1001:/srv/deployment/wdqs/wdqs/gui$ md5sum $(pwd)/index.html
11ee88217d78d98e52f9008add62a23c  /srv/deployment/wdqs/wdqs/gui/index.html
hoo@wdqs1005:/srv/deployment/wdqs/wdqs/gui$ md5sum $(pwd)/index.html
29ce000ed3201781a6c06eb03d73b002  /srv/deployment/wdqs/wdqs/gui/index.html

@Mathew.onipe Do you want to re-scap?

hoo added a comment.Wed, Nov 6, 9:11 AM

I just checked the live site and I still see the old code deployed when looking at the minified JS.

The new minified versions are present (alongside the current versions), but index.html was not updated on the individual hosts (so the state on deploy1001 differs from the state on the actual wdqs servers).

hoo@deploy1001:/srv/deployment/wdqs/wdqs/gui$ md5sum $(pwd)/index.html
11ee88217d78d98e52f9008add62a23c  /srv/deployment/wdqs/wdqs/gui/index.html
hoo@wdqs1005:/srv/deployment/wdqs/wdqs/gui$ md5sum $(pwd)/index.html
29ce000ed3201781a6c06eb03d73b002  /srv/deployment/wdqs/wdqs/gui/index.html

@Mathew.onipe Do you want to re-scap?

That apparently just happened, this should be fine now.

Sorry all.. I just did another deployment and patch has been correctly applied. Also test works fine now

Addshore added a comment.EditedWed, Nov 6, 9:14 AM

XSS looks fixed to me :)
And formula rendering still works too

Yes. The layout in firefox and chrome looks different now. And the success criteria defined in T214980 are not fulfilled.

Yes. The layout in firefox and chrome looks different now. And the success criteria defined in T214980 are not fulfilled.

The acceptance criteria of T214980 were also not fulfilled before this XSS fix was deployed. This task is not about meeting the acceptance criteria of T214980

Addshore added a subscriber: Tarrow.Wed, Nov 6, 9:40 AM

Okay, now it’s time for making the patch public and trying to get it deployed on other WDQS instances. COM people, should we do a pre-announcement (“we will soon publish a fix for a serious security vulnerability, you’ll want to update as quickly as possible”)?

The acceptance criteria of T214980 were also not fulfilled before this XSS fix was deployed. This task is not about meeting the acceptance criteria of T214980

I could not agree more. That is why I am complaining all the time. Linking T214980 and this change in one patch is a serious mistake. The patch is a significant change to the rendering which was done without community involvement. This is really a big issue for my work in building up a community and fighting against the myth that WMF randomly changes things without community involvement. The announcement will make it even worse. Fixing XSS is important and I am really happy that you do a great job here. But linking that to new features and bypass the open approach of code review etc. to deploy a new feature is a really bad practice.

The patch is a significant change to the rendering which was done without community involvement.

Turning off the rendering entirely is also a big change in the rendering which would have been done without community involvement.

Fixing XSS is important and I am really happy that you do a great job here. But linking that to new features and bypass the open approach of code review etc. to deploy a new feature is a really bad practice.

We are not saying T214980 is done and closed with this change & fix.
T214980 can be re evaluated and assessed once all of the work around this security fix is done, but right now that is the priority.

Physikerwelt added a comment.EditedWed, Nov 6, 11:29 AM

Turning off the rendering entirely is also a big change in the rendering which would have been done without community involvement.

If you call the current rendering turned off, it has never been turned on. So there would be no change for all users except for those that used the bind construct.

After all, now that MathJax 3 is deployed in production, I suggest to continue the discussion here T237516.

When discussing anywhere else, please keep in mind that this is still a non-public security issue that is exploitable on many WDQS UI installations in the wild.

Thanks. I checked with a private browser tab that the link is not visible from the outside. I would still suggest not to distribute the patch that bundles MathJax 3. To include a new external library I would very much like to see that this takes the standard WMF code review route.

Addshore claimed this task.EditedWed, Nov 6, 4:53 PM

Has a little chat with @Reedy and got some things figured out.

So tomorrow EU time (2pm GMT) we will...

Get the on gerrit code in shape:

  • Merge the patch on gerrit into the main repo
  • Build versions of our docker images with the new code
  • Merge the patch that will be generated in wikidata/query/gui-deploy
  • Update gui submodule in wikidata/query/deploy repository, bringing it in line with the deployment server’s version
  • Confirm that https://archiva.wikimedia.org/repository/snapshots/org/wikidata/query/rdf/service 0.3.5 has been regenerated
  • Make a new wdqs docker image that will use the 0.3.5 zip.....

Announce and update final things:

  • Update our test things on labs (and wikibase-registry) that also run this code
  • Create a CVE including details of the fixed versions
  • Announce to wikitech-l/wikidata-l/wikidata-tech/wikibase/telegram
  • Directly contact the people that we know run this GUI in the wild

(now in the task description)

Restricted Application added a project: User-Addshore. · View Herald TranscriptWed, Nov 6, 4:53 PM

Announce to wikitech-l/wikidata-l/wikibase/telegram

wikidata-tech as well?

The one thing that remains to figure out is updating the zips on https://archiva.wikimedia.org/repository/snapshots/org/wikidata/query/rdf/service

Pinging @Mathew.onipe and @Gehel for more info on that.

wikidata-tech as well?

Added

Addshore updated the task description. (Show Details)Wed, Nov 6, 5:17 PM
Gehel added a comment.Wed, Nov 6, 6:30 PM

The one thing that remains to figure out is updating the zips on https://archiva.wikimedia.org/repository/snapshots/org/wikidata/query/rdf/service
Pinging @Mathew.onipe and @Gehel for more info on that.

The docker image is generated from the Maven artefact? I was expecting it to use a separate release process (I don't know why). Another thing that we should cleanup at some point.

The Maven artefacts are generated from the main wikidata/query/gui repo, not from the deploy repo (yes, I know). As soon as the code is merge there, @Mathew.onipe can create a new SNAPSHOT. (and yes, we should stop relying on SNAPSHOT, E_TOO_MANY_THINGS_TO_CLEANUP).

Addshore updated the task description. (Show Details)Thu, Nov 7, 1:32 PM
Addshore updated the task description. (Show Details)
Addshore updated the task description. (Show Details)Thu, Nov 7, 1:38 PM
Addshore updated the task description. (Show Details)Thu, Nov 7, 4:02 PM
Addshore updated the task description. (Show Details)Thu, Nov 7, 4:36 PM
Addshore updated the task description. (Show Details)Thu, Nov 7, 4:49 PM
Addshore updated the task description. (Show Details)Thu, Nov 7, 4:59 PM

Announcement done on the mailing-lists noted above and the Wikibase Telegram group.

Addshore updated the task description. (Show Details)Thu, Nov 7, 5:12 PM
Addshore updated the task description. (Show Details)Fri, Nov 8, 10:22 AM

@JBennett I think we can make this public now?
I had a quick flick through the comments and don't see anything private in here.

Addshore removed Addshore as the assignee of this task.Fri, Nov 8, 10:46 AM

@JBennett I think we can make this public now?
I had a quick flick through the comments and don't see anything private in here.

Given that this went out to public mailing lists, yes :) I'll do that now. It appears the only outstanding items within the task description are:

  1. Create a CVE including details of the fixed versions.
  2. Discuss any further changes to math rendering in T214980.

The former can be done here, though if you'd like the Security-Team to handle this, we can. The latter I'm assuming is ongoing, indefinitely and shouldn't block resolving this task.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Tue, Nov 12, 8:42 PM

Thanks @sbassett for opening the task and summarizing the status. WMDE is going to create the CVE soon (we're currently out at the conference, so that's why we've gotten to do it yet).

Addshore claimed this task.Fri, Nov 15, 2:22 PM
Addshore lowered the priority of this task from High to Normal.Thu, Nov 21, 10:17 AM

Change 552293 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[wikidata/query/gui@master] Add security task to Gruntfile

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

Change 552293 merged by jenkins-bot:
[wikidata/query/gui@master] Add security task to Gruntfile

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