From a3d77ce84b0e8f30a6cbf9f87d5ebf188dbcaa4c Mon Sep 17 00:00:00 2001 From: William Desportes Date: Wed, 15 May 2019 15:52:07 +0200 Subject: [PATCH 1/3] Fix #14184 - HTTPS/HTTP cookie separation issue (aka Failed to set session cookie. Maybe you are using HTTP instead of HTTPS) Duplicate-of-pull-request: #14420 Fixes: #14184 Signed-off-by: William Desportes --- libraries/classes/Config.php | 63 ++++++++++++++----- libraries/classes/ErrorReport.php | 5 +- libraries/classes/LanguageManager.php | 4 +- .../Plugins/Auth/AuthenticationCookie.php | 29 +++++---- .../classes/Plugins/AuthenticationPlugin.php | 5 +- libraries/classes/Session.php | 10 +-- libraries/classes/ThemeManager.php | 7 ++- libraries/classes/Url.php | 11 +++- libraries/common.inc.php | 6 +- 9 files changed, 98 insertions(+), 42 deletions(-) diff --git a/libraries/classes/Config.php b/libraries/classes/Config.php index aa265a67b9..39eac61d0e 100644 --- a/libraries/classes/Config.php +++ b/libraries/classes/Config.php @@ -987,7 +987,7 @@ class Config } // save language - if (isset($_COOKIE['pma_lang']) || isset($_POST['lang'])) { + if ($this->issetCookie('pma_lang') || isset($_POST['lang'])) { if ((! isset($config_data['lang']) && $GLOBALS['lang'] != 'en') || isset($config_data['lang']) @@ -1061,7 +1061,7 @@ class Config */ public function getUserValue($cookie_name, $cfg_value) { - $cookie_exists = isset($_COOKIE) && !empty($_COOKIE[$cookie_name]); + $cookie_exists = isset($_COOKIE) && !empty($this->getCookie($cookie_name)); $prefs_type = $this->get('user_preferences'); if ($prefs_type == 'db') { // permanent user preferences value exists, remove cookie @@ -1069,7 +1069,7 @@ class Config $this->removeCookie($cookie_name); } } elseif ($cookie_exists) { - return $_COOKIE[$cookie_name]; + return $this->getCookie($cookie_name); } // return value from $cfg array return $cfg_value; @@ -1528,20 +1528,22 @@ class Config /** * removes cookie * - * @param string $cookie name of cookie to remove + * @param string $cookieName name of cookie to remove * * @return boolean result of setcookie() */ - public function removeCookie($cookie) + public function removeCookie($cookieName) { + $httpCookieName = $this->getCookieName($cookieName); + + if ($this->issetCookie($cookieName)) { + unset($_COOKIE[$httpCookieName]); + } if (defined('TESTSUITE')) { - if (isset($_COOKIE[$cookie])) { - unset($_COOKIE[$cookie]); - } return true; } return setcookie( - $cookie, + $httpCookieName, '', time() - 3600, $this->getRootPath(), @@ -1568,19 +1570,21 @@ class Config if (strlen($value) > 0 && null !== $default && $value === $default ) { // default value is used - if (isset($_COOKIE[$cookie])) { + if ($this->issetCookie($cookie)) { // remove cookie return $this->removeCookie($cookie); } return false; } - if (strlen($value) === 0 && isset($_COOKIE[$cookie])) { + if (strlen($value) === 0 && $this->issetCookie($cookie)) { // remove cookie, value is empty return $this->removeCookie($cookie); } - if (! isset($_COOKIE[$cookie]) || $_COOKIE[$cookie] !== $value) { + $httpCookieName = $this->getCookieName($cookie); + + if (! $this->issetCookie($cookie) || $this->getCookie($cookie) !== $value) { // set cookie with new value /* Calculate cookie validity */ if ($validity === null) { @@ -1593,11 +1597,11 @@ class Config $validity = time() + $validity; } if (defined('TESTSUITE')) { - $_COOKIE[$cookie] = $value; + $_COOKIE[$httpCookieName] = $value; return true; } return setcookie( - $cookie, + $httpCookieName, $value, $validity, $this->getRootPath(), @@ -1611,6 +1615,37 @@ class Config return true; } + /** + * get cookie + * + * @param string $cookieName The name of the cookie to get + * + * @return mixed result of getCookie() + */ + public function getCookie(string $cookieName) { + return @$_COOKIE[$this->getCookieName($cookieName)]; + } + + /** + * Get the real cookie name + * + * @param string $cookieName The name of the cookie + * @return string + */ + public function getCookieName(string $cookieName) { + return $cookieName. ( ($this->isHttps()) ? '_https' : '' ); + } + + /** + * isset cookie + * + * @param string $cookieName The name of the cookie to check + * + * @return bool result of issetCookie() + */ + public function issetCookie(string $cookieName) { + return isset($_COOKIE[$this->getCookieName($cookieName)]); + } /** * Error handler to catch fatal errors when loading configuration diff --git a/libraries/classes/ErrorReport.php b/libraries/classes/ErrorReport.php index d11462a67a..6a3e65a230 100644 --- a/libraries/classes/ErrorReport.php +++ b/libraries/classes/ErrorReport.php @@ -72,6 +72,9 @@ class ErrorReport */ public function getData($exceptionType = 'js') { + /** @var Config $PMA_Config */ + global $PMA_Config; + $relParams = $this->relation->getRelationsParam(); // common params for both, php & js exceptions $report = [ @@ -81,7 +84,7 @@ class ErrorReport "user_os" => PMA_USR_OS, "server_software" => $_SERVER['SERVER_SOFTWARE'], "user_agent_string" => $_SERVER['HTTP_USER_AGENT'], - "locale" => $_COOKIE['pma_lang'], + "locale" => $PMA_Config->getCookie('pma_lang'), "configuration_storage" => is_null($relParams['db']) ? "disabled" : "enabled", "php_version" => phpversion() diff --git a/libraries/classes/LanguageManager.php b/libraries/classes/LanguageManager.php index 6e117df9eb..692b8d9ceb 100644 --- a/libraries/classes/LanguageManager.php +++ b/libraries/classes/LanguageManager.php @@ -864,8 +864,8 @@ class LanguageManager } // check previous set language - if (! empty($_COOKIE['pma_lang'])) { - $lang = $this->getLanguage($_COOKIE['pma_lang']); + if (! empty($GLOBALS['PMA_Config']->getCookie('pma_lang'))) { + $lang = $this->getLanguage($GLOBALS['PMA_Config']->getCookie('pma_lang')); if ($lang !== false) { return $lang; } diff --git a/libraries/classes/Plugins/Auth/AuthenticationCookie.php b/libraries/classes/Plugins/Auth/AuthenticationCookie.php index a681f60db3..997fe9632d 100644 --- a/libraries/classes/Plugins/Auth/AuthenticationCookie.php +++ b/libraries/classes/Plugins/Auth/AuthenticationCookie.php @@ -357,12 +357,13 @@ class AuthenticationCookie extends AuthenticationPlugin // and $this->password variables from cookies // check cookies - if (empty($_COOKIE['pmaUser-' . $GLOBALS['server']])) { + $serverCookie = $GLOBALS['PMA_Config']->getCookie('pmaUser-' . $GLOBALS['server']); + if (empty($serverCookie)) { return false; } $value = $this->cookieDecrypt( - $_COOKIE['pmaUser-' . $GLOBALS['server']], + $serverCookie, $this->_getEncryptionSecret() ); @@ -404,11 +405,13 @@ class AuthenticationCookie extends AuthenticationPlugin } // check password cookie - if (empty($_COOKIE['pmaAuth-' . $GLOBALS['server']])) { + $serverCookie = $GLOBALS['PMA_Config']->getCookie('pmaAuth-' . $GLOBALS['server']); + + if (empty($serverCookie)) { return false; } $value = $this->cookieDecrypt( - $_COOKIE['pmaAuth-' . $GLOBALS['server']], + $serverCookie, $this->_getSessionEncryptionSecret() ); if ($value === false) { @@ -868,20 +871,22 @@ class AuthenticationCookie extends AuthenticationPlugin */ public function logOut() { + /** @var Config $PMA_Config */ + global $PMA_Config; + // -> delete password cookie(s) if ($GLOBALS['cfg']['LoginCookieDeleteAll']) { foreach ($GLOBALS['cfg']['Servers'] as $key => $val) { - $GLOBALS['PMA_Config']->removeCookie('pmaAuth-' . $key); - if (isset($_COOKIE['pmaAuth-' . $key])) { - unset($_COOKIE['pmaAuth-' . $key]); + $PMA_Config->removeCookie('pmaAuth-' . $key); + if ($PMA_Config->issetCookie('pmaAuth-' . $key)) { + $PMA_Config->removeCookie('pmaAuth-' . $key); } } } else { - $GLOBALS['PMA_Config']->removeCookie( - 'pmaAuth-' . $GLOBALS['server'] - ); - if (isset($_COOKIE['pmaAuth-' . $GLOBALS['server']])) { - unset($_COOKIE['pmaAuth-' . $GLOBALS['server']]); + $cookieName = 'pmaAuth-' . $GLOBALS['server']; + $PMA_Config->removeCookie($cookieName); + if ($PMA_Config->issetCookie($cookieName)) { + $PMA_Config->removeCookie($cookieName); } } parent::logOut(); diff --git a/libraries/classes/Plugins/AuthenticationPlugin.php b/libraries/classes/Plugins/AuthenticationPlugin.php index b174df0173..7e3edf6dbf 100644 --- a/libraries/classes/Plugins/AuthenticationPlugin.php +++ b/libraries/classes/Plugins/AuthenticationPlugin.php @@ -100,6 +100,9 @@ abstract class AuthenticationPlugin */ public function logOut() { + /** @var Config $PMA_Config */ + global $PMA_Config; + /* Obtain redirect URL (before doing logout) */ if (! empty($GLOBALS['cfg']['Server']['LogoutURL'])) { $redirect_url = $GLOBALS['cfg']['Server']['LogoutURL']; @@ -119,7 +122,7 @@ abstract class AuthenticationPlugin && $GLOBALS['cfg']['Server']['auth_type'] == 'cookie' ) { foreach ($GLOBALS['cfg']['Servers'] as $key => $val) { - if (isset($_COOKIE['pmaAuth-' . $key])) { + if ($PMA_Config->issetCookie('pmaAuth-' . $key)) { $server = $key; } } diff --git a/libraries/classes/Session.php b/libraries/classes/Session.php index 7e4cf62811..4290fd6081 100644 --- a/libraries/classes/Session.php +++ b/libraries/classes/Session.php @@ -175,12 +175,12 @@ class Session // proxy servers session_cache_limiter('private'); - $session_name = 'phpMyAdmin'; - @session_name($session_name); + $httpCookieName = $config->getCookieName('phpMyAdmin'); + @session_name($httpCookieName); // Restore correct sesion ID (it might have been reset by auto started session - if (isset($_COOKIE['phpMyAdmin'])) { - session_id($_COOKIE['phpMyAdmin']); + if ($config->issetCookie('phpMyAdmin')) { + session_id($config->getCookie('phpMyAdmin')); } // on first start of session we check for errors @@ -192,7 +192,7 @@ class Session if ($session_result !== true || $orig_error_count != $errorHandler->countErrors(false) ) { - setcookie($session_name, '', 1); + setcookie($httpCookieName, '', 1); $errors = $errorHandler->sliceErrors($orig_error_count); self::sessionFailed($errors); } diff --git a/libraries/classes/ThemeManager.php b/libraries/classes/ThemeManager.php index 9d7ff2c2de..0ac38fcfee 100644 --- a/libraries/classes/ThemeManager.php +++ b/libraries/classes/ThemeManager.php @@ -212,9 +212,12 @@ class ThemeManager */ public function getThemeCookie() { + /** @var Config $PMA_Config */ + global $PMA_Config; + $name = $this->getThemeCookieName(); - if (isset($_COOKIE[$name])) { - return $_COOKIE[$name]; + if ($PMA_Config->issetCookie($name)) { + return $PMA_Config->getCookie($name); } return false; diff --git a/libraries/classes/Url.php b/libraries/classes/Url.php index 6733d1265c..f527819fbb 100644 --- a/libraries/classes/Url.php +++ b/libraries/classes/Url.php @@ -33,6 +33,9 @@ class Url public static function getHiddenInputs($db = '', $table = '', $indent = 0, $skip = array() ) { + /** @var Config $PMA_Config */ + global $PMA_Config; + if (is_array($db)) { $params =& $db; $_indent = empty($table) ? $indent : $table; @@ -54,7 +57,7 @@ class Url ) { $params['server'] = $GLOBALS['server']; } - if (empty($_COOKIE['pma_lang']) && ! empty($GLOBALS['lang'])) { + if (empty($PMA_Config->getCookie('pma_lang')) && ! empty($GLOBALS['lang'])) { $params['lang'] = $GLOBALS['lang']; } @@ -198,18 +201,20 @@ class Url */ public static function getCommonRaw($params = array(), $divider = '?') { + /** @var Config $PMA_Config */ + global $PMA_Config; $separator = Url::getArgSeparator(); // avoid overwriting when creating navi panel links to servers if (isset($GLOBALS['server']) && $GLOBALS['server'] != $GLOBALS['cfg']['ServerDefault'] && ! isset($params['server']) - && ! $GLOBALS['PMA_Config']->get('is_setup') + && ! $PMA_Config->get('is_setup') ) { $params['server'] = $GLOBALS['server']; } - if (empty($_COOKIE['pma_lang']) && ! empty($GLOBALS['lang'])) { + if (empty($PMA_Config->getCookie('pma_lang')) && ! empty($GLOBALS['lang'])) { $params['lang'] = $GLOBALS['lang']; } diff --git a/libraries/common.inc.php b/libraries/common.inc.php index ed92fea624..b938bd0098 100644 --- a/libraries/common.inc.php +++ b/libraries/common.inc.php @@ -152,7 +152,8 @@ if (Core::checkPageValidity($_REQUEST['goto'])) { $GLOBALS['goto'] = $_REQUEST['goto']; $GLOBALS['url_params']['goto'] = $_REQUEST['goto']; } else { - unset($_REQUEST['goto'], $_GET['goto'], $_POST['goto'], $_COOKIE['goto']); + $GLOBALS['PMA_Config']->removeCookie('goto'); + unset($_REQUEST['goto'], $_GET['goto'], $_POST['goto']); } /** @@ -162,7 +163,8 @@ if (Core::checkPageValidity($_REQUEST['goto'])) { if (Core::checkPageValidity($_REQUEST['back'])) { $GLOBALS['back'] = $_REQUEST['back']; } else { - unset($_REQUEST['back'], $_GET['back'], $_POST['back'], $_COOKIE['back']); + $GLOBALS['PMA_Config']->removeCookie('back'); + unset($_REQUEST['back'], $_GET['back'], $_POST['back']); } /** From 09464ba8f5fecc3cbba61c0be7ae7b1b592a92f2 Mon Sep 17 00:00:00 2001 From: William Desportes Date: Wed, 15 May 2019 21:17:41 +0200 Subject: [PATCH 2/3] Fix unit tests Signed-off-by: William Desportes --- test/classes/ConfigTest.php | 1 + test/classes/LanguageTest.php | 1 + test/classes/Plugins/Auth/AuthenticationCookieTest.php | 9 +++++++++ 3 files changed, 11 insertions(+) diff --git a/test/classes/ConfigTest.php b/test/classes/ConfigTest.php index 954f21047e..3726d27528 100644 --- a/test/classes/ConfigTest.php +++ b/test/classes/ConfigTest.php @@ -863,6 +863,7 @@ class ConfigTest extends PmaTestCase */ public function testSetCookie() { + $this->object->set('is_https', false); $this->assertFalse( $this->object->setCookie( 'TEST_DEF_COOKIE', diff --git a/test/classes/LanguageTest.php b/test/classes/LanguageTest.php index 72c9032afc..f225b969fa 100644 --- a/test/classes/LanguageTest.php +++ b/test/classes/LanguageTest.php @@ -159,6 +159,7 @@ class LanguageTest extends PmaTestCase public function testSelect($lang, $post, $get, $cookie, $accept, $agent, $default, $expect) { $GLOBALS['PMA_Config']->set('Lang', $lang); + $GLOBALS['PMA_Config']->set('is_https', false); $_POST['lang'] = $post; $_GET['lang'] = $get; $_COOKIE['pma_lang'] = $cookie; diff --git a/test/classes/Plugins/Auth/AuthenticationCookieTest.php b/test/classes/Plugins/Auth/AuthenticationCookieTest.php index 576ca7d49a..fc5e034c20 100644 --- a/test/classes/Plugins/Auth/AuthenticationCookieTest.php +++ b/test/classes/Plugins/Auth/AuthenticationCookieTest.php @@ -366,6 +366,7 @@ class AuthenticationCookieTest extends PmaTestCase */ public function testAuthHeaderPartial() { + $GLOBALS['PMA_Config']->set('is_https', false); $GLOBALS['cfg']['LoginCookieDeleteAll'] = false; $GLOBALS['cfg']['Servers'] = array(1, 2, 3); $GLOBALS['cfg']['Server']['LogoutURL'] = 'https://example.com/logout'; @@ -413,6 +414,7 @@ class AuthenticationCookieTest extends PmaTestCase $GLOBALS['cfg']['CaptchaLoginPublicKey'] = ''; $GLOBALS['cfg']['LoginCookieDeleteAll'] = true; $GLOBALS['PMA_Config']->set('PmaAbsoluteUri', ''); + $GLOBALS['PMA_Config']->set('is_https', false); $GLOBALS['cfg']['Servers'] = array(1); $_COOKIE['pmaAuth-0'] = 'test'; @@ -438,6 +440,7 @@ class AuthenticationCookieTest extends PmaTestCase $GLOBALS['cfg']['CaptchaLoginPublicKey'] = ''; $GLOBALS['cfg']['LoginCookieDeleteAll'] = false; $GLOBALS['PMA_Config']->set('PmaAbsoluteUri', ''); + $GLOBALS['PMA_Config']->set('is_https', false); $GLOBALS['cfg']['Servers'] = array(1); $GLOBALS['server'] = 1; $GLOBALS['cfg']['Server'] = array('auth_type' => 'cookie'); @@ -550,6 +553,7 @@ class AuthenticationCookieTest extends PmaTestCase $_SESSION['last_access_time'] = ''; $GLOBALS['cfg']['CaptchaLoginPrivateKey'] = ''; $GLOBALS['cfg']['CaptchaLoginPublicKey'] = ''; + $GLOBALS['PMA_Config']->set('is_https', false); // mock for blowfish function $this->object = $this->getMockBuilder('PhpMyAdmin\Plugins\Auth\AuthenticationCookie') @@ -590,6 +594,7 @@ class AuthenticationCookieTest extends PmaTestCase $GLOBALS['cfg']['CaptchaLoginPublicKey'] = ''; $_SESSION['browser_access_time']['default'] = time() - 1000; $GLOBALS['cfg']['LoginCookieValidity'] = 1440; + $GLOBALS['PMA_Config']->set('is_https', false); // mock for blowfish function $this->object = $this->getMockBuilder('PhpMyAdmin\Plugins\Auth\AuthenticationCookie') @@ -635,6 +640,8 @@ class AuthenticationCookieTest extends PmaTestCase $GLOBALS['cfg']['CaptchaLoginPublicKey'] = ''; $GLOBALS['cfg']['LoginCookieValidity'] = 0; $_SESSION['browser_access_time']['default'] = -1; + $GLOBALS['PMA_Config']->set('is_https', false); + // mock for blowfish function $this->object = $this->getMockBuilder('PhpMyAdmin\Plugins\Auth\AuthenticationCookie') ->disableOriginalConstructor() @@ -678,6 +685,7 @@ class AuthenticationCookieTest extends PmaTestCase $GLOBALS['server'] = 2; $GLOBALS['cfg']['LoginCookieStore'] = true; $GLOBALS['from_cookie'] = true; + $GLOBALS['PMA_Config']->set('is_https', false); $this->object->storeCredentials(); @@ -1064,6 +1072,7 @@ class AuthenticationCookieTest extends PmaTestCase public function testPasswordChange() { $newPassword = 'PMAPASSWD2'; + $GLOBALS['PMA_Config']->set('is_https', false); $GLOBALS['cfg']['AllowArbitraryServer'] = true; $GLOBALS['pma_auth_server'] = 'b 2'; $_SESSION['encryption_key'] = ''; From 45d46a6316c7a79d8c110ccbd18035c4d0c633fb Mon Sep 17 00:00:00 2001 From: William Desportes Date: Wed, 15 May 2019 21:26:45 +0200 Subject: [PATCH 3/3] Remove typehints for hhvm Signed-off-by: William Desportes --- libraries/classes/Config.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/classes/Config.php b/libraries/classes/Config.php index 39eac61d0e..969db189f6 100644 --- a/libraries/classes/Config.php +++ b/libraries/classes/Config.php @@ -1622,7 +1622,7 @@ class Config * * @return mixed result of getCookie() */ - public function getCookie(string $cookieName) { + public function getCookie($cookieName) { return @$_COOKIE[$this->getCookieName($cookieName)]; } @@ -1632,7 +1632,7 @@ class Config * @param string $cookieName The name of the cookie * @return string */ - public function getCookieName(string $cookieName) { + public function getCookieName($cookieName) { return $cookieName. ( ($this->isHttps()) ? '_https' : '' ); } @@ -1643,7 +1643,7 @@ class Config * * @return bool result of issetCookie() */ - public function issetCookie(string $cookieName) { + public function issetCookie($cookieName) { return isset($_COOKIE[$this->getCookieName($cookieName)]); }