Page MenuHomePhabricator

Special:MergeHistory doesn't verify the edit token (vulnerable to CSRF)
Closed, ResolvedPublic

Description

Special:MergeHistory doesn't verify the edit token (it's vulnerable to CSRF). This is a regression from rMW598068334e72: Convert Special:MergeHistory to use OOUI. (https://gerrit.wikimedia.org/r/#/c/287435/). The simplest fix is to revert that patch. Somebody should also check Sethakill's other commits, since he did several similar OOUI conversions.

Event Timeline

matmarex created this task.Jun 21 2016, 7:56 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 21 2016, 7:56 PM

(I'll note that merging page histories can be very disruptive and is difficult to undo.)

dpatrick triaged this task as High priority.Jun 21 2016, 8:07 PM

There doesn't seem to be a fix deployed still (or at least not a revert, I didn't try testing this in production), so perhaps I should just submit a public revert and have it SWAT-ted on Monday?

Somebody should also check Sethakill's other commits, since he did several similar OOUI conversions.

The only other write special pages they converted were Unlockdb/Lockdb which was already using HTMLForm and looks fine.

There doesn't seem to be a fix deployed still (or at least not a revert, I didn't try testing this in production), so perhaps I should just submit a public revert and have it SWAT-ted on Monday?

There are also some code quality issues in the new Special:MergeHistory so overall a revert seems sensible.

matmarex claimed this task.Jul 5 2016, 10:02 AM

1.28.0-wmf.8 version:

Deployed to wmf.8 by @thcipriani. Could anyone from Security confirm that it is okay to make the task public, commit the patch to master and close the task? (@dpatrick @Bawolff?)

mmodell added a subscriber: mmodell.Jul 5 2016, 8:19 PM

If it's being reverted then can't we just revert on master and avoid having to maintain a messy security patch on each new branch?

The patch did not apply cleanly to wmf.9, though I was able to fix the conflict. Still it would be much more straightforward to just revert on master.

Anomie added a subscriber: Anomie.Jul 5 2016, 8:33 PM

When it's a security bug that affects WMF wikis, we want the wikis fixed before we make the bug public by putting the patch in Gerrit. In this case, since the bug wasn't in a released version (1.27 or earlier) we can probably make it public now instead of having to wait for the next security release, but we usually wait for the Security team to make the final decision.

Reverting such a large change is probably pretty noticeable. I might argue that the presence of this patch on live wikis counts as a public disclosure ;)

I'm not intending to dispute the process though. Hopefully Security will respond before this patch lives on for another week.

It's fine to go ahead and make this public and commit to Gerrit. Thanks for your work on this everyone.

I wanted to have the patch made public and committed to master immediately after deploying it, but @thcipriani and I were not comfortable making a security patch public by ourselves, and no one from the security team was available (and no one responded to my question here in T138346#2404165). @mmodell the first patch I posted here applies cleanly to wmf.9 and master (T138346#2428732).

For the future, @dpatrick, what do I need to do to have security issues like this resolved more quickly, before people merge conflicting changes to the same code? It's kind of sad that no one picked this up for two weeks until I scheduled a SWAT deployment for it…

matmarex changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 5 2016, 11:34 PM
matmarex changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptJul 5 2016, 11:34 PM

Change 297544 had a related patch set uploaded (by Bartosz Dziewoński):
Revert "Convert Special:MergeHistory to use OOUI."

https://gerrit.wikimedia.org/r/297544

Change 297544 merged by jenkins-bot:
Revert "Convert Special:MergeHistory to use OOUI."

https://gerrit.wikimedia.org/r/297544

For the future, @dpatrick, what do I need to do to have security issues like this resolved more quickly, before people merge conflicting changes to the same code? It's kind of sad that no one picked this up for two weeks until I scheduled a SWAT deployment for it…

@matmarex, you can feel free to contact me on IRC, Google Hangouts/GChat, or direct e-mail (rather than a Phab ticket; I get a lot of Phab e-mail, filter it, and look at it less frequently throughout the day than I do non-Phab messages). As for two weeks passing from the opening of the bug until its resolution, unfortunately, this happened during a busy period of time, with scheduled travel for Wikimania, scheduled time off, and reduced capacity on the Security team. I apologize if this put you (all) in a tight spot/created more work for you.

Thanks, I see. To be honest, it couldn't have taken more than an hour or two of my time (and I guess another hour for the deployers). Mostly I'm worried that there didn't seem to be anyone taking responsibility for this. But it's true that there were many things happening at the same time.