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.

In T233213#5619689, @Physikerwelt wrote:

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

In T233213#5573393, @Addshore wrote:

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?

In T233213#5625870, @Physikerwelt wrote:

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

In T233213#5635381, @Lucas_Werkmeister_WMDE wrote:

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

In T233213#5639358, @Addshore wrote:

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?

In T233213#5639426, @hoo wrote:

In T233213#5639358, @Addshore wrote:

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

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.

In T233213#5639449, @Physikerwelt wrote:

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

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”)?

In T233213#5639465, @Addshore wrote:

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.

In T233213#5639673, @Physikerwelt wrote:

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.

In T233213#5639758, @Addshore wrote:

In T233213#5639673, @Physikerwelt wrote:

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.

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.

In T233213#5641189, @WMDE-leszek wrote:

wikidata-tech as well?

Added

In T233213#5641190, @Addshore wrote:

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

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

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

In T233213#5647214, @Addshore wrote:

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

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

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

CVE just requested via https://cveform.mitre.org/

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-19329