Page MenuHomePhabricator

Wikidata Query UI lets users build links with arbitrary link text and javascript: URL
Closed, ResolvedPublicSecurity

Description

The Wikidata Query GUI allows users to build links with arbitrary, innocuous-looking link text and a javascript: URL, potentially allowing code execution when another user clicks the link: example query.

In T297570#7568944, I observed that the query UI already generates links with arbitrary HREFs as long as they’re returned by the query service, which can either come from Wikidata or just be created directly in the query, for example:

SELECT (<javascript:alert('hi')> AS ?url) {}

Try it!

This generates a results table like this:

url
<javascript:alert('hi')>

Except that the link is clickable, which Phabricator doesn’t allow. I thought that this was not a catastrophic issue, since the URL is visible, and users hopefully won’t click it if it looks suspicious. But then I realized that we sometimes create links with custom link text, e.g. when we encounter a variable together with an associated label variable, in some result views:

#defaultView:Map
SELECT
(<javascript:alert('hi')> AS ?item)
("hi"@en AS ?itemLabel)
("Point(0 0)"^^geo:wktLiteral AS ?coordinates)
{}

Try it!
If you run this query (or directly go to the embed link) and click the red dot, you see something like this:

Screenshot 2021-12-14 at 12-09-29 Wikidata Query Service.png (183×233 px, 9 KB)

<a title="item" href="javascript:alert('hi')" target="_blank" class="item-link" rel="noopener">hi</a>

This link may execute JavaScript if a user clicks it; thanks to the custom link text, they don’t really have any reason not to click it.

In my testing, the impact of this seems to be mitigated by the target=_blank, though. A simple click on the link opens javascript:alert('hi') in a new tab, apparently without running the JavaScript; a shift-click opens a new window on a blank tab, and the JavaScript runs (the alert is shown), but it does not seem to run on query.wikidata.org: changing the code to alert(document.location) shows about:blank.

Also, note that query.wikidata.org is not on the CORS allowlist (with each issue I discover I’m more and more glad that T218568 was declined).

Details

Author Affiliation
Wikimedia Deutschland

Event Timeline

Change was reviewed and is looking good, local tests also showed this fixes the issue at hand

