From ce088e05df897030ff8e7282d3309b1396d5fab9 Mon Sep 17 00:00:00 2001 From: rhertogh Date: Sat, 31 Oct 2020 09:58:34 +0100 Subject: [PATCH] Fix #18247: Added support for the 'session.use_strict_mode' ini directive in `yii\web\Session` --- .github/workflows/build.yml | 3 +- docs/guide/runtime-sessions-cookies.md | 1 + framework/CHANGELOG.md | 1 + framework/UPGRADE.md | 35 +++++++++ framework/web/CacheSession.php | 25 +++++++ framework/web/DbSession.php | 41 ++++++++++- framework/web/Session.php | 72 ++++++++++++++++++ .../web/session/AbstractDbSessionTest.php | 14 +++- .../web/session/CacheSessionTest.php | 12 +++ tests/framework/web/session/SessionTest.php | 23 ++++++ .../web/session/SessionTestTrait.php | 73 +++++++++++++++++++ 11 files changed, 295 insertions(+), 5 deletions(-) create mode 100644 tests/framework/web/session/SessionTestTrait.php diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a90f678ea6..a912dbfd1b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -56,7 +56,7 @@ jobs: php-version: ${{ matrix.php }} tools: pecl extensions: apc, curl, dom, imagick, intl, mbstring, mcrypt, memcached, mysql, pdo, pdo_mysql, pdo_pgsql, pdo_sqlite, pgsql, sqlite - ini-values: date.timezone='UTC' + ini-values: date.timezone='UTC', session.save_path="${{ runner.temp }}" - name: Install php-sqlsrv run: | curl -sSL https://packages.microsoft.com/keys/microsoft.asc | sudo apt-key add - >/dev/null 2>&1 @@ -114,6 +114,7 @@ jobs: uses: shivammathur/setup-php@v2 with: php-version: 7.2 + ini-values: session.save_path=${{ runner.temp }} - name: Get composer cache directory id: composer-cache run: echo "::set-output name=dir::$(composer config cache-files-dir)" diff --git a/docs/guide/runtime-sessions-cookies.md b/docs/guide/runtime-sessions-cookies.md index 92e0760476..384b62b3b8 100644 --- a/docs/guide/runtime-sessions-cookies.md +++ b/docs/guide/runtime-sessions-cookies.md @@ -403,3 +403,4 @@ To use this feature across different PHP versions check the version first. E.g. As [noted in PHP manual](https://www.php.net/manual/en/session.security.ini.php), `php.ini` has important session security settings. Please ensure recommended settings are applied. Especially `session.use_strict_mode` that is not enabled by default in PHP installations. +This setting can also be set with [[yii\web\Session::useStrictMode]]. diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 7aaa89dcbc..a5a1031e64 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -4,6 +4,7 @@ Yii Framework 2 Change Log 2.0.39 under development ------------------------ +- Enh #18247: Added support for the 'session.use_strict_mode' ini directive in `yii\web\Session` (rhertogh) - Bug #18263: Fix writing `\yii\caching\FileCache` files to the same directory when `keyPrefix` is set (githubjeka) - Bug #18160, #18192: Fixed `registerFile` with argument depends set does not use the position and appendTimestamp argument, also modify the unit view (baleeny) - Bug #18290: Fix response with non-seekable streams (schmunk42) diff --git a/framework/UPGRADE.md b/framework/UPGRADE.md index 6ae88d4a3f..ac65ca2fc1 100644 --- a/framework/UPGRADE.md +++ b/framework/UPGRADE.md @@ -63,6 +63,41 @@ Upgrade from Yii 2.0.37 * Resolving DI references inside of arrays in dependencies was made optional and turned off by default. In order to turn it on, set `resolveArrays` of container instance to `true`. +* `yii\web\Session` now respects the 'session.use_strict_mode' ini directive. + In case you use a custom `Session` class and have overwritten the `Session::openSession()` and/or + `Session::writeSession()` functions changes might be required: + * When in strict mode the `openSession()` function should check if the requested session id exists + (and mark it for forced regeneration if not). + For example, the `DbSession` does this at the beginning of the function as follows: + ```php + if ($this->getUseStrictMode()) { + $id = $this->getId(); + if (!$this->getReadQuery($id)->exists()) { + //This session id does not exist, mark it for forced regeneration + $this->_forceRegenerateId = $id; + } + } + // ... normal function continues ... + ``` + * When in strict mode the `writeSession()` function should ignore writing the session under the old id. + For example, the `DbSession` does this at the beginning of the function as follows: + ```php + if ($this->getUseStrictMode() && $id === $this->_forceRegenerateId) { + //Ignore write when forceRegenerate is active for this id + return true; + } + // ... normal function continues ... + ``` + > Note: The sample code above is specific for the `yii\web\DbSession` class. + Make sure you use the correct implementation based on your parent class, + e.g. `yii\web\CacheSession`, `yii\redis\Session`, `yii\mongodb\Session`, etc. + + > Note: In case your custom functions call their `parent` functions, there are probably no changes needed to your + code if those parents implement the `useStrictMode` checks. + + > Warning: in case `openSession()` and/or `writeSession()` functions do not implement the `useStrictMode` code + the session could be stored under the forced id without warning even if `useStrictMode` is enabled. + Upgrade from Yii 2.0.36 ----------------------- diff --git a/framework/web/CacheSession.php b/framework/web/CacheSession.php index 918535973a..7cbc580547 100644 --- a/framework/web/CacheSession.php +++ b/framework/web/CacheSession.php @@ -69,6 +69,26 @@ class CacheSession extends Session return true; } + /** + * Session open handler. + * @internal Do not call this method directly. + * @param string $savePath session save path + * @param string $sessionName session name + * @return bool whether session is opened successfully + */ + public function openSession($savePath, $sessionName) + { + if ($this->getUseStrictMode()) { + $id = $this->getId(); + if (!$this->cache->exists($this->calculateKey($id))) { + //This session id does not exist, mark it for forced regeneration + $this->_forceRegenerateId = $id; + } + } + + return parent::openSession($savePath, $sessionName); + } + /** * Session read handler. * @internal Do not call this method directly. @@ -91,6 +111,11 @@ class CacheSession extends Session */ public function writeSession($id, $data) { + if ($this->getUseStrictMode() && $id === $this->_forceRegenerateId) { + //Ignore write when forceRegenerate is active for this id + return true; + } + return $this->cache->set($this->calculateKey($id), $data, $this->getTimeout()); } diff --git a/framework/web/DbSession.php b/framework/web/DbSession.php index 26c392f0d3..bc23f66066 100644 --- a/framework/web/DbSession.php +++ b/framework/web/DbSession.php @@ -93,6 +93,26 @@ class DbSession extends MultiFieldSession $this->db = Instance::ensure($this->db, Connection::className()); } + /** + * Session open handler. + * @internal Do not call this method directly. + * @param string $savePath session save path + * @param string $sessionName session name + * @return bool whether session is opened successfully + */ + public function openSession($savePath, $sessionName) + { + if ($this->getUseStrictMode()) { + $id = $this->getId(); + if (!$this->getReadQuery($id)->exists()) { + //This session id does not exist, mark it for forced regeneration + $this->_forceRegenerateId = $id; + } + } + + return parent::openSession($savePath, $sessionName); + } + /** * {@inheritdoc} */ @@ -160,9 +180,7 @@ class DbSession extends MultiFieldSession */ public function readSession($id) { - $query = new Query(); - $query->from($this->sessionTable) - ->where('[[expire]]>:expire AND [[id]]=:id', [':expire' => time(), ':id' => $id]); + $query = $this->getReadQuery($id); if ($this->readCallback !== null) { $fields = $query->one($this->db); @@ -182,6 +200,11 @@ class DbSession extends MultiFieldSession */ public function writeSession($id, $data) { + if ($this->getUseStrictMode() && $id === $this->_forceRegenerateId) { + //Ignore write when forceRegenerate is active for this id + return true; + } + // exception must be caught in session write handler // https://secure.php.net/manual/en/function.session-set-save-handler.php#refsect1-function.session-set-save-handler-notes try { @@ -240,6 +263,18 @@ class DbSession extends MultiFieldSession return true; } + /** + * Generates a query to get the session from db + * @param string $id The id of the session + * @return Query + */ + protected function getReadQuery($id) + { + return (new Query()) + ->from($this->sessionTable) + ->where('[[expire]]>:expire AND [[id]]=:id', [':expire' => time(), ':id' => $id]); + } + /** * Method typecasts $fields before passing them to PDO. * Default implementation casts field `data` to `\PDO::PARAM_LOB`. diff --git a/framework/web/Session.php b/framework/web/Session.php index fcc8a3a3f7..4d9311608a 100644 --- a/framework/web/Session.php +++ b/framework/web/Session.php @@ -68,12 +68,25 @@ use yii\base\InvalidConfigException; * @property bool $useCustomStorage Whether to use custom storage. This property is read-only. * @property bool $useTransparentSessionID Whether transparent sid support is enabled or not, defaults to * false. + * @property bool $useStrictMode Whether strict mode is enabled or not. + * Note: Enabling `useStrictMode` on PHP < 5.5.2 is only supported with custom storage classes. * * @author Qiang Xue * @since 2.0 */ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Countable { + /** + * @var string|null Holds the original session module (before a custom handler is registered) so that it can be + * restored when a Session component without custom handler is used after one that has. + */ + static protected $_originalSessionModule = null; + + /** + * Polyfill for ini directive session.use-strict-mode for PHP < 5.5.2. + */ + static private $_useStrictModePolyfill = false; + /** * @var string the name of the session variable that stores the flash message data. */ @@ -82,6 +95,10 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co * @var \SessionHandlerInterface|array an object implementing the SessionHandlerInterface or a configuration array. If set, will be used to provide persistency instead of build-in methods. */ public $handler; + /** + * @var string|null Holds the session id in case useStrictMode is enabled and the session id needs to be regenerated + */ + protected $_forceRegenerateId = null; /** * @var array parameter-value pairs to override default session cookie parameters that are used for session_set_cookie_params() function @@ -136,6 +153,11 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co YII_DEBUG ? session_start() : @session_start(); + if ($this->getUseStrictMode() && $this->_forceRegenerateId) { + $this->regenerateID(); + $this->_forceRegenerateId = null; + } + if ($this->getIsActive()) { Yii::info('Session started', __METHOD__); $this->updateFlashCounters(); @@ -152,6 +174,11 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co */ protected function registerSessionHandler() { + $sessionModuleName = session_module_name(); + if (static::$_originalSessionModule === null) { + static::$_originalSessionModule = $sessionModuleName; + } + if ($this->handler !== null) { if (!is_object($this->handler)) { $this->handler = Yii::createObject($this->handler); @@ -180,6 +207,12 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co [$this, 'gcSession'] ); } + } elseif ( + $sessionModuleName !== static::$_originalSessionModule + && static::$_originalSessionModule !== null + && static::$_originalSessionModule !== 'user' + ) { + session_module_name(static::$_originalSessionModule); } } @@ -191,6 +224,8 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co if ($this->getIsActive()) { YII_DEBUG ? session_write_close() : @session_write_close(); } + + $this->_forceRegenerateId = null; } /** @@ -514,6 +549,43 @@ class Session extends Component implements \IteratorAggregate, \ArrayAccess, \Co $this->unfreeze(); } + /** + * @var bool Whether strict mode is enabled or not. + * When `true` this setting prevents the session component to use an uninitialized session ID. + * Note: Enabling `useStrictMode` on PHP < 5.5.2 is only supported with custom storage classes. + * Warning! Although enabling strict mode is mandatory for secure sessions, the default value of 'session.use-strict-mode' is `0`. + * @see https://www.php.net/manual/en/session.configuration.php#ini.session.use-strict-mode + * @since 2.0.38 + */ + public function setUseStrictMode($value) + { + if (PHP_VERSION_ID < 50502) { + if ($this->getUseCustomStorage() || !$value) { + self::$_useStrictModePolyfill = $value; + } else { + throw new InvalidConfigException('Enabling `useStrictMode` on PHP < 5.5.2 is only supported with custom storage classes.'); + } + } else { + $this->freeze(); + ini_set('session.use_strict_mode', $value ? '1' : '0'); + $this->unfreeze(); + } + } + + /** + * @return bool Whether strict mode is enabled or not. + * @see setUseStrictMode() + * @since 2.0.38 + */ + public function getUseStrictMode() + { + if (PHP_VERSION_ID < 50502) { + return self::$_useStrictModePolyfill; + } + + return (bool)ini_get('session.use_strict_mode'); + } + /** * Session open handler. * This method should be overridden if [[useCustomStorage]] returns true. diff --git a/tests/framework/web/session/AbstractDbSessionTest.php b/tests/framework/web/session/AbstractDbSessionTest.php index b9f29d2696..835ea13b21 100644 --- a/tests/framework/web/session/AbstractDbSessionTest.php +++ b/tests/framework/web/session/AbstractDbSessionTest.php @@ -9,8 +9,8 @@ namespace yiiunit\framework\web\session; use Yii; use yii\db\Connection; -use yii\db\Query; use yii\db\Migration; +use yii\db\Query; use yii\web\DbSession; use yiiunit\framework\console\controllers\EchoMigrateController; use yiiunit\TestCase; @@ -20,6 +20,8 @@ use yiiunit\TestCase; */ abstract class AbstractDbSessionTest extends TestCase { + use SessionTestTrait; + /** * @return string[] the driver names that are suitable for the test (mysql, pgsql, etc) */ @@ -266,4 +268,14 @@ abstract class AbstractDbSessionTest extends TestCase Yii::$app->set('sessionDb', null); ini_set('session.gc_maxlifetime', $oldTimeout); } + + public function testInitUseStrictMode() + { + $this->initStrictModeTest(DbSession::className()); + } + + public function testUseStrictMode() + { + $this->useStrictModeTest(DbSession::className()); + } } diff --git a/tests/framework/web/session/CacheSessionTest.php b/tests/framework/web/session/CacheSessionTest.php index 018d1643fb..e31b0f970f 100644 --- a/tests/framework/web/session/CacheSessionTest.php +++ b/tests/framework/web/session/CacheSessionTest.php @@ -16,6 +16,8 @@ use yii\web\CacheSession; */ class CacheSessionTest extends \yiiunit\TestCase { + use SessionTestTrait; + protected function setUp() { parent::setUp(); @@ -51,4 +53,14 @@ class CacheSessionTest extends \yiiunit\TestCase $this->assertTrue($session->destroySession($session->getId())); } + + public function testInitUseStrictMode() + { + $this->initStrictModeTest(CacheSession::className()); + } + + public function testUseStrictMode() + { + $this->useStrictModeTest(CacheSession::className()); + } } diff --git a/tests/framework/web/session/SessionTest.php b/tests/framework/web/session/SessionTest.php index 28fc1e10d1..6ad327f68f 100644 --- a/tests/framework/web/session/SessionTest.php +++ b/tests/framework/web/session/SessionTest.php @@ -15,6 +15,8 @@ use yiiunit\TestCase; */ class SessionTest extends TestCase { + use SessionTestTrait; + /** * Test to prove that after Session::destroy session id set to old value. */ @@ -69,6 +71,7 @@ class SessionTest extends TestCase $newGcProbability = $session->getGCProbability(); $this->assertNotEquals($oldGcProbability, $newGcProbability); $this->assertEquals(100, $newGcProbability); + $session->setGCProbability($oldGcProbability); } /** @@ -88,4 +91,24 @@ class SessionTest extends TestCase $session->destroy(); } + + public function testInitUseStrictMode() + { + $this->initStrictModeTest(Session::className()); + } + + public function testUseStrictMode() + { + //Manual garbage collection since native storage module might not support removing data via Session::destroySession() + $sessionSavePath = session_save_path() ?: sys_get_temp_dir(); + // Only perform garbage collection if "N argument" is not used, + // see https://www.php.net/manual/en/session.configuration.php#ini.session.save-path + if (strpos($sessionSavePath, ';') === false) { + foreach (['non-existing-non-strict', 'non-existing-strict'] as $sessionId) { + @unlink($sessionSavePath . '/sess_' . $sessionId); + } + } + + $this->useStrictModeTest(Session::className()); + } } diff --git a/tests/framework/web/session/SessionTestTrait.php b/tests/framework/web/session/SessionTestTrait.php new file mode 100644 index 0000000000..1c3e3da687 --- /dev/null +++ b/tests/framework/web/session/SessionTestTrait.php @@ -0,0 +1,73 @@ +useStrictMode = false; + $this->assertEquals(false, $session->getUseStrictMode()); + + if (PHP_VERSION_ID < 50502 && !$session->getUseCustomStorage()) { + $this->expectException('yii\base\InvalidConfigException'); + $session->useStrictMode = true; + return; + } + + $session->useStrictMode = true; + $this->assertEquals(true, $session->getUseStrictMode()); + } + + /** + * @param string $class + */ + protected function useStrictModeTest($class) + { + /** @var Session $session */ + $session = new $class(); + + if (PHP_VERSION_ID < 50502 && !$session->getUseCustomStorage()) { + $this->markTestSkipped('Can not be tested on PHP < 5.5.2 without custom storage class.'); + return; + } + + //non-strict-mode test + $session->useStrictMode = false; + $session->close(); + $session->destroySession('non-existing-non-strict'); + $session->setId('non-existing-non-strict'); + $session->open(); + $this->assertEquals('non-existing-non-strict', $session->getId()); + $session->close(); + + //strict-mode test + $session->useStrictMode = true; + $session->close(); + $session->destroySession('non-existing-strict'); + $session->setId('non-existing-strict'); + $session->open(); + $id = $session->getId(); + $this->assertNotEquals('non-existing-strict', $id); + $session->set('strict_mode_test', 'session data'); + $session->close(); + //Ensure session was not stored under forced id + $session->setId('non-existing-strict'); + $session->open(); + $this->assertNotEquals('session data', $session->get('strict_mode_test')); + $session->close(); + //Ensure session can be accessed with the new (and thus existing) id. + $session->setId($id); + $session->open(); + $this->assertNotEmpty($id); + $this->assertEquals($id, $session->getId()); + $this->assertEquals('session data', $session->get('strict_mode_test')); + $session->close(); + } +}