Page MenuHomePhabricator

Use token when logging out
Closed, ResolvedPublic

Description

Including Special:Userlogout and api.php?action=logout

Details

Reference
bz23227

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

No, it's probably better not to introduce another token type when 'csrf' should do nicely.

Since this issue is already publicly known (there was that log-me-out-of-everything site that hit hacker news a while back), and I think the approach looks right, should we merge this publicly in gerrit?

Well, it will break anything that's actually using API action=logout in such a way that the user won't be logged out. And might not even be doing error checking on that. We should probably look how many "anythings" actually use it before merging. @bd808 might be able to give us some numbers.

I can't get great numbers on the use of action=logout from the current web-requests dataset in Hive because we don't record the POST payload data. Here's what I did find:

hive (wmf)> SELECT
          >     http_method,
          >     COUNT(*)
          > FROM wmf.webrequest
          > WHERE year = 2016
          >   AND month = 2
          >   AND day = 25
          >   AND uri_path = '/w/api.php'
          >   AND uri_query LIKE '%action=logout%'
          > GROUP BY http_method;
... lots of waiting ...
Total MapReduce CPU Time Spent: 1 days 9 hours 1 minutes 32 seconds 550 msec
OK
http_method     _c1
GET     11471
POST    178

I can't get great numbers on the use of action=logout from the current web-requests dataset in Hive because we don't record the POST payload data. Here's what I did find:

I forgot the new logging stuff isn't done yet. :(

I've started a grep on fluorine over api.log for the past week. Someone remind me in a day or two to see what the results are.

Anomie added a comment.Mar 2 2016, 9:23 PM

I've started a grep on fluorine over api.log for the past week. Someone remind me in a day or two to see what the results are.

Over the one-week period sampled, there were 89898 hits to action=logout. Since the log entry happens after the logout is processed, the log does not contain the usernames.

79920 of these were on zerowiki from 2620:0:860::/46, so it's probably some Wikipedia Zero thing.

6887 of the remaining hits came from just one Tool Labs IP, sent to a variety of wikis. Considering the low number of hits from other Tool Labs IPs, this is probably mostly due to just one tool.

Two unrelated-looking external IPs account for 1108 and 1167 of the rest of the hits. The remaining 816 hits come from 74 different IPs.

Bawolff merged a task: Restricted Task.May 23 2017, 5:03 PM
Bawolff added a subscriber: Dhirajindexes.
Bawolff merged a task: Restricted Task.Jul 19 2018, 8:02 PM
Bawolff added a subscriber: Ladsgroup.

Hey,
I just want to point out that this vulnerability is quite important (An attacker can paralyze a wiki by putting the logout URL as background image in common.css, and fixing it is almost impossible) and it has been reported 9 years ago. Can this be addressed? Count me in if you need any help. @JBennett @chasemp @Reedy @sbassett @Bawolff

Well, it will break anything that's actually using API action=logout in such a way that the user won't be logged out. And might not even be doing error checking on that. We should probably look how many "anythings" actually use it before merging. @bd808 might be able to give us some numbers.

@Ladsgroup I guess the question is how we address this. At this point, we need to be looking at stats again, and we definitely need to get some announcements written up and publicised before

Reedy updated the task description. (Show Details)
Reedy removed a subscriber: RobLa-WMF.

Well, it will break anything that's actually using API action=logout in such a way that the user won't be logged out. And might not even be doing error checking on that. We should probably look how many "anythings" actually use it before merging. @bd808 might be able to give us some numbers.

@Ladsgroup I guess the question is how we address this. At this point, we need to be looking at stats again, and we definitely need to get some announcements written up and publicised before

I can make the patches ready for common libraries like pywikibot and mwapi (if it uses logout) and push it once it's deployed.

And @Bawolff's patch will need a rebase as it's nearly 3 years old at this point

Anomie added a comment.EditedJan 23 2019, 4:28 PM

At this point, we need to be looking at stats again

Much easier now that https://wikitech.wikimedia.org/wiki/Analytics/Data_Lake/Traffic/ApiAction exists.

