Last modified: 2014-09-24 01:32:54 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 12945 - CSRF for anonymous page edits
CSRF for anonymous page edits
Status: RESOLVED DUPLICATE of bug 38417
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
unspecified
All All
: Low normal (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-06 21:48 UTC by Edward Z. Yang
Modified: 2014-09-24 01:32 UTC (History)
5 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Fixes issue (2.64 KB, patch)
2008-02-07 02:58 UTC, Edward Z. Yang
Details
Updated patch taking Tim's Squid plans into account (1.88 KB, patch)
2008-02-09 04:57 UTC, Edward Z. Yang
Details

Description Edward Z. Yang 2008-02-06 21:48:46 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.
Comment 1 Brion Vibber 2008-02-06 21:50:46 UTC
This is currently done deliberately, as anonymous users have no special privileges and no guaranteed session state.
Comment 2 Edward Z. Yang 2008-02-06 21:53:07 UTC
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.
Comment 3 Brion Vibber 2008-02-06 21:57:10 UTC
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.
Comment 4 Edward Z. Yang 2008-02-06 22:01:20 UTC
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.
Comment 5 Roan Kattouw 2008-02-06 22:08:49 UTC
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.
Comment 6 Brion Vibber 2008-02-06 22:33:24 UTC
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. :)
Comment 7 Edward Z. Yang 2008-02-07 02:58:41 UTC
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.
Comment 8 Edward Z. Yang 2008-02-07 02:59:16 UTC
Marking patch and need-review.
Comment 9 Edward Z. Yang 2008-02-07 05:54:38 UTC
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
Comment 10 Roan Kattouw 2008-02-07 12:02:38 UTC
(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.

Comment 11 Edward Z. Yang 2008-02-09 04:14:09 UTC
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.
Comment 12 Edward Z. Yang 2008-02-09 04:57:38 UTC
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.
Comment 13 Sumana Harihareswara 2012-01-10 22:45:00 UTC
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 14 Sumana Harihareswara 2012-01-10 22:45:33 UTC
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
Comment 15 Tyler Romeo 2013-06-11 03:49:54 UTC

*** This bug has been marked as a duplicate of bug 38417 ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links