Issues

ZF-3378: Zend_Session allow invalid session ids

Description

this are my options (php.ini):


; Argument passed to save_handler.  In the case of files, this is the path
; where data files are stored. Note: Windows users have to change this
; variable in order to use PHP's session functions.
;
; As of PHP 4.0.1, you can define the path as:
;
;     session.save_path = "N;/path"
;
; where N is an integer.  Instead of storing all the session files in
; /path, what this will do is use subdirectories N-levels deep, and
; store the session data in those directories.  This is useful if you
; or your OS have problems with lots of files in one directory, and is
; a more efficient layout for servers that handle lots of sessions.
;
; NOTE 1: PHP will not create this directory structure automatically.
;         You can use the script in the ext/session dir for that purpose.
; NOTE 2: See the section on garbage collection below if you choose to
;         use subdirectories for session storage
;
; The file storage module creates files using mode 600 by default.
; You can change that by using
;
session.save_path = "2;666;/var/sessions/php"

; Define how many bits are stored in each character when converting
; the binary hash data to something readable.
;
; 4 bits: 0-9, a-f
; 5 bits: 0-9, a-v
; 6 bits: 0-9, a-z, A-Z, "-", ","
session.hash_bits_per_character = 5

In the path "/var/sessions/php" all directories (0-9 + a-v) and are created

The problem is - If the user set the session id like "?PHPSESSID=xxx" I can't regenerate the session id because session_start() was called before.


try {
    Zend_Session::start();
} catch (Exception $e) {
    // echo $e->getMessage(); -> Zend_Session::start() - session_start() [function.session-start]: open(/var/sessions/php/x/x/sess_xxx, O_RDWR) failed: No such file or directory (2)
    Zend_Session::regenerateId();
    // echo session_id(); // -> xxx
    Zend_Session::setId('test'); -> Exception: The session has already been started. The session id must be set first.
}

I think it is usefull to test the current given session id before session_start will call like:


$hashBitsPerChar = ini_get('session.hash_bits_per_character');
if (!$hashBitsPerChar) {
    $hashBitsPerChar = 5; // the default value
}
switch($hashBitsPerChar) {
    case 4: $pattern = '^[0-9a-f]*$'; break;
    case 5: $pattern = '^[0-9a-v]*$'; break;
    case 6: $pattern = '^[0-9a-zA-Z-,]*$'; break;
}
if ( !preg_match('#'.$pattern.'#', session_id()) ) {
    throw new Zend_Session_Exception('Invalid session id "'.session_id.'"');
}

Comments

Actually, it's much worse...actually a big security issue.

If the session cookie is already set before a user has logged in, Zend_Session will use this value when creating a new session. The exploit scenario is: You have a public computer somewhere, and you know that a php application is used by alot of people. Using Firefox it's pretty easy to delete and set cookies. You set a PHPSESSID cookie, and note down the value you use. (using it later to do the exploit)

Then, when someone comes and logs in to the php application, your already set cookie will be used. This only works of course if the browser isn't closed and restarted in the mean time.

The solution is that Zend_Session always should check if the session referred to by the PHPSESSID actually exists, and if not always regenerate the sessionid before creating a new session.

This isn't really a Zend_Session issue alone, since php native sessions are also affected. But Zend should have a workaround for this I think.

If an invalid session ID is provided to PHP before {{Zend_Session::start()}} is run, then {{Zend_Session}} will continue to use it as it doesn't automatically regenerate session IDs on start. So, this unit test fails against trunk:


Index: tests/Zend/Session/SessionTest.php
===================================================================
--- tests/Zend/Session/SessionTest.php  (revision 24490)
+++ tests/Zend/Session/SessionTest.php  (working copy)
@@ -1062,4 +1062,51 @@
         }
     }

+    /**
+     * @group ZF-3378
+     */
+    public function testInvalidPreexistingSessionIdDoesNotPreventRegenerationOfSid()
+    {
+        // Pattern: [0-9a-v]*
+        ini_set('session.hash_bits_per_character', 5);
+
+        // Session store
+        $sessionCharSet = array_merge(range(0,9), range('a','v'));
+        $sessionStore = dirname(__FILE__)
+                      . DIRECTORY_SEPARATOR . "_files"
+                      . DIRECTORY_SEPARATOR . "ZF-3378";
+        if ( !is_dir($sessionStore) ) @mkdir($sessionStore, 0755, true);
+        ini_set('session.save_path', "1;666;" . $sessionStore);
+
+        // When using subdirs for session.save_path, the directory structure
+        // is your own responsibility...set it up, or else bad things happen
+        foreach ( $sessionCharSet as $subdir ) {
+            @mkdir($sessionStore . DIRECTORY_SEPARATOR . $subdir);
+        }
+
+        // Set session ID to invalid value
+        session_id('xxx');
+
+        // Attempt to start the session
+        try {
+            /** @see Zend_Session */
+            require_once "Zend/Session.php";
+            Zend_Session::start();
+        } catch (Zend_Session_Exception $e) {
+            Zend_Session::regenerateId();
+        }
+        // Get the current SID
+        $sid = Zend_Session::getId();
+
+        // We don't need the session any more, clean it up
+        Zend_Session::destroy();
+        foreach ( $sessionCharSet as $subdir ) {
+            @rmdir($sessionStore . DIRECTORY_SEPARATOR . $subdir);
+        }
+        @rmdir($sessionStore);
+
+        // Check the result
+        $this->assertRegExp('/^[0-9a-v]+$/', $sid);
+        $this->assertNotEquals('xxx', $sid);
+    }
 }

When run like so:


(I can't get {{./runtests.sh}} to run the {{Zend_Session}} suite)

Updating {{Zend_Session::start}} like this solves the issue:

Index: library/Zend/Session.php

--- library/Zend/Session.php (revision 24490) +++ library/Zend/Session.php (working copy) @@ -415,6 +415,14 @@ */ public static function start($options = false) { + // Check to see if we've been passed an invalid session ID + if ( self::getId() && !self::_checkId(self::getId()) ) { + // Generate a valid, temporary replacement + self::setId(md5(self::getId())); + // Force a regenerate after session is started + self::$_regenerateIdState = -1; + } + if (self::$_sessionStarted && self::$_destroyed) { require_once 'Zend/Session/Exception.php'; throw new Zend_Session_Exception('The session was explicitly destroyed during this request, attempting to re-start is not allowed.'); @@ -498,6 +506,26 @@

     self::_processStartupMetadataGlobal();
 }

+ + /** + * Perform a hash-bits check on the session ID + * + * @param string $id Session ID + * @return bool + / + protected static function _checkId($id) + { + $hashBitsPerChar = ini_get('session.hash_bits_per_character'); + if (!$hashBitsPerChar) { + $hashBitsPerChar = 5; // the default value + } + switch($hashBitsPerChar) { + case 4: $pattern = '^[0-9a-f]$'; break; + case 5: $pattern = '^[0-9a-v]$'; break; + case 6: $pattern = '^[0-9a-zA-Z-,]$'; break; + } + return preg_match('#'.$pattern.'#', $id); + }

 /**

```

This resets an invalid session ID to something that is valid (I chose to md5() the invalid one), and then forces {{Zend_Session}} to regenerate the session ID if it's invalid.

I'm not able to confirm if this breaks anything in the existing {{Zend_Session}} test suite, as I can't get it to run. Using {{./runtests.sh}} always returns "0 tests", and running phpunit directly (ie: {{phpunit --verbose --debug Zend/Session/AllTests}}) ends in the process infinitely hanging with no output produced.

Attached patch

Fixed in svn r24819.