Multiple users with the same session ID
Closed, ResolvedPublic

Description

It was reported that multiple meta.wikimedia.org users received the same session ID. These users could act as the user who happened to be logged in according to the session value. It seems that Varnish may have cached some responses with Set-Cookie headers.

Clearing the session store and avoiding the Varnish cache by switching from Varnish to Squid did not rectify the situation. The session value which was the subject of the report was immediately recreated with the same username. My best theory on the cause of this is that the session was recreated by User::loadFromSession() due to a persistent token cookie.

Although we have no more reports, it's likely that more than one session ID was affected. So, to force regeneration of all session IDs, I am proposing to change the name of the session cookie.


Version: unspecified
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=57906

bzimport set Reference to bz53032.
tstarling created this task.Via LegacyAug 19 2013, 11:21 AM
csteipp added a comment.Via ConduitAug 20 2013, 4:08 AM

The token cookie is specifically varied on in OutputPage::getCacheVaryCookies, which should be called as long as OutputPage::sendCacheControl is called. OutputPage::output always calls sendCacheControl, so in most cases, we should be doing the right thing.

There are a couple pages that disable the OutputPage handling, and may not be setting the cache-control headers correctly (some don't set any headers, other set their own CC headers):

  • specials/SpecialExport.php
  • specials/SpecialUploadStash.php
  • specials/SpecialUndelete.php
  • specials/SpecialRevisiondelete.php
  • AjaxResponse.php

Due to bug 52975, I'm wondering if SpecialUploadStash might be the issue?

Since Tim changed the local wiki session name, I'm also assuming the issue was with the local wiki's token or session cookie, and not CentralAuth's? Is there a way we can confirm that?

tstarling added a comment.Via ConduitAug 20 2013, 5:30 AM

(In reply to comment #1)

Since Tim changed the local wiki session name, I'm also assuming the issue
was with the local wiki's token or session cookie, and not CentralAuth's? Is
there a way we can confirm that?

What we know is:

  • Two users who were not Bjankuloski06 reported being logged in to metawiki as Bjankuloski06
  • One of those users (Russavia) supplied a full trace of request/response headers for metawiki. The cookie header was:

centralauth_User=Russavia; vector-nav-p-coll-print_export=true;
metawiki_session=...;
uls-previous-languages=%5B%22zh-hant%22%2C%22en%22%2C%22zh-hans%22%5D;
vector-nav-p-tb=true

  • The session data corresponding to the metawiki_session was a valid login for Bjankuloski06
  • Bjankuloski06 had no globaluser row, which rules out a few possible CentralAuth connections.
  • Deleting the affected session data led to recreation of a valid Bjankuloski06 session after a minute or two.
  • Invalidating Bjankuloski06's sessions by resetting the user_token field and then clearing the session led to the session being recreated as a logged-out user, with ChronologyProtector data updated at least once every second or so.
  • Using a packet sniffer (httpry), Mark Bergsma discovered responses coming out of MediaWiki with a Set-Cookie header for the local session, with Cache-Control: public, s-maxage=600, max-age=0. Mark also identified an issue with the Varnish configuration which would allow such responses to be cached.

So you can see why we were led to believe that the local session was at fault.

Note that caching of responses with a Set-Cookie header is recommended by RFC 2109. They are usually not cached, but this is just a common hack rather than a standard. So there is definitely a MediaWiki bug here.

RobLa-WMF added a comment.Via ConduitAug 20 2013, 6:42 PM

(In reply to comment #2)

Note that caching of responses with a Set-Cookie header is recommended by RFC

  1. They are usually not cached, but this is just a common hack rather than a standard. So there is definitely a MediaWiki bug here.

Shouldn't the Cache-control header have no-cache="set-cookie"? RFC 2109 says "The Set-Cookie header should usually not be cached" and "If the cookie is intended for use by a single user, the Set-cookie header should not be cached". However, it allows for the possibility that cookies can be cached if not specifically instructed otherwise.

Presumably, it's up to MediaWiki to set Cache-control: no-cache="set-cookie", right?

csteipp added a comment.Via ConduitAug 20 2013, 7:10 PM

Thanks! There are only a few scripts that output a Cache-Control header that's public with the s-maxage and max-age in that order. CentralNotice does that in SpecialBannerLoader.php, SpecialCNReporter.php, and SpecialBannerRandom.php.

I've also been testing autocreate, and you do get back local-wiki session cookie when your CentralAuth user is autocreated on a new wiki.

So that, combined with Mark mentioning the majority of cases he saw were on meta, where I think we server most of CentralNotice banners, makes that seem like a good candidate for causing this.

csteipp added a comment.Via ConduitAug 20 2013, 9:30 PM

Yep, just confirmed it for a call to Special:BannerRandom that autocreates a user on the CentralNotice wiki, there is a Set-Cookie header to set the session cookie, and Cache-Control is "public, s-maxage=600, max-age=0". I'm guessing that's the majority of the issue.

I'll attach a patch for CentralNotice that sets the CC to private if there is a logged in user, which appears to fix the issue (in my dev setup, but I'll need Mark to confirm this will work for our setup).

While digging into this, we have several other places in the code where a script sets their own headers, so this may need to be fixed in a few other places.

csteipp added a comment.Via ConduitAug 20 2013, 9:32 PM

Created attachment 13138
Set Cache-control private when there is a session

attachment bug53032-centralnotice.patch ignored as obsolete

bzimport added a comment.Via ConduitAug 21 2013, 12:53 AM

mwalker wrote:

Slightly different CentralNotice patch (does not cache for logged in)

For CentralNotice it makes more sense to just not set CC headers at all for logged in users. My patch does that.

Attached: 0001-Ensure-requests-are-not-cached-with-session-data.patch

csteipp added a comment.Via ConduitAug 30 2013, 6:47 PM

Created attachment 13201
patch for core

A few places in core were outputing headers that could allow cookies to be cached:

  • When action=raw is used (which mark identified was happening in production)
  • When stashed images were accessed (probably couldn't trigger the issue, but we shouldn't cache these private images)

Attached: bug53032-core.patch

csteipp added a comment.Via ConduitSep 3 2013, 7:26 PM

Mark was ok with the patch, but I haven't been able to deploy the patch yet, so we'll make both the core and central notice patches public with the 1.21.3 release later this month.

csteipp added a comment.Via ConduitSep 9 2013, 4:44 PM

Adding Teles and Trijnstel since this may have caused an issue the stewards are investigating (bug 52975).

csteipp added a comment.Via ConduitSep 9 2013, 5:52 PM

The core patch was deployed on Sept 5th to wmf16, so it will be on all wikis by Sept 12th.

22:44 logmsgbot: csteipp synchronized php-1.22wmf16/includes/specials
22:43 logmsgbot: csteipp synchronized php-1.22wmf16/includes/actions

csteipp added a comment.Via ConduitNov 13 2013, 12:34 AM

Created attachment 13777
Patch for 1.21

Attached: bug53032-core-121.patch

csteipp added a comment.Via ConduitNov 13 2013, 12:35 AM

Created attachment 13778
Patch for 1.20

Attached: bug53032-core-120.patch

csteipp added a comment.Via ConduitNov 13 2013, 12:36 AM

Created attachment 13779
Patch for 1.19

Remove :? for 5.2 compatibility.

Attached: bug53032-core-119.patch

csteipp added a comment.Via ConduitNov 14 2013, 7:32 PM

This issue was assigned CVE-2013-4572

gerritbot added a comment.Via ConduitNov 14 2013, 10:13 PM

Change 95546 merged by jenkins-bot:
SECURITY: Don't cache when a call could autocreate

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

gerritbot added a comment.Via ConduitNov 14 2013, 10:21 PM

Change 95543 merged by jenkins-bot:
SECURITY: Don't cache when a call could autocreate

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

gerritbot added a comment.Via ConduitNov 14 2013, 10:37 PM

Change 95558 had a related patch set uploaded by CSteipp:
SECURITY: Don't cache when a call could autocreate

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

gerritbot added a comment.Via ConduitNov 14 2013, 10:51 PM

Change 95558 merged by jenkins-bot:
SECURITY: Don't cache when a call could autocreate

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

gerritbot added a comment.Via ConduitNov 14 2013, 10:52 PM

Change 95539 merged by jenkins-bot:
SECURITY: Don't cache when a call could autocreate

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

Aklapper added a comment.Via ConduitNov 15 2013, 11:00 AM

No open patches to review here, hence restting status to RESOLVED FIXED.

gerritbot added a comment.Via ConduitJan 14 2014, 1:49 AM

Change 107303 had a related patch set uploaded by CSteipp:
SECURITY: Don't cache when a call could autocreate

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

gerritbot added a comment.Via ConduitJan 14 2014, 1:51 AM

Change 107303 merged by MarkAHershberger:
SECURITY: Don't cache when a call could autocreate

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

Aklapper added a comment.Via ConduitJan 14 2014, 10:38 AM

[Patch merged into REL1_22 branch; closing again]

csteipp added a project: Security.Via WebMar 26 2015, 8:39 PM

Add Comment