Page MenuHomePhabricator

Inadequate CSRF protection in Comments API modules which perform write actions
Closed, ResolvedPublic

Description

Since version 4.0, committed on 19 January 2015 (Phabricator diff link), the Comments extension has used API modules instead of the old and nowadays-deprecated AJAX stuff ($wgAjaxExportList and all that). However, in this big commit, all anti-cross site request forgery (CSRF) protections present in the old AJAX code (see, for example, this old version of the Comments_AjaxFunctions.php file, which no longer exists and CTRL+F for matchEditToken) were removed, which makes said API modules vulnerable to CSRF.

Simplest possible test case:

  1. You are an admin/a comment admin/other privileged user in a group that has the commentadmin user right (say, on my Labs wiki, for example)
  2. While logged into this wiki, you visit some-malicious-site.tld which loads http://social-tools.wmflabs.org/w/api.php?action=commentdelete&commentID=1 in a hidden <iframe>
  3. And poof! Like magic, the first comment (comment which has the ID 1) from http://social-tools.wmflabs.org/wiki/Talk:Main_Page is now gone.

Now, I'm guessing the correct way to fix this is like how it was done with the VoteNY extension:

  1. define a needsToken method in the respective API modules (CommentBlock.api.php, CommentDelete.api.php, CommentSubmit.api.php, CommentVote.api.php)
  2. add a dependency on the mediawiki.api ResourceLoader module to the JS module in extension.json
  3. and thus make use of the handy mw.Api class and its postWithToken method in Comment.js

One problem with this approach is that it doesn't seem to work on 1.26, or at least on Brickipedia('s version of it). Brickipedia is running master versions of the Comments and VoteNY extensions, but trying to vote (on a page such as this) fails: a GET request is made to http://en.brickimedia.org/api.php?action=tokens&format=json&type=csrf and the returned response is:

{"warnings":{"tokens":{"*":"Unrecognized value for parameter 'type': csrf\naction=tokens has been deprecated. Please use action=query&meta=tokens instead."}},"tokens":{}}

This is, of course, annoying, but voting being broken is slightly less of an issue than commenting being broken. Brickipedia can live with the former for a while, but not with the latter. Comments is finally working nicely, so I'd rather not break it while fixing this issue. Brickipedia is also stuck on MW 1.26 for a while because the upgrade to 1.27 won't be possible before PHP is upgraded on the server, as 1.27 requires PHP 5.5.9 (according to the release notes) whereas Brickipedia currently has only 5.4.44.

cc @SamanthaNguyen (because this is relevant to Brickimedia's interests), @lcawte (ShoutWiki)

Event Timeline

I don't remember in what version this was changed, but the 'csrf' token used to be called the 'edit' token. Perhaps it'll work if you just change that. I think newer MediaWiki versions are backwards-compatible with the old name.

The mw.Api JavaScript module was updated to use action=query&meta=tokens in rMW298cf413dbc3: mediawiki.api: Use action=query&meta=tokens instead of action=tokens. Using 'edit' as the token name will work for 1.26, and should continue to work in 1.27 via the backwards-compatibility code included in that patch.

Bawolff triaged this task as Medium priority.Aug 31 2016, 7:16 PM
Bawolff moved this task from Backlog / Other to External (Non-WMF) Issues on the acl*security board.
dpatrick raised the priority of this task from Medium to High.Aug 31 2016, 7:17 PM

Proposed (and lightly tested, but only under MW 1.27) patch:

The { ajax: { cache: false } } shouldn't be needed, as POST requests shouldn't be cached by the browser or server.

Otherwise looks good.

ashley claimed this task.

Per MatmaRex's suggestion (on IRC), I removed the cache: false stuff from the mw.Api constructor calls and committed the patch to gerrit & merged it.

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 1 2016, 1:25 AM
Legoktm changed Security from Software security bug to None.