Fix h1 reports 93809 and 93813

Session Fixation ---------------- The HackerOne users kaviya and Kamini Singh have independendltly reported that Revive Adserver was vulnerable to session fixation, by allowing arbitrary session identifiers to be forced and at the same time by not invalidating the existing session upon a successful authentication. Under some circumstances, that could have been an opportunity for an attacker to steal an authenticated sessions. A CVE-ID has been requested, but not assigned yet. CWE: CWE-384 CVSSv2: 7.8 (AV:N/AC:M/Au:N/C:C/I:P/A:N)
revive-adserver · Mar 1, 2016 · 4910365 · 4910365
1 parent 8479413
commit 4910365
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 20 deletions.
diff --git a/lib/OA/Auth.php b/lib/OA/Auth.php
@@ -86,6 +86,9 @@ function login($redirectCallback = null)
                 OA_Auth::restart($GLOBALS['strUsernameOrPasswordWrong']);
             }
 
+            // Regenerate session ID now
+            phpAds_SessionRegenerateId();
+
             return OA_Auth::getSessionData($doUser);
         }
 
@@ -190,7 +193,7 @@ function getFakeSessionData()
      */
     function restart($sMessage = '')
     {
-        $_COOKIE['sessionID'] = phpAds_SessionStart();
+        $_COOKIE['sessionID'] = phpAds_SessionRegenerateId();
         OA_Auth::displayLogin($sMessage, $_COOKIE['sessionID']);
     }
 