I’m not actually sure how to deploy a security patch for the query service UI – I think the last times we deployed security changes (T233213#5639437 and T238822#5688390) were before we moved the WDQS UI to microsites. Does anyone know more?

(Note: presumably this will involve creating a local build at some point, so I should point out that last time I tried that, it didn’t work well, see T295601#7500334. We should make sure that whatever build we deploy look correct.)

The microsite is configured like this:

git::clone { 'wikidata/query/gui-deploy':
    ensure    => 'latest',
    source    => 'gerrit',
    directory => '/srv/org/wikidata/query',
    branch    => 'production',
}

I assume that, if somebody SSHs into this machine and manually applies a patch on the /srv/org/wikidata/query Git repository, the patch will stay around. (Once another patch in the deployment repo is merged, Puppet will probably start complaining that the git pull fails, but that should still leave the fix in place, I hope.) That should only be a short-term solution, though, because it blocks any other changes from being deployed (there’s no hope of auto-rebasing the minified build files).

That said, I don’t think I have SSH access to these systems (Ganeti?).

I’m not actually sure how to deploy a security patch for the query service UI

Apparently we’re having some public discussion in this direction now: https://gerrit.wikimedia.org/r/c/operations/puppet/+/745634

Apparently that other discussion was triggered by T218900 (which I don’t have access to). Pinging some folks who joined the discussion on Gerrit.

@Lucas_Werkmeister_WMDE - so I see a patch that was confirmed tested above. Is this just blocked on a deployment? Do we need to find an SRE who can assist?

sbassett triaged this task as Medium priority.Dec 20 2021, 9:48 PM
sbassett moved this task from Watching to Security Patch To Deploy on the Security-Team board.
sbassett changed Risk Rating from N/A to Medium.

Hi @Lucas_Werkmeister_WMDE @sbassett ,

I looked at this to see if I can apply the patch but I have some questions:

As you said above the deploy repo for the content of this microsite is: wikidata/query/gui-deploy.

In the patch you attached I see the file wikibase/queryService/ui/resultBrowser/helper/FormatterHelper.js is edited.

I just git cloned the repo and looked for the file but wikibase/queryService/ui/ is pretty much empty except one file called 304px_querybuilder_final.gif.

Then I checked what is actually in production on a backend server and it's the same as in f.e. [miscweb1002:/srv/org/wikidata/query/wikibase/queryService/ui] (as expected it is in sync with the repo).

So I am not sure how we would apply that patch!?

Did something go wrong with the deploy repo state related to T295601 ?

I assume that, if somebody SSHs into this machine and manually applies a patch on the /srv/org/wikidata/query Git repository, the patch will stay around. (Once another patch in the deployment repo is merged, Puppet will probably start complaining that the git pull fails, but that should still leave the fix in place, I hope.)

This is not the case. If we manually edit a file within /srv/org/wikidata/query puppet with "ensure => latest" will take corrective action and revert the manual hack. This is like a feature.

My recent, but now abandoned, Gerrit change to flip it from "latest" to "present" (that was by the way from my side entirely unrelated to this ticket), would have changed exactly that though.

That said, I don’t think I have SSH access to these systems (Ganeti?).

That's true but it would be change-able with a new admin group if we need that.

@Lucas_Werkmeister_WMDE So.. since you are patching FormatterHelper.js but I can't find a file with that name in the entire repo I assume that is not in the deploy repo but something that creates what ends up in the deploy repo. Could you create a patch on the actual deploy repo? We would still have to disable puppet or puppetization of that git clone though on prod servers.

@Dzahn yes, the patch is against the /gui repo, not the /gui-deploy repo. A /gui-deploy patch on its own would be impossible to review, because the deploy repo only contains minified JS (js/wdqs.min.[hash].js). I’ll try to create a /gui-deploy patch, though as I mentioned last time I tried that it went poorly (normally /gui-deploy patches are built in CI), so let’s see if I have better luck this time. (My question above was if there was any other process for this which I wasn’t aware of, but it looks like that’s not the case.)

Alright, I built a patch using more or less the wikidata-query-gui-build CI steps, the diffstat looks okay to me (no spurious deletions as in this change), and it seems to work locally.

Unfortunately, the patch is 15 MB large (containing previous and current versions of all its JavaScript, including a MathJax bundle), and Phabricator tells me that “No configured storage engine can store this file.” I have rsynced it onto deploy1002 (/home/lucaswerkmeister-wmde/T297686.patch); can someone deploy it from there?

To test the change after deployment, follow the “try it!” links in the task description, run the query with Ctrl+Enter, and check that there are no clickable javascript: links in the results (in the second example, click the red dot in the middle of the ocean first). Also, make sure that the upper left corner brand still reads “Wikidata Query Service” and not “Localhost”, that’s usually a good indicator that the local config is still loading correctly.

In my testing, the impact of this seems to be mitigated by the target=_blank, though. A simple click on the link opens javascript:alert('hi') in a new tab, apparently without running the JavaScript; a shift-click opens a new window on a blank tab, and the JavaScript runs (the alert is shown), but it does not seem to run on query.wikidata.org: changing the code to alert(document.location) shows about:blank.

(I’d also still be interested whether this matches everyone else’s experience, or whether in some other browsers the JS actually runs in a query.wikidata.org context.)

(I’d also still be interested whether this matches everyone else’s experience, or whether in some other browsers the JS actually runs in a query.wikidata.org context.)

Testing within recent versions of Chrome, Firefox, Opera, Edge and TorBrowser under MacOS 11, I get the same behavior. TorBrowser actually tries to proactively block the JS (payload detection?) and then seemed to stall a bit when attempting to execute it once I indicated I wanted the scripts to execute.

@Dzahn yes, the patch is against the /gui repo, not the /gui-deploy repo. A /gui-deploy patch on its own would be impossible to review ... My question above was if there was any other process for this which I wasn’t aware of, but it looks like that’s not the case.

I'm afraid not. The process is that we deploy from the deploy repo. If it's impossible to patch the deploy repo then that's a new problem to be solved that we weren't aware of.

While I can do the rsync it also means a bunch of other things:

  • puppet has to be disabled permanently (which is ok for a few days but starting to be a real problem after a week, when the host falls out of puppet db and doesn't get any other updates / security patches)
  • monitoring that would normally alert about disabled puppet has to be silenced
  • we need to leave a reason why this host (or both backends, actually) were live patched
  • people will ask when we can enable puppet again
  • we have to override that we are in code freeze and even manually deploy vs regular deploy
  • next deployment will need clean up work to get back to normal setup

So.. I think this needs some risk assessment. If this is an urgent security fix of course we will do that. But if this is not that urgent (and the priority of medium seems to mean it's not? correct me if I'm wrong), then I'd suggest not doing all that.

Can we do this instead?

  • merge your simple change to the gui-deploy repo
  • let CI build content as it normally would
  • run puppet?

It would make everything sooo much simpler and the only difference is that someone can see what we are fixing (which we would do anyways, just a few minutes later?)

@sbassett What do you think? Can't we just merge the change and deploy the regular (public) way?

  • merge your simple change to the gui-deploy repo
  • let CI build content as it normally would

I think there’s still a misunderstanding here? My change against the gui-deploy repo replaces the regular CI build. If you push that to Gerrit, then you should be able to let Puppet pull the commit directly; but if you’re putting it on Gerrit anyways, you might as well upload the original change (T297686#7569330) to the gui (non-deploy) repo, merge it, and then trigger a rebuild of wikidata-query-gui-build, which should upload a new gui-deploy change, ready to merge. The gui-deploy change I rsynced to my deploy1002 home directory is mainly useful if you want to apply it directly to the git repository on whatever host serves the query service UI (miscweb1002?).

@Dzahn @Lucas_Werkmeister_WMDE - If a non-gerrit security deployment process is just too onerous, then I think the Security-Team would be fine with some well-timed merges in gerrit and a quick deploy. But obviously, once we disclose a security issue via gerrit or any other public environment, we want to deploy any mitigations to affected environments as quickly as possible, i.e. plan and coordinate.

@sbassett ACK, makes sense. thanks! I have talked to Lucas on IRC and, given that this task is medium prio and not UBN, he is out on vacation, the complications described above etc.. we have decided to not do anything right now but get back to it asap in January. (unless you say that's a problem and raise the priority to UBN, basically).

@sbassett ACK, makes sense. thanks! I have talked to Lucas on IRC and, given that this task is medium prio and not UBN, he is out on vacation, the complications described above etc.. we have decided to not do anything right now but get back to it asap in January. (unless you say that's a problem and raise the priority to UBN, basically).

Given the likely primary audience of users for WDQS and the fact that these payloads still require at least two interactions from the user (running the query and then clicking on at least one rendered element), and then may still not execute without a new target (via a Shift+) and may not make it past many modern browser's XSS protections, I think a medium risk that can wait until January sounds right, for now. Thanks.

Great, thanks for the assessment!

Okay, so if I understand it correctly, we have two options to deploy this change to query.wikidata.org:

  1. Change the Puppet git::clone from ensure => 'latest' to present, and manually git am the patch I rsynced to deploy1002. This fixes the bug without making it public, but practically blocks any other updates to the query service UI – I don’t think any WMDE developers even have the required access to wherever that Git clone lives. This was, I think, the main part of our objection to the earlier Puppet change in this direction, though I could live with it as a relatively short-term fix.
  2. Push the fix to Gerrit, let the wikidata-query-gui-build job push a corresponding change to the deploy repo, merge that, everything gets deployed as usual. This is simple, but makes the vulnerability public, including for third parties using the WDQS UI for their own Wikibase.

Ideally, I’d like to have a decision from the Wikibase Ecosystem PM (@Lea_WMDE) about whether option 2 is acceptable (probably depending on when we publish the next Wikibase Docker release which would include the fix – most of our users don’t get the Wikidata Query UI directly from Git). Unfortunately, Leszek says she’ll only be available from January 17th or so. Is it okay if we wait until then, and leave the issue unfixed for another week and a half? Or do you think we should pursue option 1 straight away?

Two more vulnerabilities came up where we should deploy fixes, see T298839 and T298871.

  1. Change the Puppet git::clone from ensure => 'latest' to present, and manually git am the patch I rsynced to deploy1002. This fixes the bug without making it public

Correction: the web host’s version of the deploy repo is public ({T294917} – https://query.wikidata.org/.git/config), so if we use this approach, the fix theoretically still becomes public, though only in a fairly hidden place.

Addshore changed the visibility from "Custom Policy" to "Public (No Login Required)".
Addshore changed the edit policy from "Custom Policy" to "All Users".
Addshore changed Risk Rating from Medium to N/A.