Looking at January 2019, usage patterns seem largely unchanged. With the Data Lake I can see agents too, which allows for a bit more in-depth analysis.

  • 87301 are hits to zerowiki from WMF IPs.
  • 12054 come from one Toolforge exec node with one agent (belonging to a 3-year-deprecated library).
  • 1572 hits come from various IPs with agents like "Wikipedia/#.#.# (iSomething; iOS #.#.#; Scale/#.00)", for example "Wikipedia/6.1.4 (iPhone; iOS 12.1.2; Scale/3.00)".
  • 1108 and 558 of the hits from from two unrelated-looking IPs, each with its own library-like agent. No idea if these are the same IPs as in T25227#2081647.
  • 302, 177, and 73 hits come from various Toolforge IPs with agents that identify specific tools or users.
  • 125 hits from various external IPs with an agent matching the name of a plugin for a particular app.
  • 123 hits from one external IP with a bot-like agent.
  • 141 hits come from a pair of external IPs in a university network, with 99 different browser-like agents. Normally this would seem like a proxy, but since normal browsing doesn't usually hit API action=logout it's more likely a bot of some sort with faked agents.
  • The remaining 277 hits come from 131 different external IPs and 79 different agents. 29 of the agents (76 hits) begin with "Mozilla/".

There are two ways we could proceed here to make the change requested in this task with respect to the API:

  1. Treat it as a security issue: break all those API clients immediately, as is done by F3328897.
  2. Treat it as a normal change: allow but don't require token use in 1.33, with a deprecation warning if the token isn't used, and break the clients in 1.34.

Since this has been open for about 9 years and seems at least somewhat known already, I'm not sure #1 is the obvious choice. But I'll leave the decision to the Security team.

There are two ways we could proceed here to make the change requested in this task with respect to the API:

  1. Treat it as a security issue: break all those API clients immediately, as is done by F3328897.
  2. Treat it as a normal change: allow but don't require token use in 1.33, with a deprecation warning if the token isn't used, and break the clients in 1.34.

    Since this has been open for about 9 years and seems at least somewhat known already, I'm not sure #1 is the obvious choice. But I'll leave the decision to the Security team.

Of course the security team has the last call here but my personal preference is #1. For several reasons:

  • This ticket is private, it seems there is consensus that the issue should not be public.
  • If it's not exploited yet, it doesn't mean it's a small issue. I was telling my friends the fact that this issue is not abused yet restores my faith in humanity.
  • Given the #flea-attacker-mitigation we have, It seems too dangerous to leave it as it is and expose the issue. When there is someone who actively looking for issues.

(An attacker can paralyze a wiki by putting the logout URL as background image in common.css, and fixing it is almost impossible)

An attacker who can edit site CSS, would also be able to edit site JS where they could get a token to get around any fix. But I'm sure there are plenty of other disruptive ways to exploit this from less privileged positions.

Just to argue the other side for a moment: An argument could be made that if we're not sure if a user is supposed to be logged in or not, it is safer to fail in the direction of logging the user out than keeping the user logged in. Not sure if I'm convinced by that argument, but thought i should throw it out there.

Legoktm merged a task: Restricted Task.Apr 14 2019, 8:18 PM
Legoktm added a subscriber: Jorm.
Reedy changed the visibility from "Custom Policy" to "Custom Policy".Apr 14 2019, 8:40 PM
Reedy changed the edit policy from "Custom Policy" to "Custom Policy".

This open security issue has passed its ninth anniversary five days ago.

Jorm added a comment.Apr 15 2019, 3:55 PM

Folks, seriously: just fix this. Let the bots break for a day or five. You don't need community consensus. You just need the Will To Do It.

"Rebased" @Bawolff's 3+ year-old patch (T25227#2013640) on master, tested locally. Talked about this with the Security-Team today - fine with just pushing it publicly in gerrit. If there are no objections, I'll create a patch set from the attached: F28682008.

"Rebased" @Bawolff's 3+ year-old patch (T25227#2013640) on master, tested locally. Talked about this with the Security-Team today - fine with just pushing it publicly in gerrit. If there are no objections, I'll create a patch set from the attached: F28682008.

Awesome! thanks. One thing in the patch, it uses the old array() system. That's my only note. Once it's in gerrit, add me as reviewer so I help (btw. Can we write tests for this?)

Reedy added a comment.Apr 17 2019, 9:35 AM

"Rebased" @Bawolff's 3+ year-old patch (T25227#2013640) on master, tested locally. Talked about this with the Security-Team today - fine with just pushing it publicly in gerrit. If there are no objections, I'll create a patch set from the attached: F28682008.

Awesome! thanks. One thing in the patch, it uses the old array() system. That's my only note. Once it's in gerrit, add me as reviewer so I help (btw. Can we write tests for this?)

