From ebd650c06c67a67da2a0d099f625b6a7ec62ab2b Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Sun, 22 Oct 2017 18:44:46 +0200 Subject: Refactor session token management Relates to https://github.com/shaarli/Shaarli/issues/324 Added: - `SessionManager` class to group session-related features - unit tests Changed: - `getToken()` -> `SessionManager->generateToken()` - `tokenOk()` -> `SessionManager->checkToken()` - inject a `$token` parameter to `PageBuilder`'s constructor Signed-off-by: VirtualTam --- application/PageBuilder.php | 6 ++-- application/SessionManager.php | 53 +++++++++++++++++++++++++++++++ index.php | 71 ++++++++++++++--------------------------- tests/SessionManagerTest.php | 72 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 49 deletions(-) create mode 100644 application/SessionManager.php create mode 100644 tests/SessionManagerTest.php diff --git a/application/PageBuilder.php b/application/PageBuilder.php index af290671..468f144b 100644 --- a/application/PageBuilder.php +++ b/application/PageBuilder.php @@ -32,12 +32,14 @@ class PageBuilder * * @param ConfigManager $conf Configuration Manager instance (reference). * @param LinkDB $linkDB instance. + * @param string $token Session token */ - public function __construct(&$conf, $linkDB = null) + public function __construct(&$conf, $linkDB = null, $token = null) { $this->tpl = false; $this->conf = $conf; $this->linkDB = $linkDB; + $this->token = $token; } /** @@ -92,7 +94,7 @@ class PageBuilder $this->tpl->assign('showatom', $this->conf->get('feed.show_atom', true)); $this->tpl->assign('feed_type', $this->conf->get('feed.show_atom', true) !== false ? 'atom' : 'rss'); $this->tpl->assign('hide_timestamps', $this->conf->get('privacy.hide_timestamps', false)); - $this->tpl->assign('token', getToken($this->conf)); + $this->tpl->assign('token', $this->token); if ($this->linkDB !== null) { $this->tpl->assign('tags', $this->linkDB->linksCountPerTag()); diff --git a/application/SessionManager.php b/application/SessionManager.php new file mode 100644 index 00000000..2083df42 --- /dev/null +++ b/application/SessionManager.php @@ -0,0 +1,53 @@ +session = &$session; + $this->conf = &$conf; + } + + /** + * Generates a session token + * + * @return string token + */ + public function generateToken() + { + $token = sha1(uniqid('', true) .'_'. mt_rand() . $this->conf->get('credentials.salt')); + $this->session['tokens'][$token] = 1; + return $token; + } + + /** + * Checks the validity of a session token, and destroys it afterwards + * + * @param string $token The token to check + * + * @return bool true if the token is valid, else false + */ + public function checkToken($token) + { + if (! isset($this->session['tokens'][$token])) { + // the token is wrong, or has already been used + return false; + } + + // destroy the token to prevent future use + unset($this->session['tokens'][$token]); + return true; + } +} diff --git a/index.php b/index.php index 1dc81843..9e698628 100644 --- a/index.php +++ b/index.php @@ -78,6 +78,7 @@ require_once 'application/Updater.php'; use \Shaarli\Languages; use \Shaarli\ThemeUtils; use \Shaarli\Config\ConfigManager; +use \Shaarli\SessionManager; // Ensure the PHP version is supported try { @@ -121,6 +122,7 @@ if (isset($_COOKIE['shaarli']) && !is_session_id_valid($_COOKIE['shaarli'])) { } $conf = new ConfigManager(); +$sessionManager = new SessionManager($_SESSION, $conf); // Sniff browser language and set date format accordingly. if (isset($_SERVER['HTTP_ACCEPT_LANGUAGE'])) { @@ -165,7 +167,7 @@ if (! is_file($conf->getConfigFileExt())) { } // Display the installation form if no existing config is found - install($conf); + install($conf, $sessionManager); } // a token depending of deployment salt, user password, and the current ip @@ -381,7 +383,7 @@ if (isset($_POST['login'])) { if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.')); if (isset($_POST['password']) - && tokenOk($_POST['token']) + && $sessionManager->checkToken($_POST['token']) && (check_auth($_POST['login'], $_POST['password'], $conf)) ) { // Login/password is OK. ban_loginOk($conf); @@ -454,32 +456,6 @@ if (isset($_POST['login'])) // Token should be used in any form which acts on data (create,update,delete,import...). if (!isset($_SESSION['tokens'])) $_SESSION['tokens']=array(); // Token are attached to the session. -/** - * Returns a token. - * - * @param ConfigManager $conf Configuration Manager instance. - * - * @return string token. - */ -function getToken($conf) -{ - $rnd = sha1(uniqid('', true) .'_'. mt_rand() . $conf->get('credentials.salt')); // We generate a random string. - $_SESSION['tokens'][$rnd]=1; // Store it on the server side. - return $rnd; -} - -// Tells if a token is OK. Using this function will destroy the token. -// true=token is OK. -function tokenOk($token) -{ - if (isset($_SESSION['tokens'][$token])) - { - unset($_SESSION['tokens'][$token]); // Token is used: destroy it. - return true; // Token is OK. - } - return false; // Wrong token, or already used. -} - /** * Daily RSS feed: 1 RSS entry per day giving all the links on that day. * Gives the last 7 days (which have links). @@ -687,12 +663,13 @@ function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) { /** * Render HTML page (according to URL parameters and user rights) * - * @param ConfigManager $conf Configuration Manager instance. - * @param PluginManager $pluginManager Plugin Manager instance, - * @param LinkDB $LINKSDB - * @param History $history instance + * @param ConfigManager $conf Configuration Manager instance. + * @param PluginManager $pluginManager Plugin Manager instance, + * @param LinkDB $LINKSDB + * @param History $history instance + * @param SessionManager $sessionManager SessionManager instance */ -function renderPage($conf, $pluginManager, $LINKSDB, $history) +function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager) { $updater = new Updater( read_updates_file($conf->get('resource.updates')), @@ -713,7 +690,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) die($e->getMessage()); } - $PAGE = new PageBuilder($conf, $LINKSDB); + $PAGE = new PageBuilder($conf, $LINKSDB, $sessionManager->generateToken()); $PAGE->assign('linkcount', count($LINKSDB)); $PAGE->assign('privateLinkcount', count_private($LINKSDB)); $PAGE->assign('plugin_errors', $pluginManager->getErrors()); @@ -1109,13 +1086,13 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) if (!empty($_POST['setpassword']) && !empty($_POST['oldpassword'])) { - if (!tokenOk($_POST['token'])) die(t('Wrong token.')); // Go away! + if (!$sessionManager->checkToken($_POST['token'])) die(t('Wrong token.')); // Go away! // Make sure old password is correct. $oldhash = sha1($_POST['oldpassword'].$conf->get('credentials.login').$conf->get('credentials.salt')); if ($oldhash!= $conf->get('credentials.hash')) { echo ''; - exit; + exit; } // Save new password // Salt renders rainbow-tables attacks useless. @@ -1149,7 +1126,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) { if (!empty($_POST['title']) ) { - if (!tokenOk($_POST['token'])) { + if (!$sessionManager->checkToken($_POST['token'])) { die(t('Wrong token.')); // Go away! } $tz = 'UTC'; @@ -1225,7 +1202,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) exit; } - if (!tokenOk($_POST['token'])) { + if (!$sessionManager->checkToken($_POST['token'])) { die(t('Wrong token.')); } @@ -1255,7 +1232,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) if (isset($_POST['save_edit'])) { // Go away! - if (! tokenOk($_POST['token'])) { + if (! $sessionManager->checkToken($_POST['token'])) { die(t('Wrong token.')); } @@ -1355,7 +1332,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) // -------- User clicked the "Delete" button when editing a link: Delete link from database. if ($targetPage == Router::$PAGE_DELETELINK) { - if (! tokenOk($_GET['token'])) { + if (! $sessionManager->checkToken($_GET['token'])) { die(t('Wrong token.')); } @@ -1572,7 +1549,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) echo ''; exit; } - if (! tokenOk($_POST['token'])) { + if (! $sessionManager->checkToken($_POST['token'])) { die('Wrong token.'); } $status = NetscapeBookmarkUtils::import( @@ -1639,7 +1616,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) // Get a fresh token if ($targetPage == Router::$GET_TOKEN) { header('Content-Type:text/plain'); - echo getToken($conf); + echo $sessionManager->generateToken($conf); exit; } @@ -1965,10 +1942,10 @@ function lazyThumbnail($conf, $url,$href=false) * Installation * This function should NEVER be called if the file data/config.php exists. * - * @param ConfigManager $conf Configuration Manager instance. + * @param ConfigManager $conf Configuration Manager instance. + * @param SessionManager $sessionManager SessionManager instance */ -function install($conf) -{ +function install($conf, $sessionManager) { // On free.fr host, make sure the /sessions directory exists, otherwise login will not work. if (endsWith($_SERVER['HTTP_HOST'],'.free.fr') && !is_dir($_SERVER['DOCUMENT_ROOT'].'/sessions')) mkdir($_SERVER['DOCUMENT_ROOT'].'/sessions',0705); @@ -2051,7 +2028,7 @@ function install($conf) exit; } - $PAGE = new PageBuilder($conf); + $PAGE = new PageBuilder($conf, null, $sessionManager->generateToken()); list($continents, $cities) = generateTimeZoneData(timezone_identifiers_list(), date_default_timezone_get()); $PAGE->assign('continents', $continents); $PAGE->assign('cities', $cities); @@ -2328,7 +2305,7 @@ $response = $app->run(true); if ($response->getStatusCode() == 404 && strpos($_SERVER['REQUEST_URI'], '/api/v1') === false) { // We use UTF-8 for proper international characters handling. header('Content-Type: text/html; charset=utf-8'); - renderPage($conf, $pluginManager, $linkDb, $history); + renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager); } else { $app->respond($response); } diff --git a/tests/SessionManagerTest.php b/tests/SessionManagerTest.php new file mode 100644 index 00000000..3a270303 --- /dev/null +++ b/tests/SessionManagerTest.php @@ -0,0 +1,72 @@ +generateToken(); + + $this->assertEquals(1, $session['tokens'][$token]); + $this->assertEquals(40, strlen($token)); + } + + /** + * Generate and check a session token + */ + public function testGenerateAndCheckToken() + { + $session = []; + $conf = new FakeConfigManager(); + $sessionManager = new SessionManager($session, $conf); + + $token = $sessionManager->generateToken(); + + // ensure a token has been generated + $this->assertEquals(1, $session['tokens'][$token]); + $this->assertEquals(40, strlen($token)); + + // check and destroy the token + $this->assertTrue($sessionManager->checkToken($token)); + $this->assertFalse(isset($session['tokens'][$token])); + + // ensure the token has been destroyed + $this->assertFalse($sessionManager->checkToken($token)); + } + + /** + * Check an invalid session token + */ + public function testCheckInvalidToken() + { + $session = []; + $conf = new FakeConfigManager(); + $sessionManager = new SessionManager($session, $conf); + + $this->assertFalse($sessionManager->checkToken('4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b')); + } +} -- cgit v1.2.3 From fd7d84616d53486c3a276a42da869390e1d7f5eb Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Sun, 22 Oct 2017 19:54:44 +0200 Subject: 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. */ -- cgit v1.2.3 From ae7c954b1279981cc23c9f67d88f55bfecc4d828 Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Tue, 24 Oct 2017 22:01:02 +0200 Subject: Improve SessionManager tests Signed-off-by: VirtualTam --- tests/SessionManagerTest.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/SessionManagerTest.php b/tests/SessionManagerTest.php index 9fa60dc5..a92c3ccc 100644 --- a/tests/SessionManagerTest.php +++ b/tests/SessionManagerTest.php @@ -50,6 +50,29 @@ class SessionManagerTest extends TestCase $this->assertEquals(40, strlen($token)); } + /** + * Check a session token + */ + public function testCheckToken() + { + $token = '4dccc3a45ad9d03e5542b90c37d8db6d10f2b38b'; + $session = [ + 'tokens' => [ + $token => 1, + ], + ]; + $conf = new FakeConfigManager(); + $sessionManager = new SessionManager($session, $conf); + + + // check and destroy the token + $this->assertTrue($sessionManager->checkToken($token)); + $this->assertFalse(isset($session['tokens'][$token])); + + // ensure the token has been destroyed + $this->assertFalse($sessionManager->checkToken($token)); + } + /** * Generate and check a session token */ -- cgit v1.2.3