From fd7d84616d53486c3a276a42da869390e1d7f5eb Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Sun, 22 Oct 2017 19:54:44 +0200 Subject: [PATCH] Move session ID check to SessionManager Relates to https://github.com/shaarli/Shaarli/issues/324 Changed: - `is_session_id_valid()` -> `SessionManager::checkId()` - update tests Signed-off-by: VirtualTam --- application/SessionManager.php | 30 +++++++++++++++ application/Utils.php | 30 --------------- index.php | 2 +- tests/SessionManagerTest.php | 67 +++++++++++++++++++++++++++++++++- tests/UtilsTest.php | 58 ----------------------------- 5 files changed, 97 insertions(+), 90 deletions(-) diff --git a/application/SessionManager.php b/application/SessionManager.php index 2083df42..3aa4ddfc 100644 --- a/application/SessionManager.php +++ b/application/SessionManager.php @@ -50,4 +50,34 @@ class SessionManager unset($this->session['tokens'][$token]); return true; } + + /** + * Validate session ID to prevent Full Path Disclosure. + * + * See #298. + * The session ID's format depends on the hash algorithm set in PHP settings + * + * @param string $sessionId Session ID + * + * @return true if valid, false otherwise. + * + * @see http://php.net/manual/en/function.hash-algos.php + * @see http://php.net/manual/en/session.configuration.php + */ + public static function checkId($sessionId) + { + if (empty($sessionId)) { + return false; + } + + if (!$sessionId) { + return false; + } + + if (!preg_match('/^[a-zA-Z0-9,-]{2,128}$/', $sessionId)) { + return false; + } + + return true; + } } diff --git a/application/Utils.php b/application/Utils.php index 2f38a8de..97b12fcf 100644 --- a/application/Utils.php +++ b/application/Utils.php @@ -181,36 +181,6 @@ function generateLocation($referer, $host, $loopTerms = array()) return $finalReferer; } -/** - * Validate session ID to prevent Full Path Disclosure. - * - * See #298. - * The session ID's format depends on the hash algorithm set in PHP settings - * - * @param string $sessionId Session ID - * - * @return true if valid, false otherwise. - * - * @see http://php.net/manual/en/function.hash-algos.php - * @see http://php.net/manual/en/session.configuration.php - */ -function is_session_id_valid($sessionId) -{ - if (empty($sessionId)) { - return false; - } - - if (!$sessionId) { - return false; - } - - if (!preg_match('/^[a-zA-Z0-9,-]{2,128}$/', $sessionId)) { - return false; - } - - return true; -} - /** * Sniff browser language to set the locale automatically. * Note that is may not work on your server if the corresponding locale is not installed. diff --git a/index.php b/index.php index 9e698628..e1516d37 100644 --- a/index.php +++ b/index.php @@ -116,7 +116,7 @@ if (session_id() == '') { } // Regenerate session ID if invalid or not defined in cookie. -if (isset($_COOKIE['shaarli']) && !is_session_id_valid($_COOKIE['shaarli'])) { +if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli'])) { session_regenerate_id(true); $_COOKIE['shaarli'] = session_id(); } diff --git a/tests/SessionManagerTest.php b/tests/SessionManagerTest.php index 3a270303..9fa60dc5 100644 --- a/tests/SessionManagerTest.php +++ b/tests/SessionManagerTest.php @@ -1,8 +1,12 @@ assertFalse($sessionManager->checkToken('4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b')); } + + /** + * Test SessionManager::checkId with a valid ID - TEST ALL THE HASHES! + * + * This tests extensively covers all hash algorithms / bit representations + */ + public function testIsAnyHashSessionIdValid() + { + foreach (self::$sidHashes as $algo => $bpcs) { + foreach ($bpcs as $bpc => $hash) { + $this->assertTrue(SessionManager::checkId($hash)); + } + } + } + + /** + * Test checkId with a valid ID - SHA-1 hashes + */ + public function testIsSha1SessionIdValid() + { + $this->assertTrue(SessionManager::checkId(sha1('shaarli'))); + } + + /** + * Test checkId with a valid ID - SHA-256 hashes + */ + public function testIsSha256SessionIdValid() + { + $this->assertTrue(SessionManager::checkId(hash('sha256', 'shaarli'))); + } + + /** + * Test checkId with a valid ID - SHA-512 hashes + */ + public function testIsSha512SessionIdValid() + { + $this->assertTrue(SessionManager::checkId(hash('sha512', 'shaarli'))); + } + + /** + * Test checkId with invalid IDs. + */ + public function testIsSessionIdInvalid() + { + $this->assertFalse(SessionManager::checkId('')); + $this->assertFalse(SessionManager::checkId([])); + $this->assertFalse( + SessionManager::checkId('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=') + ); + } } diff --git a/tests/UtilsTest.php b/tests/UtilsTest.php index 840eaf21..6cd37a7a 100644 --- a/tests/UtilsTest.php +++ b/tests/UtilsTest.php @@ -5,10 +5,6 @@ require_once 'application/Utils.php'; require_once 'application/Languages.php'; -require_once 'tests/utils/ReferenceSessionIdHashes.php'; - -// Initialize reference data before PHPUnit starts a session -ReferenceSessionIdHashes::genAllHashes(); /** @@ -16,9 +12,6 @@ ReferenceSessionIdHashes::genAllHashes(); */ class UtilsTest extends PHPUnit_Framework_TestCase { - // Session ID hashes - protected static $sidHashes = null; - // Log file protected static $testLogFile = 'tests.log'; @@ -30,13 +23,11 @@ class UtilsTest extends PHPUnit_Framework_TestCase */ protected static $defaultTimeZone; - /** * Assign reference data */ public static function setUpBeforeClass() { - self::$sidHashes = ReferenceSessionIdHashes::getHashes(); self::$defaultTimeZone = date_default_timezone_get(); // Timezone without DST for test consistency date_default_timezone_set('Africa/Nairobi'); @@ -221,56 +212,7 @@ class UtilsTest extends PHPUnit_Framework_TestCase $this->assertEquals('?', generateLocation($ref, 'localhost')); } - /** - * Test is_session_id_valid with a valid ID - TEST ALL THE HASHES! - * - * This tests extensively covers all hash algorithms / bit representations - */ - public function testIsAnyHashSessionIdValid() - { - foreach (self::$sidHashes as $algo => $bpcs) { - foreach ($bpcs as $bpc => $hash) { - $this->assertTrue(is_session_id_valid($hash)); - } - } - } - /** - * Test is_session_id_valid with a valid ID - SHA-1 hashes - */ - public function testIsSha1SessionIdValid() - { - $this->assertTrue(is_session_id_valid(sha1('shaarli'))); - } - - /** - * Test is_session_id_valid with a valid ID - SHA-256 hashes - */ - public function testIsSha256SessionIdValid() - { - $this->assertTrue(is_session_id_valid(hash('sha256', 'shaarli'))); - } - - /** - * Test is_session_id_valid with a valid ID - SHA-512 hashes - */ - public function testIsSha512SessionIdValid() - { - $this->assertTrue(is_session_id_valid(hash('sha512', 'shaarli'))); - } - - /** - * Test is_session_id_valid with invalid IDs. - */ - public function testIsSessionIdInvalid() - { - $this->assertFalse(is_session_id_valid('')); - $this->assertFalse(is_session_id_valid(array())); - $this->assertFalse( - is_session_id_valid('c0ZqcWF3VFE2NmJBdm1HMVQ0ZHJ3UmZPbTFsNGhkNHI=') - ); - } - /** * Test generateSecretApi. */ -- 2.41.0