Last modified: 2014-09-24 01:32:54 UTC
MediaWiki doesn't generate a unique wpEditToken for anonymous users, so it's trivially easy to build a CSRF attack. PoC is in the URL. There are several ways to fix this: - Generate wpEditToken based on the user's IP address - Randomly generate a token and save it in the user's session - Randomly generate a token and save it in a database which regularly cleans out stale tokens I am not sure what the performance implications of each are, however. I can provide a patch once a decision is made.
This is currently done deliberately, as anonymous users have no special privileges and no guaranteed session state.
Sounds fair to me. However, this means that any website can build a form and then submit it on the behalf of an anonymous user using JavaScript; this doesn't sound very good. The IP method would work independently of session state, but the user would have to have the same IP from the retrieval request to the submit request.
I don't believe we can guarantee consistent IPs for a session from some ISPs, for instance AOL. I suppose we could do something like using an IP lock only if no cookie-based session is available... (we already open a session if possible when editing begins) Of course an IP address is a limited lock; many ISPs will have a huge number of users behind a single proxy IP (sometimes whole countries), so you'd still be open to CSRF for everybody within that group.
It sounds like a session/IP based lock would be good. The users that would be negatively affected by this change are highly-dynamic IP addresses that are blocking cookies; anyone else would simply just not have this protection. It seems to be a reasonable trade-off, although since Wikimedia is so big you never know. :-) I'll have a patch in some time soon.
Even a non-stale edit token for anons is not that hard to work around. A JavaScript could still request an edit token through XHR (grabbing it from the edit form or the API) and use that.
Roan, that would require an XSS vulnerability in MediaWiki (or elsewhere on the same host and port). At that point, you'd have the same problem with logged-in users. :)
Created attachment 4628 [details] Fixes issue This patch enables CSRF token generation for anonymous users. As it turns out, anonymous users did NOT get cookies when they went to action=edit, so Wiki.php was modified slightly to do that.
Marking patch and need-review.
Additional discussion with Tim Starling resulted in the following conclusions: - Assigning sessions to users that merely view an edit page will result in an unacceptably high amount of cache misses later on, due to the session cookie - A patch for Squid which makes Vary respect the presence or absence of header/token pairs will allow Squid to disregard some cookies when determining whether or not to serve a cached version - The anonymous edit token should be given its own cookie, which won't trigger a cache miss
(In reply to comment #6) > Roan, that would require an XSS vulnerability in MediaWiki (or elsewhere on the > same host and port). At that point, you'd have the same problem with logged-in > users. :) > Why would it? You'd have a JavaScript that uses AJAX to retrieve an edit token (the request will be sent by *the client*, as we use XHR), then use AJAX again to submit an edit using that token (again, XHR, so the request is sent by the same client, same IP and all that). This would require cookies being passed around, though, does XHR pass cookies? (In reply to comment #9) > - The anonymous edit token should be given its own cookie, which won't trigger > a cache miss Whoever implements this, please remember to update the token retrieval in ApiQueryInfo as well.
IP based checks are going to be a bit more complicated than originally planned. The primary trouble with them is that there's no convenient way to determine if the user has cookies disabled. Most of the usual approaches fail: - Checking if a session is initialized with session_id() fails because PHP will always know about the session even if the client rejects the cookie - Checking if a session cookie is present fails because, if the session was just initialized (which will be the case for all first-time anonymous users), no cookie will be present--we'd only have sent the cookies The latter method works, but exposes first-time users on the same IP to CSRF. (Is this that big of an issue?) Perhaps an ideal solution would be to send the cookie, and then do a Location: redirect to "refresh" (not truly, because otherwise we'd have an infinite loop) the page and then check if the cookie stuck. However, this means that all users with cookies disabled will end up double the number of page requests when they make edits (this may not be a big problem) and it will also complicate the controller logic. I'm working on an updated patch that plays nice with Tim Starling's proposed squid patch, but it won't have the IP check functionality. (In reply to comment #10) > Why would it? You'd have a JavaScript that uses AJAX to retrieve an edit token > (the request will be sent by *the client*, as we use XHR), then use AJAX again > to submit an edit using that token (again, XHR, so the request is sent by the > same client, same IP and all that). This would require cookies being passed > around, though, does XHR pass cookies? This is, in theory, impossible, because Cross-Site XHR is forbidden.
Created attachment 4635 [details] Updated patch taking Tim's Squid plans into account Here is the updated patch that sends anonymous users a "{$wgCookiePrefix}EditToken" cookie with a session long expiry. If the non-redirecting cookie check trigger for the IP field is used, we should probably extend this cookie's lifetime beyond a session. Sessions are NOT started when a user navigates to an edit page, with this patch. I could have also used the edit token cookie for logged in users, but decided against it to save bandwidth.
Edward, thank you for the patch. I'm very sorry, but in the time since you provided your patch, MediaWiki's codebase has changed enough that your patch no longer applies cleanly to trunk. Therefore I'm marking it obsolete and removing the "need-review" and "patch" keywords. If the issue is still one you're interested in fixing, please join us in IRC https://www.mediawiki.org/wiki/MediaWiki_on_IRC and talk with other developers before renewing and possibly revising your approach. Thanks for your interest in improving MediaWiki! And my apologies again on the long delay.
Comment on attachment 4635 [details] Updated patch taking Tim's Squid plans into account This patch no longer applies cleanly to MediaWiki's SVN trunk per Rusty Burchfield's automated testing. https://docs.google.com/spreadsheet/ccc?key=0Ah_71HHl7qa7dGtvSms3TGpHQU9NU2Y1VmNzUEUteWc
*** This bug has been marked as a duplicate of bug 38417 ***
