From 4d1ad32d8acbd443346253d2f6a95024c833295c Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Sat, 30 Jan 2016 10:54:24 -0500
Subject: [PATCH] Close a loophole in CookieSessionProvider

There's a crazy-small chance that someone could have a logged-out
session (e.g. by logging out or visiting a page that creates a session
despite being logged out), then the session expires, then someone else
logs in and gets the same session ID (which is about a 1 in a
quindecillion chance), then the first person comes in and picks up the
second person's session.

To avoid that, if there's no UserID cookie set (or the cookie value is
0) then indicate that the SessionInfo is for a logged-out user.

No idea if this is actually what happened in T125283, but it's worth
fixing anyway.

Bug: T125283
Change-Id: I44096c69aa7bd285e4e2472959e8d892200c5f2c
---
 includes/session/CookieSessionProvider.php    | 26 +++++++++++--------
 .../session/CookieSessionProviderTest.php     |  4 ++-
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/includes/session/CookieSessionProvider.php b/includes/session/CookieSessionProvider.php
index 2d01d1d0f4..f989cbc7f2 100644
--- a/includes/session/CookieSessionProvider.php
+++ b/includes/session/CookieSessionProvider.php
@@ -104,11 +104,14 @@ class CookieSessionProvider extends SessionProvider {
 
 	public function provideSessionInfo( WebRequest $request ) {
 		$info = array(
-			'id' => $this->getCookie( $request, $this->params['sessionName'], '' )
+			'id' => $this->getCookie( $request, $this->params['sessionName'], '' ),
+			'provider' => $this,
+			'forceHTTPS' => $this->getCookie( $request, 'forceHTTPS', '', false )
 		);
 		if ( !SessionManager::validateSessionId( $info['id'] ) ) {
 			unset( $info['id'] );
 		}
+		$info['persisted'] = isset( $info['id'] );
 
 		list( $userId, $userName, $token ) = $this->getUserInfoFromCookies( $request );
 		if ( $userId !== null ) {
@@ -128,21 +131,22 @@ class CookieSessionProvider extends SessionProvider {
 					return null;
 				}
 				$info['userInfo'] = $userInfo->verified();
-			} elseif ( isset( $info['id'] ) ) { // No point if no session ID
+			} elseif ( isset( $info['id'] ) ) {
 				$info['userInfo'] = $userInfo;
+			} else {
+				// No point in returning, loadSessionInfoFromStore() will
+				// reject it anyway.
+				return null;
 			}
-		}
-
-		if ( !$info ) {
+		} elseif ( isset( $info['id'] ) ) {
+			// No UserID cookie, so insist that the session is anonymous.
+			$info['userInfo'] = UserInfo::newAnonymous();
+		} else {
+			// No session ID and no user is the same as an empty session, so
+			// there's no point.
 			return null;
 		}
 
-		$info += array(
-			'provider' => $this,
-			'persisted' => isset( $info['id'] ),
-			'forceHTTPS' => $this->getCookie( $request, 'forceHTTPS', '', false )
-		);
-
 		return new SessionInfo( $this->priority, $info );
 	}
 
diff --git a/tests/phpunit/includes/session/CookieSessionProviderTest.php b/tests/phpunit/includes/session/CookieSessionProviderTest.php
index 2b7e0a1372..e5df458802 100644
--- a/tests/phpunit/includes/session/CookieSessionProviderTest.php
+++ b/tests/phpunit/includes/session/CookieSessionProviderTest.php
@@ -184,7 +184,9 @@ class CookieSessionProviderTest extends MediaWikiTestCase {
 		$this->assertNotNull( $info );
 		$this->assertSame( $params['priority'], $info->getPriority() );
 		$this->assertSame( $sessionId, $info->getId() );
-		$this->assertNull( $info->getUserInfo() );
+		$this->assertNotNull( $info->getUserInfo() );
+		$this->assertSame( 0, $info->getUserInfo()->getId() );
+		$this->assertNull( $info->getUserInfo()->getName() );
 		$this->assertFalse( $info->forceHTTPS() );
 
 		// User, no session key
-- 
2.20.1