Old array would probably fail phpcs, so NBD

Feel free to write tests :)

Er, I think I fixed the array() => [] issues in F28682008. Anyhow, I'll plan to make this task public today, push a patch set up to gerrit and work on some tests.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 17 2019, 2:23 PM
sbassett removed subscribers: Parent5446, csteipp.

Change 504565 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/core@master] [PATCH] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 504565 merged by jenkins-bot:
[mediawiki/core@master] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506386 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Turn logout link into a POST API call with refresh

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

Johan added a subscriber: Johan.Thu, Apr 25, 12:11 PM

So will this be in production before or after Tech News goes out on Monday?

Wikimedia wikis now use a token when you log out. This changes how the API works. Some tools might need to be updated. LINK

Something like that?

So will this be in production before or after Tech News goes out on Monday?

After

Wikimedia wikis now use a token when you log out. This changes how the API works. Some tools might need to be updated. LINK

Something like that?

Sounds good to me. There might be another change that will go live and affects how users log out but it's too soon for that.

Change 506421 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] ApiLogout: Follow up Icb674095

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

Change 506429 had a related patch set uploaded (by Reedy; owner: SBassett):
[mediawiki/core@REL1_33] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506430 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_33] ApiLogout: Follow up Icb674095

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

Change 506431 had a related patch set uploaded (by Reedy; owner: SBassett):
[mediawiki/core@REL1_32] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506432 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_32] ApiLogout: Follow up Icb674095

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

Change 506433 had a related patch set uploaded (by Reedy; owner: SBassett):
[mediawiki/core@REL1_31] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506434 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_31] ApiLogout: Follow up Icb674095

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

Change 506438 had a related patch set uploaded (by Reedy; owner: SBassett):
[mediawiki/core@REL1_30] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506439 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_30] ApiLogout: Follow up Icb674095

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

Change 506438 merged by Reedy:
[mediawiki/core@REL1_30] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506448 had a related patch set uploaded (by Reedy; owner: SBassett):
[mediawiki/core@REL1_27] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506449 had a related patch set uploaded (by Reedy; owner: Anomie):
[mediawiki/core@REL1_27] ApiLogout: Follow up Icb674095

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

Change 506439 merged by Reedy:
[mediawiki/core@REL1_30] ApiLogout: Follow up Icb674095

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

Change 506431 merged by jenkins-bot:
[mediawiki/core@REL1_32] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506432 merged by jenkins-bot:
[mediawiki/core@REL1_32] ApiLogout: Follow up Icb674095

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

Change 506433 merged by jenkins-bot:
[mediawiki/core@REL1_31] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506434 merged by jenkins-bot:
[mediawiki/core@REL1_31] ApiLogout: Follow up Icb674095

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

Change 506429 merged by jenkins-bot:
[mediawiki/core@REL1_33] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506430 merged by jenkins-bot:
[mediawiki/core@REL1_33] ApiLogout: Follow up Icb674095

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

Change 506421 merged by jenkins-bot:
[mediawiki/core@master] ApiLogout: Follow up Icb674095

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

Change 506448 merged by SBassett:
[mediawiki/core@REL1_27] [SECURITY] [API BREAKING CHANGE] Require logout token.

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

Change 506449 merged by SBassett:
[mediawiki/core@REL1_27] ApiLogout: Follow up Icb674095

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

Reedy added a parent task: Restricted Task.Tue, Apr 30, 3:54 PM

Can this task be closed?

Kindly suggest, was security researcher who reported this vulnerability was added in HoF page?

Reedy added a comment.Sat, May 4, 3:16 PM

Kindly suggest, was security researcher who reported this vulnerability was added in HoF page?

The original reporter was @liangent who is a community member... Followed by 5 duplicates... You weren't the original reporter

Reedy added a comment.Sat, May 4, 3:21 PM

Can this task be closed?

This patch is still open atm

Change 506386 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Turn logout link into a POST API call with refresh

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

This patch is still open atm

I would be very happy to move this patch to another ticket. My goal is to remove any GET request that involves token but one can argue it's not part of this ticket.

sbassett closed this task as Resolved.Mon, May 6, 3:14 PM
sbassett removed a project: Patch-For-Review.
sbassett claimed this task.

@Framawiki, @Ladsgroup, @Reedy - split "Turn logout link into a POST API call with refresh" and moved here: T222626. @Ladsgroup - will let you update patch set with new bug id. Resolving this task for now.