diff --git a/lib/OA/Upgrade/Login.php b/lib/OA/Upgrade/Login.php
@@ -25,7 +25,7 @@ class OA_Upgrade_Login
     function checkLogin()
     {
         // Clean up session
-        $GLOBALS['session'] = array();
+        phpAds_clearSession();
 
         // Detection needs to happen every time to make sure that database parameters are
         $oUpgrader = new OA_Upgrade();
@@ -93,6 +93,7 @@ function autoLogin()
             $doUser->joinAdd($doAUA);
             $doUser->find();
             if ($doUser->fetch()) {
+                phpAds_SessionRegenerateId();
                 phpAds_SessionDataRegister(OA_Auth::getSessionData($doUser));
                 phpAds_SessionDataStore();
             }
@@ -106,6 +107,8 @@ function _checkLoginNew()
         $aCredentials = $oPlugin->_getCredentials(false);
 
         if (!PEAR::isError($aCredentials)) {
+            phpAds_SessionRegenerateId();
+
             $doUser = $oPlugin->checkPassword($aCredentials['username'], $aCredentials['password']);
 
             if ($doUser) {
@@ -140,6 +143,8 @@ function _checkLoginOld($tableName, $agencySupport)
                 $aCredentials = $oPlugin->_getCredentials(false);
 
                 if (!PEAR::isError($aCredentials)) {
+                    phpAds_SessionRegenerateId();
+
                     if (strtolower($aPref['admin']) == strtolower($aCredentials['username']) &&
                         $aPref['admin_pw'] == md5($aCredentials['password']))
                     {

diff --git a/lib/max/Dal/Admin/Session.php b/lib/max/Dal/Admin/Session.php
@@ -76,10 +76,10 @@ function storeSerializedSession($serialized_session_data, $session_id)
         if ($doSession) {
             $doSession->sessiondata = $serialized_session_data;
             $doSession->update();
-        }
-        else {
+        } else {
             $doSession = OA_Dal::factoryDO('session');
-            $doSession->sessionid = $session_id;
+            // It's an md5, so 32 chars max
+            $doSession->sessionid = substr($session_id, 0, 32);
             $doSession->sessiondata = $serialized_session_data;
             $doSession->insert();
         }

diff --git a/www/admin/lib-sessions.inc.php b/www/admin/lib-sessions.inc.php
@@ -30,28 +30,29 @@
 function phpAds_SessionDataFetch()
 {
     global $session;
-    $dal = new MAX_Dal_Admin_Session();
 
     // Guard clause: Can't fetch a session without an ID
-	if (empty($_COOKIE['sessionID'])) {
+    if (empty($_COOKIE['sessionID']) || !preg_match('#^[0-9a-f]{32}$#D', $_COOKIE['sessionID'])) {
         return;
     }
 
+    $dal = new MAX_Dal_Admin_Session();
     $serialized_session = $dal->getSerializedSession($_COOKIE['sessionID']);
 
-    // This is required because 'sessionID' cookie is set to new during logout.
-    // According to comments in the file it is because some servers do not
-    // support setting cookies during redirect.
-    if (empty($serialized_session)) {
+    // Return if the session was not found (expired or forged)
+    if (!$serialized_session) {
         return;
     }
 
     $loaded_session = unserialize($serialized_session);
-	if (!$loaded_session) {
-        // XXX: Consider raising an error
+
+    // Or if it can't be unserialized and/or is not a session we started
+    if (empty($loaded_session['__authentic__'])) {
         return;
     }
-	$session = $loaded_session;
+
+    $session = $loaded_session;
+
     $dal->refreshSession($_COOKIE['sessionID']);
 }
 
@@ -75,21 +76,75 @@ function phpAds_SessionSetAdminCookie($name, $value)
 }
 
 /*-------------------------------------------------------*/
-/* Create a new sessionid                                */
+/* Start a new session                                   */
 /*-------------------------------------------------------*/
 
 function phpAds_SessionStart()
 {
 	global $session;
+
 	if (empty($_COOKIE['sessionID'])) {
-		$session = array();
-		$_COOKIE['sessionID'] = md5(uniqid('phpads', 1));
+		phpAds_clearSession();
+
+        $sessionId = phpAds_SessionGenerateId();
 
-        phpAds_SessionSetAdminCookie('sessionID', $_COOKIE['sessionID']);
+        $dal = new MAX_Dal_Admin_Session();
+        $dal->storeSerializedSession(serialize($session), $sessionId);
 	}
+
 	return $_COOKIE['sessionID'];
 }
 
+/*-------------------------------------------------------*/
+/* Generate a sessionid                                  */
+/*-------------------------------------------------------*/
+
+function phpAds_SessionGenerateId()
+{
+    $_COOKIE['sessionID'] = md5(uniqid('phpads', 1));
+
+    phpAds_SessionSetAdminCookie('sessionID', $_COOKIE['sessionID']);
+
+    return $_COOKIE['sessionID'];
+}
+
+/*-------------------------------------------------------*/
+/* Re-generate the sessionid                             */
+/*-------------------------------------------------------*/
+
+function phpAds_SessionRegenerateId()
+{
+    global $session;
+
+    $dal = new MAX_Dal_Admin_Session();
+
+    if (!empty($_COOKIE['sessionID'])) {
+        $dal->deleteSession($_COOKIE['sessionID']);
+    }
+
+    if (!empty($session['__authentic__'])) {
+        $sessionId = phpAds_SessionGenerateId();
+        $dal->storeSerializedSession(serialize($session), $sessionId);
+
+        return $sessionId;
+    }
+
+    unset($_COOKIE['sessionID']);
+
+    return phpAds_SessionStart();
+}
+
+/*-------------------------------------------------------*/
+/* Clear the session and mark it as authentic            */
+/*-------------------------------------------------------*/
+
+function phpAds_clearSession()
+{
+    $GLOBALS['session'] = array(
+        '__authentic__' => true,
+    );
+}
+
 /*-------------------------------------------------------*/
 /* Register the data in the session array                */
 /*-------------------------------------------------------*/

diff --git a/www/api/v1/xmlrpc/LogonServiceImpl.php b/www/api/v1/xmlrpc/LogonServiceImpl.php
@@ -93,7 +93,8 @@ function logon($username, $password, &$sessionId)
 
         $_POST['login'] = 'Login';
 
-        $_COOKIE['sessionID'] = uniqid('phpads', 1);
+        unset($_COOKIE['sessionID']);
+        phpAds_SessionStart();
         $_POST['phpAds_cookiecheck'] = $_COOKIE['sessionID'];
 
         $this->preInitSession();

diff --git a/www/api/v2/xmlrpc/LogonServiceImpl.php b/www/api/v2/xmlrpc/LogonServiceImpl.php
@@ -93,7 +93,8 @@ function logon($username, $password, &$sessionId)
 
         $_POST['login'] = 'Login';
 
-        $_COOKIE['sessionID'] = uniqid('phpads', 1);
+        unset($_COOKIE['sessionID']);
+        phpAds_SessionStart();
         $_POST['phpAds_cookiecheck'] = $_COOKIE['sessionID'];
 
         $this->preInitSession();