Page MenuHomePhabricator

API action=parse&prop=headhtml leaking user tokens and other private info in cross-origin requests (again)
Closed, ResolvedPublicSecurity


This is almost the same as T139570. Make sure you are logged to enwiki and aren't blocking third-party cookies, then switch to some non-WMF site, and try this in your browser console:

$.getJSON("", function(r) {console.log(r.parse.headhtml["*"]);})

The expected result is for patrolToken, watchToken, and csrfToken to equal +\, and the whole response to be indistinguishable from the response to the same request made without cookies.

The actual result is the full (40-digit) tokens for your account. There are also a few other pieces of private information there, such as your skin and language choices.

Event Timeline

Bisected, caused by rMW0d75fdb4f73d: Use CsrfTokenSet as CSRF token source. Probably the bug is caused by the change in ResourceLoaderUserOptionsModule.php, where it now uses RequestContext::getMain() instead of the local context, bypassing the limited context set up by the API in ApiMain::__construct() (search for 'lacksSameOriginSecurity').

sbassett moved this task from Incoming to Watching on the Security-Team board.
sbassett added a project: SecTeam-Processed.
sbassett added a subscriber: daniel.

Can we just revert the problematic patch?

Was introduced in wmf.14 and labeled as a "risky" patch, though it had a one line commit message, no Phabricator task, etc.

Revert against master, which applies cleanly to wmf.17:

Revert against wmf.16:

Unless there are any objections I'd like to deploy these tomorrow given that all CSRF protection is basically bypassable right now.

Alternatively we could revert just the problematic piece of a large patch. Tested locally that this is solving the problem:

Alternatively we could revert just the problematic piece of a large patch. Tested locally that this is solving the problem:

Are you planning to deploy these? They aren't really deployable, they're not patch files with commit metadata...

I'm also not really sold that the patch was appropriate in the first place, that it caused a security issue is just another factor on top.

@Legoktm @Pchelolo - Given the discussion above, the Security-Team would prefer a revert of the original change set and the completion of any relevant backports, as I don't think we have a great idea what, if any, additional security or performance issues may have been introduced by c697641. Once that has been completed, we'd recommend filing a bug and resubmitting the change set.

I deployed the reverts in production and verified that the provided POC no longer works. The master revert will be merged shortly.

Legoktm claimed this task.
Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".
Legoktm changed the edit policy from "Custom Policy" to "All Users".