Last modified: 2012-03-22 19:40:26 UTC
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...
Created attachment 10192 [details] 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.
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.
Sounds like this should work from looking it over (untested)
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.
(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.
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)
Created attachment 10297 [details] Patch against 1.17
Created attachment 10298 [details] Patch against 1.18
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.
