Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-6cq5-8cj7-g558
Fix session handlers bug
  • Loading branch information
MGatner committed Dec 22, 2022
2 parents bd3343a + 3128af9 commit f9fb657
Show file tree
Hide file tree
Showing 11 changed files with 131 additions and 39 deletions.
23 changes: 18 additions & 5 deletions system/Session/Handlers/DatabaseHandler.php
Expand Up @@ -60,12 +60,18 @@ class DatabaseHandler extends BaseHandler
*/
protected $rowExists = false;

/**
* ID prefix for multiple session cookies
*/
protected string $idPrefix;

/**
* @throws SessionException
*/
public function __construct(AppConfig $config, string $ipAddress)
{
parent::__construct($config, $ipAddress);

$this->table = $config->sessionSavePath;

if (empty($this->table)) {
Expand All @@ -77,6 +83,9 @@ public function __construct(AppConfig $config, string $ipAddress)
$this->db = Database::connect($this->DBGroup);

$this->platform = $this->db->getPlatform();

// Add sessionCookieName for multiple session cookies.
$this->idPrefix = $config->sessionCookieName . ':';
}

/**
Expand Down Expand Up @@ -115,7 +124,7 @@ public function read($id)
$this->sessionID = $id;
}

$builder = $this->db->table($this->table)->where('id', $id);
$builder = $this->db->table($this->table)->where('id', $this->idPrefix . $id);

if ($this->matchIP) {
$builder = $builder->where('ip_address', $this->ipAddress);
Expand Down Expand Up @@ -182,7 +191,7 @@ public function write($id, $data): bool

if ($this->rowExists === false) {
$insertData = [
'id' => $id,
'id' => $this->idPrefix . $id,
'ip_address' => $this->ipAddress,
'data' => $this->prepareData($data),
];
Expand All @@ -197,7 +206,7 @@ public function write($id, $data): bool
return true;
}

$builder = $this->db->table($this->table)->where('id', $id);
$builder = $this->db->table($this->table)->where('id', $this->idPrefix . $id);

if ($this->matchIP) {
$builder = $builder->where('ip_address', $this->ipAddress);
Expand Down Expand Up @@ -242,7 +251,7 @@ public function close(): bool
public function destroy($id): bool
{
if ($this->lock) {
$builder = $this->db->table($this->table)->where('id', $id);
$builder = $this->db->table($this->table)->where('id', $this->idPrefix . $id);

if ($this->matchIP) {
$builder = $builder->where('ip_address', $this->ipAddress);
Expand Down Expand Up @@ -276,7 +285,11 @@ public function gc($max_lifetime)
$separator = ' ';
$interval = implode($separator, ['', "{$max_lifetime} second", '']);

return $this->db->table($this->table)->where('timestamp <', "now() - INTERVAL {$interval}", false)->delete() ? 1 : $this->fail();
return $this->db->table($this->table)->where(
'timestamp <',
"now() - INTERVAL {$interval}",
false
)->delete() ? 1 : $this->fail();
}

/**
Expand Down
32 changes: 26 additions & 6 deletions system/Session/Handlers/MemcachedHandler.php
Expand Up @@ -61,6 +61,9 @@ public function __construct(AppConfig $config, string $ipAddress)
throw SessionException::forEmptySavepath();
}

// Add sessionCookieName for multiple session cookies.
$this->keyPrefix .= $config->sessionCookieName . ':';

if ($this->matchIP === true) {
$this->keyPrefix .= $this->ipAddress . ':';
}
Expand Down Expand Up @@ -89,7 +92,14 @@ public function open($path, $name): bool
$serverList[] = $server['host'] . ':' . $server['port'];
}

if (! preg_match_all('#,?([^,:]+)\:(\d{1,5})(?:\:(\d+))?#', $this->savePath, $matches, PREG_SET_ORDER)) {
if (
! preg_match_all(
'#,?([^,:]+)\:(\d{1,5})(?:\:(\d+))?#',
$this->savePath,
$matches,
PREG_SET_ORDER
)
) {
$this->memcached = null;
$this->logger->error('Session: Invalid Memcached save path format: ' . $this->savePath);

Expand All @@ -99,13 +109,17 @@ public function open($path, $name): bool
foreach ($matches as $match) {
// If Memcached already has this server (or if the port is invalid), skip it
if (in_array($match[1] . ':' . $match[2], $serverList, true)) {
$this->logger->debug('Session: Memcached server pool already has ' . $match[1] . ':' . $match[2]);
$this->logger->debug(
'Session: Memcached server pool already has ' . $match[1] . ':' . $match[2]
);

continue;
}

if (! $this->memcached->addServer($match[1], (int) $match[2], $match[3] ?? 0)) {
$this->logger->error('Could not add ' . $match[1] . ':' . $match[2] . ' to Memcached server pool.');
$this->logger->error(
'Could not add ' . $match[1] . ':' . $match[2] . ' to Memcached server pool.'
);
} else {
$serverList[] = $match[1] . ':' . $match[2];
}
Expand Down Expand Up @@ -260,7 +274,9 @@ protected function lockSession(string $sessionID): bool
}

if (! $this->memcached->set($lockKey, Time::now()->getTimestamp(), 300)) {
$this->logger->error('Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID);
$this->logger->error(
'Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID
);

return false;
}
Expand All @@ -270,7 +286,9 @@ protected function lockSession(string $sessionID): bool
} while (++$attempt < 30);

if ($attempt === 30) {
$this->logger->error('Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.');
$this->logger->error(
'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.'
);

return false;
}
Expand All @@ -290,7 +308,9 @@ protected function releaseLock(): bool
! $this->memcached->delete($this->lockKey)
&& $this->memcached->getResultCode() !== Memcached::RES_NOTFOUND
) {
$this->logger->error('Session: Error while trying to free lock for ' . $this->lockKey);
$this->logger->error(
'Session: Error while trying to free lock for ' . $this->lockKey
);

return false;
}
Expand Down
3 changes: 3 additions & 0 deletions system/Session/Handlers/RedisHandler.php
Expand Up @@ -71,6 +71,9 @@ public function __construct(AppConfig $config, string $ipAddress)

$this->setSavePath();

// Add sessionCookieName for multiple session cookies.
$this->keyPrefix .= $config->sessionCookieName . ':';

if ($this->matchIP === true) {
$this->keyPrefix .= $this->ipAddress . ':';
}
Expand Down
4 changes: 4 additions & 0 deletions user_guide_src/source/changelogs/v4.2.11.rst
Expand Up @@ -13,11 +13,15 @@ SECURITY
********

- *Attackers may spoof IP address when using proxy* was fixed. See the `Security advisory GHSA-ghw3-5qvm-3mqc <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-ghw3-5qvm-3mqc>`_ for more information.
- *Potential Session Handlers Vulnerability* was fixed. See the `Security advisory GHSA-6cq5-8cj7-g558 <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-6cq5-8cj7-g558>`_ for more information.

BREAKING
********

- The ``Config\App::$proxyIPs`` value format has been changed. See :ref:`Upgrading Guide <upgrade-4211-proxyips>`.
- The key of the session data record for :ref:`sessions-databasehandler-driver`,
:ref:`sessions-memcachedhandler-driver` and :ref:`sessions-redishandler-driver`
has changed. See :ref:`Upgrading Guide <upgrade-4211-session-key>`.

Bugs Fixed
**********
Expand Down
24 changes: 24 additions & 0 deletions user_guide_src/source/installation/upgrade_4211.rst
Expand Up @@ -29,6 +29,29 @@ The config value format has been changed. Now you must set your proxy IP address

``ConfigException`` will be thrown for old format config value.

.. _upgrade-4211-session-key:

Session Handler Key Changes
===========================

The key of the session data record for :ref:`sessions-databasehandler-driver`,
:ref:`sessions-memcachedhandler-driver` and :ref:`sessions-redishandler-driver`
has changed. Therefore, any existing session data will be invalidated after
the upgrade if you are using these session handlers.

- When using ``DatabaseHandler``, the ``id`` column value in the session table
now contains the session cookie name (``Config\App::$sessionCookieName``).
- When using ``MemcachedHandler`` or ``RedisHandler``, the key value contains
the session cookie name (``Config\App::$sessionCookieName``).

There is maximum length for the ``id`` column and Memcached key (250 bytes).
If the following values exceed those maximum length, the session will not work properly.

- the session cookie name, delimiter, and session id (32 characters by default)
when using ``DatabaseHandler``
- the prefix (``ci_session``), session cookie name, delimiters, and session id
when using ``MemcachedHandler``

Project Files
*************

Expand All @@ -46,3 +69,4 @@ many will be simple comments or formatting that have no effect on the runtime:
* app/Config/Toolbar.php
* app/Views/welcome_message.php
* composer.json
* phpunit.xml.dist
69 changes: 44 additions & 25 deletions user_guide_src/source/libraries/sessions.rst
Expand Up @@ -358,8 +358,8 @@ same way:
unusable during the same request after you destroy the session.

You may also use the ``stop()`` method to completely kill the session
by removing the old session_id, destroying all data, and destroying
the cookie that contained the session id:
by removing the old session ID, destroying all data, and destroying
the cookie that contained the session ID:

.. literalinclude:: sessions/038.php

Expand Down Expand Up @@ -390,26 +390,35 @@ all of the options and their effects.
You'll find the following Session related preferences in your
**app/Config/App.php** file:

============================== ============================================ ================================================= ============================================================================================
Preference Default Options Description
============================== ============================================ ================================================= ============================================================================================
**sessionDriver** CodeIgniter\\Session\\Handlers\\FileHandler CodeIgniter\\Session\\Handlers\\FileHandler The session storage driver to use.
CodeIgniter\\Session\\Handlers\\DatabaseHandler
CodeIgniter\\Session\\Handlers\\MemcachedHandler
CodeIgniter\\Session\\Handlers\\RedisHandler
CodeIgniter\\Session\\Handlers\\ArrayHandler
**sessionCookieName** ci_session [A-Za-z\_-] characters only The name used for the session cookie.
**sessionExpiration** 7200 (2 hours) Time in seconds (integer) The number of seconds you would like the session to last.
If you would like a non-expiring session (until browser is closed) set the value to zero: 0
**sessionSavePath** null None Specifies the storage location, depends on the driver being used.
**sessionMatchIP** false true/false (boolean) Whether to validate the user's IP address when reading the session cookie.
Note that some ISPs dynamically changes the IP, so if you want a non-expiring session you
will likely set this to false.
**sessionTimeToUpdate** 300 Time in seconds (integer) This option controls how often the session class will regenerate itself and create a new
session ID. Setting it to 0 will disable session ID regeneration.
**sessionRegenerateDestroy** false true/false (boolean) Whether to destroy session data associated with the old session ID when auto-regenerating
the session ID. When set to false, the data will be later deleted by the garbage collector.
============================== ============================================ ================================================= ============================================================================================
============================== ================== =========================== ============================================================
Preference Default Options Description
============================== ================== =========================== ============================================================
**sessionDriver** FileHandler::class FileHandler::class The session storage driver to use.
DatabaseHandler::class All the session drivers are located in the
MemcachedHandler::class ``CodeIgniter\Session\Handlers\`` namespace.
RedisHandler::class
ArrayHandler::class
**sessionCookieName** ci_session [A-Za-z\_-] characters only The name used for the session cookie.
The value will be included in the key of the
Database/Memcached/Redis session records. So, set the value
so that it does not exceed the maximum length of the key.
**sessionExpiration** 7200 (2 hours) Time in seconds (integer) The number of seconds you would like the session to last.
If you would like a non-expiring session (until browser is
closed) set the value to zero: 0
**sessionSavePath** null None Specifies the storage location, depends on the driver being
used.
**sessionMatchIP** false true/false (boolean) Whether to validate the user's IP address when reading the
session cookie. Note that some ISPs dynamically changes the IP,
so if you want a non-expiring session you will likely set this
to false.
**sessionTimeToUpdate** 300 Time in seconds (integer) This option controls how often the session class will
regenerate itself and create a new session ID. Setting it to 0
will disable session ID regeneration.
**sessionRegenerateDestroy** false true/false (boolean) Whether to destroy session data associated with the old
session ID when auto-regenerating
the session ID. When set to false, the data will be later
deleted by the garbage collector.
============================== ================== =========================== ============================================================

.. note:: As a last resort, the Session library will try to fetch PHP's
session related INI settings, as well as legacy CI settings such as
Expand Down Expand Up @@ -498,9 +507,9 @@ permissions will probably break your application.
Instead, you should do something like this, depending on your environment
::

mkdir /<path to your application directory>/Writable/sessions/
chmod 0700 /<path to your application directory>/Writable/sessions/
chown www-data /<path to your application directory>/Writable/sessions/
> mkdir /<path to your application directory>/writable/sessions/
> chmod 0700 /<path to your application directory>/writable/sessions/
> chown www-data /<path to your application directory>/writable/sessions/

Bonus Tip
---------
Expand All @@ -518,6 +527,8 @@ In addition, if performance is your only concern, you may want to look
into using `tmpfs <https://eddmann.com/posts/storing-php-sessions-file-caches-in-memory-using-tmpfs/>`_,
(warning: external resource), which can make your sessions blazing fast.

.. _sessions-databasehandler-driver:

DatabaseHandler Driver
======================

Expand Down Expand Up @@ -561,6 +572,10 @@ For PostgreSQL::

CREATE INDEX "ci_sessions_timestamp" ON "ci_sessions" ("timestamp");

.. note:: The ``id`` value contains the session cookie name (``Config\App::$sessionCookieName``)
and the session ID and a delimiter. It should be increased as needed, for example,
when using long session IDs.

You will also need to add a PRIMARY KEY **depending on your 'sessionMatchIP'
setting**. The examples below work both on MySQL and PostgreSQL::

Expand Down Expand Up @@ -595,6 +610,8 @@ when it generates the code.
done processing session data if you're having performance
issues.

.. _sessions-redishandler-driver:

RedisHandler Driver
===================

Expand Down Expand Up @@ -631,6 +648,8 @@ sufficient:

.. literalinclude:: sessions/041.php

.. _sessions-memcachedhandler-driver:

MemcachedHandler Driver
=======================

Expand Down
4 changes: 3 additions & 1 deletion user_guide_src/source/libraries/sessions/039.php
Expand Up @@ -6,7 +6,9 @@

class App extends BaseConfig
{
public $sessionDriver = 'CodeIgniter\Session\Handlers\DatabaseHandler';
// ...
public $sessionDriver = 'CodeIgniter\Session\Handlers\DatabaseHandler';
// ...
public $sessionSavePath = 'ci_sessions';
// ...
}
1 change: 1 addition & 0 deletions user_guide_src/source/libraries/sessions/040.php
Expand Up @@ -6,6 +6,7 @@

class App extends BaseConfig
{
// ...
public $sessionDBGroup = 'groupName';
// ...
}
4 changes: 3 additions & 1 deletion user_guide_src/source/libraries/sessions/041.php
Expand Up @@ -6,7 +6,9 @@

class App extends BaseConfig
{
public $sessionDiver = 'CodeIgniter\Session\Handlers\RedisHandler';
// ...
public $sessionDiver = 'CodeIgniter\Session\Handlers\RedisHandler';
// ...
public $sessionSavePath = 'tcp://localhost:6379';
// ...
}
4 changes: 3 additions & 1 deletion user_guide_src/source/libraries/sessions/042.php
Expand Up @@ -6,7 +6,9 @@

class App extends BaseConfig
{
public $sessionDriver = 'CodeIgniter\Session\Handlers\MemcachedHandler';
// ...
public $sessionDriver = 'CodeIgniter\Session\Handlers\MemcachedHandler';
// ...
public $sessionSavePath = 'localhost:11211';
// ...
}
2 changes: 2 additions & 0 deletions user_guide_src/source/libraries/sessions/043.php
Expand Up @@ -6,6 +6,8 @@

class App extends BaseConfig
{
// ...

// localhost will be given higher priority (5) here,
// compared to 192.0.2.1 with a weight of 1.
public $sessionSavePath = 'localhost:11211:5,192.0.2.1:11211:1';
Expand Down

0 comments on commit f9fb657

Please sign in to comment.