Last modified: 2014-01-14 10:38:05 UTC
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.
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?
(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.
(In reply to comment #2) > 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. 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?
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.
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.
Created attachment 13138 [details] Set Cache-control private when there is a session
Created attachment 13143 [details] 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.
Created attachment 13201 [details] 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)
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.
Adding Teles and Trijnstel since this may have caused an issue the stewards are investigating (bug 52975).
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
Created attachment 13777 [details] Patch for 1.21
Created attachment 13778 [details] Patch for 1.20
Created attachment 13779 [details] Patch for 1.19 Remove :? for 5.2 compatibility.
This issue was assigned CVE-2013-4572
Change 95546 merged by jenkins-bot: SECURITY: Don't cache when a call could autocreate https://gerrit.wikimedia.org/r/95546
Change 95543 merged by jenkins-bot: SECURITY: Don't cache when a call could autocreate https://gerrit.wikimedia.org/r/95543
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
Change 95558 merged by jenkins-bot: SECURITY: Don't cache when a call could autocreate https://gerrit.wikimedia.org/r/95558
Change 95539 merged by jenkins-bot: SECURITY: Don't cache when a call could autocreate https://gerrit.wikimedia.org/r/95539
No open patches to review here, hence restting status to RESOLVED FIXED.
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
Change 107303 merged by MarkAHershberger: SECURITY: Don't cache when a call could autocreate https://gerrit.wikimedia.org/r/107303
[Patch merged into REL1_22 branch; closing again]
