Improve handling of logout

- add separate script for handling logout - no longer require old_usr for all authentication methods (this avoids potential information leak) - require valid token for logout Signed-off-by: Michal Čihař <michal@cihar.com>
2016-05-23 13:02:21 +02:00 · 2016-05-23 13:02:21 +02:00 · 11eb574242
commit 11eb574242
parent 381c80dda4
10 changed files with 175 additions and 104 deletions
--- a/1
+++ b/1
@ -12,6 +12,7 @@ phpMyAdmin - ChangeLog
 - issue #11705 Fix occassional 200 errors on Windows
 - issue #12219 Fix locking issues when importing SQL
 - issue #12231 Avoid confusing warning when mysql extension is missing
+- issue        Improve handling of logout

 4.6.1 (2016-05-02)
 - issue #12120 PMA_Util not found in insert_edit.lib.php
--- a/libraries/navigation/NavigationHeader.php
+++ b/libraries/navigation/NavigationHeader.php
@ -167,8 +167,7 @@ class NavigationHeader
        if ($GLOBALS['server'] != 0) {
            // Logout for advanced authentication
            if ($GLOBALS['cfg']['Server']['auth_type'] != 'config') {
-                $link = 'index.php' . $GLOBALS['url_query'];
-                $link .= '&amp;old_usr=' . urlencode($GLOBALS['PHP_AUTH_USER']);
+                $link = 'logout.php' . $GLOBALS['url_query'];
                $retval .= PMA\libraries\Util::getNavigationLink(
                    $link,
                    $showText,
--- a/libraries/plugins/AuthenticationPlugin.php
+++ b/libraries/plugins/AuthenticationPlugin.php
@ -52,6 +52,46 @@ abstract class AuthenticationPlugin
     */
    abstract public function authFails();

+    /**
+     * Perform logout
+     *
+     * @return void
+     */
+    public function logOut()
+    {
+        global $PHP_AUTH_USER, $PHP_AUTH_PW;
+
+        /* Obtain redirect URL (before doing logout) */
+        if (! empty($GLOBALS['cfg']['Server']['LogoutURL'])) {
+            $redirect_url = $GLOBALS['cfg']['Server']['LogoutURL'];
+        } else {
+            $redirect_url = $this->getLoginFormURL();
+        }
+
+        /* Clear credentials */
+        $PHP_AUTH_USER = '';
+        $PHP_AUTH_PW = '';
+
+        /* delete user's choices that were stored in session */
+        $_SESSION = array();
+        if (!defined('TESTSUITE')) {
+            session_destroy();
+        }
+
+        /* Redirect to login form (or configured URL) */
+        PMA_sendHeaderLocation($redirect_url);
+    }
+
+    /**
+     * Returns URL for login form.
+     *
+     * @return string
+     */
+    public function getLoginFormURL()
+    {
+        return './index.php';
+    }
+
    /**
     * Returns error message for failed authentication.
     *
--- a/libraries/plugins/auth/AuthenticationCookie.php
+++ b/libraries/plugins/auth/AuthenticationCookie.php
@ -67,18 +67,6 @@ class AuthenticationCookie extends AuthenticationPlugin
            }
        }

-        /* Perform logout to custom URL */
-        if (! empty($_REQUEST['old_usr'])
-            && ! empty($GLOBALS['cfg']['Server']['LogoutURL'])
-        ) {
-            PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
-            if (defined('TESTSUITE')) {
-                return true;
-            } else {
-                exit;
-            }
-        }
-
        // No recall if blowfish secret is not configured as it would produce
        // garbage
        if ($GLOBALS['cfg']['LoginCookieRecall']
@ -295,34 +283,6 @@ class AuthenticationCookie extends AuthenticationPlugin
        }
        // END Swekey Integration

-        if (! empty($_REQUEST['old_usr'])) {
-            // The user wants to be logged out
-            // -> delete his choices that were stored in session
-
-            // according to the PHP manual we should do this before the destroy:
-            //$_SESSION = array();
-
-            if (! defined('TESTSUITE')) {
-                session_destroy();
-            }
-            // -> delete password cookie(s)
-            if ($GLOBALS['cfg']['LoginCookieDeleteAll']) {
-                foreach ($GLOBALS['cfg']['Servers'] as $key => $val) {
-                    $GLOBALS['PMA_Config']->removeCookie('pmaPass-' . $key);
-                    if (isset($_COOKIE['pmaPass-' . $key])) {
-                        unset($_COOKIE['pmaPass-' . $key]);
-                    }
-                }
-            } else {
-                $GLOBALS['PMA_Config']->removeCookie(
-                    'pmaPass-' . $GLOBALS['server']
-                );
-                if (isset($_COOKIE['pmaPass-' . $GLOBALS['server']])) {
-                    unset($_COOKIE['pmaPass-' . $GLOBALS['server']]);
-                }
-            }
-        }
-
        if (! empty($_REQUEST['pma_username'])) {

            // Verify Captcha if it is required.
@ -831,4 +791,30 @@ class AuthenticationCookie extends AuthenticationPlugin
    {
        $this->storePasswordCookie($password);
    }
+
+    /**
+     * Perform logout
+     *
+     * @return void
+     */
+    public function logOut()
+    {
+        // -> delete password cookie(s)
+        if ($GLOBALS['cfg']['LoginCookieDeleteAll']) {
+            foreach ($GLOBALS['cfg']['Servers'] as $key => $val) {
+                $GLOBALS['PMA_Config']->removeCookie('pmaPass-' . $key);
+                if (isset($_COOKIE['pmaPass-' . $key])) {
+                    unset($_COOKIE['pmaPass-' . $key]);
+                }
+            }
+        } else {
+            $GLOBALS['PMA_Config']->removeCookie(
+                'pmaPass-' . $GLOBALS['server']
+            );
+            if (isset($_COOKIE['pmaPass-' . $GLOBALS['server']])) {
+                unset($_COOKIE['pmaPass-' . $GLOBALS['server']]);
+            }
+        }
+        parent::logOut();
+    }
 }
--- a/libraries/plugins/auth/AuthenticationHttp.php
+++ b/libraries/plugins/auth/AuthenticationHttp.php
@ -49,18 +49,6 @@ class AuthenticationHttp extends AuthenticationPlugin
     */
    public function authForm()
    {
-        /* Perform logout to custom URL */
-        if (!empty($_REQUEST['old_usr'])
-            && !empty($GLOBALS['cfg']['Server']['LogoutURL'])
-        ) {
-            PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
-            if (!defined('TESTSUITE')) {
-                exit;
-            } else {
-                return false;
-            }
-        }
-
        if (empty($GLOBALS['cfg']['Server']['auth_http_realm'])) {
            if (empty($GLOBALS['cfg']['Server']['verbose'])) {
                $server_message = $GLOBALS['cfg']['Server']['host'];
@ -262,4 +250,14 @@ class AuthenticationHttp extends AuthenticationPlugin

        return true;
    }
+
+    /**
+     * Returns URL for login form.
+     *
+     * @return string
+     */
+    public function getLoginFormURL()
+    {
+        return './index.php?old_usr=' . $GLOBALS['PHP_AUTH_USER'];
+    }
 }
--- a/libraries/plugins/auth/AuthenticationSignon.php
+++ b/libraries/plugins/auth/AuthenticationSignon.php
@ -28,11 +28,6 @@ class AuthenticationSignon extends AuthenticationPlugin
        unset($_SESSION['LAST_SIGNON_URL']);
        if (empty($GLOBALS['cfg']['Server']['SignonURL'])) {
            PMA_fatalError('You must set SignonURL!');
-        } elseif (!empty($_REQUEST['old_usr'])
-            && !empty($GLOBALS['cfg']['Server']['LogoutURL'])
-        ) {
-            /* Perform logout to custom URL */
-            PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['LogoutURL']);
        } else {
            PMA_sendHeaderLocation($GLOBALS['cfg']['Server']['SignonURL']);
        }
@ -82,9 +77,6 @@ class AuthenticationSignon extends AuthenticationPlugin
        /* No configuration updates */
        $single_signon_cfgupdate = array();

-        /* Are we requested to do logout? */
-        $do_logout = !empty($_REQUEST['old_usr']);
-
        /* Handle script based auth */
        if (!empty($script_name)) {
            if (!file_exists($script_name)) {
@ -117,18 +109,10 @@ class AuthenticationSignon extends AuthenticationPlugin

            /* Grab credentials if they exist */
            if (isset($_SESSION['PMA_single_signon_user'])) {
-                if ($do_logout) {
-                    $PHP_AUTH_USER = '';
-                } else {
-                    $PHP_AUTH_USER = $_SESSION['PMA_single_signon_user'];
-                }
+                $PHP_AUTH_USER = $_SESSION['PMA_single_signon_user'];
            }
            if (isset($_SESSION['PMA_single_signon_password'])) {
-                if ($do_logout) {
-                    $PHP_AUTH_PW = '';
-                } else {
-                    $PHP_AUTH_PW = $_SESSION['PMA_single_signon_password'];
-                }
+                $PHP_AUTH_PW = $_SESSION['PMA_single_signon_password'];
            }
            if (isset($_SESSION['PMA_single_signon_host'])) {
                $single_signon_host = $_SESSION['PMA_single_signon_host'];
--- a/logout.php
+++ b/logout.php
@ -0,0 +1,15 @@
+<?php
+/* vim: set expandtab sw=4 ts=4 sts=4: */
+/**
+ * Logout script
+ *
+ * @package PhpMyAdmin
+ */
+require_once 'libraries/common.inc.php';
+
+if ($token_mismatch) {
+    PMA_sendHeaderLocation('./index.php');
+} else {
+    $auth_plugin->logOut();
+}
+
--- a/test/classes/plugin/auth/AuthenticationCookieTest.php
+++ b/test/classes/plugin/auth/AuthenticationCookieTest.php
@ -373,6 +373,9 @@ class AuthenticationCookieTest extends PMATestCase
     */
    public function testAuthHeader()
    {
+        $GLOBALS['cfg']['LoginCookieDeleteAll'] = false;
+        $GLOBALS['cfg']['Servers'] = array(1);
+
        $restoreInstance = PMA\libraries\Response::getInstance();

        $mockResponse = $this->getMockBuilder('PMA\libraries\Response')
@ -380,11 +383,6 @@ class AuthenticationCookieTest extends PMATestCase
            ->setMethods(array('isAjax', 'headersSent', 'header'))
            ->getMock();

-        $mockResponse->expects($this->once())
-            ->method('isAjax')
-            ->with()
-            ->will($this->returnValue(false));
-
        $mockResponse->expects($this->any())
            ->method('headersSent')
            ->with()
@ -398,12 +396,9 @@ class AuthenticationCookieTest extends PMATestCase
        $attrInstance->setAccessible(true);
        $attrInstance->setValue($mockResponse);

-        $_REQUEST['old_usr'] = 'user1';
        $GLOBALS['cfg']['Server']['LogoutURL'] = 'http://www.phpmyadmin.net/logout';

-        $this->assertTrue(
-            $this->object->auth()
-        );
+        $this->object->logOut();

        $attrInstance->setValue($restoreInstance);
    }
@ -454,20 +449,40 @@ class AuthenticationCookieTest extends PMATestCase
     */
    public function testLogoutDelete()
    {
+        $restoreInstance = PMA\libraries\Response::getInstance();
+
+        $mockResponse = $this->getMockBuilder('PMA\libraries\Response')
+            ->disableOriginalConstructor()
+            ->setMethods(array('isAjax', 'headersSent', 'header'))
+            ->getMock();
+
+        $mockResponse->expects($this->any())
+            ->method('headersSent')
+            ->with()
+            ->will($this->returnValue(false));
+
+        $mockResponse->expects($this->once())
+            ->method('header')
+            ->with('Location: ./index.php' . ((SID) ? '?' . SID : ''));
+
+        $attrInstance = new ReflectionProperty('PMA\libraries\Response', '_instance');
+        $attrInstance->setAccessible(true);
+        $attrInstance->setValue($mockResponse);
+
        $GLOBALS['cfg']['Server']['auth_swekey_config'] = '';
        $GLOBALS['cfg']['CaptchaLoginPrivateKey'] = '';
        $GLOBALS['cfg']['CaptchaLoginPublicKey'] = '';
-        $_REQUEST['old_usr'] = 'pmaolduser';
        $GLOBALS['cfg']['LoginCookieDeleteAll'] = true;
        $GLOBALS['cfg']['Servers'] = array(1);

        $_COOKIE['pmaPass-0'] = 'test';

-        $this->object->authCheck();
+        $this->object->logOut();

        $this->assertFalse(
            isset($_COOKIE['pmaPass-0'])
        );
+        $attrInstance->setValue($restoreInstance);
    }

    /**
@ -477,21 +492,40 @@ class AuthenticationCookieTest extends PMATestCase
     */
    public function testLogout()
    {
+        $restoreInstance = PMA\libraries\Response::getInstance();
+
+        $mockResponse = $this->getMockBuilder('PMA\libraries\Response')
+            ->disableOriginalConstructor()
+            ->setMethods(array('isAjax', 'headersSent', 'header'))
+            ->getMock();
+
+        $mockResponse->expects($this->any())
+            ->method('headersSent')
+            ->with()
+            ->will($this->returnValue(false));
+
+        $mockResponse->expects($this->once())
+            ->method('header')
+            ->with('Location: ./index.php' . ((SID) ? '?' . SID : ''));
+
+        $attrInstance = new ReflectionProperty('PMA\libraries\Response', '_instance');
+        $attrInstance->setAccessible(true);
+        $attrInstance->setValue($mockResponse);
        $GLOBALS['cfg']['Server']['auth_swekey_config'] = '';
        $GLOBALS['cfg']['CaptchaLoginPrivateKey'] = '';
        $GLOBALS['cfg']['CaptchaLoginPublicKey'] = '';
-        $_REQUEST['old_usr'] = 'pmaolduser';
        $GLOBALS['cfg']['LoginCookieDeleteAll'] = false;
        $GLOBALS['cfg']['Servers'] = array(1);
        $GLOBALS['server'] = 1;

        $_COOKIE['pmaPass-1'] = 'test';

-        $this->object->authCheck();
+        $this->object->logOut();

        $this->assertFalse(
            isset($_COOKIE['pmaPass-1'])
        );
+        $attrInstance->setValue($restoreInstance);
    }

    /**
--- a/test/classes/plugin/auth/AuthenticationHttpTest.php
+++ b/test/classes/plugin/auth/AuthenticationHttpTest.php
@ -123,9 +123,13 @@ class AuthenticationHttpTest extends PMATestCase
        call_user_func_array(array($header_method, 'withConsecutive'), $headers);

        try {
-            $this->assertFalse(
-                $this->object->auth()
-            );
+            if (!empty($_REQUEST['old_usr'])) {
+                $this->object->logOut();
+            } else {
+                $this->assertFalse(
+                    $this->object->auth()
+                );
+            }
        } finally {
            $attrInstance->setValue($restoreInstance);
        }
--- a/test/classes/plugin/auth/AuthenticationSignonTest.php
+++ b/test/classes/plugin/auth/AuthenticationSignonTest.php
@ -73,10 +73,9 @@ class AuthenticationSignonTest extends PMATestCase
        // case 2

        $GLOBALS['cfg']['Server']['SignonURL'] = 'http://phpmyadmin.net/SignonURL';
-        $_REQUEST['old_usr'] = 'oldUser';
        $GLOBALS['cfg']['Server']['LogoutURL'] = 'http://phpmyadmin.net/logoutURL';

-        $this->object->auth();
+        $this->object->logOut();

        $this->assertContains(
            'Location: http://phpmyadmin.net/logoutURL?PHPSESSID=',
@ -87,10 +86,9 @@ class AuthenticationSignonTest extends PMATestCase

        $GLOBALS['header'] = array();
        $GLOBALS['cfg']['Server']['SignonURL'] = 'http://phpmyadmin.net/SignonURL';
-        $_REQUEST['old_usr'] = '';
        $GLOBALS['cfg']['Server']['LogoutURL'] = '';

-        $this->object->auth();
+        $this->object->logOut();

        $this->assertContains(
            'Location: http://phpmyadmin.net/SignonURL?PHPSESSID=',
@ -155,6 +153,26 @@ class AuthenticationSignonTest extends PMATestCase
     */
    public function testAuthCheckToken()
    {
+        $restoreInstance = PMA\libraries\Response::getInstance();
+
+        $mockResponse = $this->getMockBuilder('PMA\libraries\Response')
+            ->disableOriginalConstructor()
+            ->setMethods(array('isAjax', 'headersSent', 'header'))
+            ->getMock();
+
+        $mockResponse->expects($this->any())
+            ->method('headersSent')
+            ->with()
+            ->will($this->returnValue(false));
+
+        $mockResponse->expects($this->once())
+            ->method('header')
+            ->with('Location: ./index.php' . ((SID) ? '?' . SID : ''));
+
+        $attrInstance = new ReflectionProperty('PMA\libraries\Response', '_instance');
+        $attrInstance->setAccessible(true);
+        $attrInstance->setValue($mockResponse);
+
        $GLOBALS['cfg']['Server']['SignonURL'] = 'http://phpmyadmin.net/SignonURL';
        $GLOBALS['cfg']['Server']['SignonSession'] = 'session123';
        $GLOBALS['cfg']['Server']['host'] = 'localhost';
@ -162,7 +180,6 @@ class AuthenticationSignonTest extends PMATestCase
        $GLOBALS['cfg']['Server']['user'] = 'user';
        $GLOBALS['cfg']['Server']['SignonScript'] = '';
        $_COOKIE['session123'] = true;
-        $_REQUEST['old_usr'] = 'oldUser';
        $_SESSION['PMA_single_signon_user'] = 'user123';
        $_SESSION['PMA_single_signon_password'] = 'pass123';
        $_SESSION['PMA_single_signon_host'] = 'local';
@ -172,28 +189,20 @@ class AuthenticationSignonTest extends PMATestCase
        $sessionName = session_name();
        $sessionID = session_id();

-        $this->assertFalse(
-            $this->object->authCheck()
-        );
+        $this->object->logOut();

        $this->assertEquals(
            array(
                'SignonURL' => 'http://phpmyadmin.net/SignonURL',
                'SignonScript' => '',
                'SignonSession' => 'session123',
-                'host' => 'local',
-                'port' => '12',
+                'host' => 'localhost',
+                'port' => '80',
                'user' => 'user',
-                'foo' => 'bar'
            ),
            $GLOBALS['cfg']['Server']
        );

-        $this->assertEquals(
-            'pmaToken',
-            $_SESSION[' PMA_token ']
-        );
-
        $this->assertEquals(
            $sessionName,
            session_name()
@ -207,6 +216,7 @@ class AuthenticationSignonTest extends PMATestCase
        $this->assertFalse(
            isset($_SESSION['LAST_SIGNON_URL'])
        );
+        $attrInstance->setValue($restoreInstance);
    }

    /**