Fixed issue #19139: [security] CSRF in Reset Survey menus (#3598)

Co-authored-by: Lapiu Dev <devgit@lapiu.biz>
LimeSurvey · Nov 15, 2023 · 487e073 · 487e073
1 parent b659187
commit 487e073
Show file tree

Hide file tree

Showing 4 changed files with 247 additions and 2 deletions.
diff --git a/application/config/internal.php b/application/config/internal.php
@@ -149,7 +149,7 @@
             'enableCookieValidation' => false, // Enable to activate cookie protection
             'noCsrfValidationParams' => array(),
             'noCsrfValidationRoutes' => array(
-                'remotecontrol',
+                'admin/remotecontrol',
                 'plugins/unsecure',
             ),
             'csrfCookie' => array(

diff --git a/application/core/LSHttpRequest.php b/application/core/LSHttpRequest.php
@@ -163,7 +163,7 @@ protected function normalizeRequest()
             // $validationParams['request'] = 'acs';
 
             foreach ($validationRoutes as $cr) {
-                if (preg_match('#' . $cr . '#', $route)) {
+                if (self::routeMatchesNoCsrfValidationRule($route, $cr)) {
                     Yii::app()->detachEventHandler(
                         'onBeginRequest',
                         array($this, 'validateCsrfToken')
@@ -250,4 +250,20 @@ public function setQueryParams($values)
     {
         $this->queryParams = $values;
     }
+
+    /**
+     * Returns true if the route matches the given validation rule.
+     * @param string $route the route to be checked
+     * @param string $rule the validation rule
+     * @return bool true if the route matches the given validation rule
+     */
+    public static function routeMatchesNoCsrfValidationRule($route, $rule)
+    {
+        // The rule should either match the whole route, or the start of the route followed by a slash.
+        // For example the routes "rest" (in the case of "index.php/rest?...") or "rest/..." (in the case of
+        // "index.php/rest/...") should be matched by the rule "rest", but the route "admin/menus/sa/restore"
+        // should not.
+        $route = ltrim($route, '/');
+        return preg_match('#^' . $rule . '$|^' . $rule . '/#', (string) $route);
+    }
 }
diff --git a/tests/functional/backend/RemoteControlTest.php b/tests/functional/backend/RemoteControlTest.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace ls\tests;
+
+use Yii;
+
+class RemoteControlTest extends TestBaseClassWeb
+{
+    private static $tmpBaseUrl;
+    private static $tmpRPCType;
+
+    private static $client;
+
+    public static function setUpBeforeClass(): void
+    {
+        parent::setUpBeforeClass();
+
+        $urlMan = Yii::app()->urlManager;
+        self::$tmpBaseUrl = $urlMan->getBaseUrl();
+        $urlMan->setBaseUrl('http://' . self::$domain . '/index.php');
+        //$serverUrl = App()->createAbsoluteUrl('/admin/remotecontrol');
+        $serverUrl = $urlMan->createUrl('/admin/remotecontrol');
+
+        self::$tmpRPCType = Yii::app()->getConfig('RPCInterface');
+
+        if (self::$tmpRPCType === 'off') {
+            \SettingGlobal::setSetting('RPCInterface', 'json');
+            $RPCType = 'json';
+        } else {
+            $RPCType = self::$tmpRPCType;
+        }
+
+        if ($RPCType == 'xml') {
+            $cur_path = get_include_path();
+            set_include_path($cur_path . PATH_SEPARATOR . APPPATH . 'helpers');
+            require_once('Zend/XmlRpc/Client.php');
+
+            self::$client = new \Zend_XmlRpc_Client($serverUrl);
+        } elseif ($RPCType == 'json') {
+            Yii::app()->loadLibrary('jsonRPCClient');
+            self::$client = new \jsonRPCClient($serverUrl);
+        }
+    }
+
+    public static function tearDownAfterClass(): void
+    {
+        parent::tearDownAfterClass();
+
+        $urlMan = Yii::app()->urlManager;
+        $urlMan->setBaseUrl(self::$tmpBaseUrl);
+
+        \SettingGlobal::setSetting('RPCInterface', self::$tmpRPCType);
+    }
+
+    public function testGetSessionKey()
+    {
+        $sessionKey = self::$client->call('get_session_key', ['admin', 'password']);
+        $this->assertIsNotArray($sessionKey);
+
+        self::$client->call('release_session_key', [$sessionKey]);
+    }
+}
diff --git a/tests/unit/CsrfHttpRequestTest.php b/tests/unit/CsrfHttpRequestTest.php
@@ -0,0 +1,167 @@
+<?php
+
+namespace ls\tests;
+
+class CsrfHttpRequestTest extends TestBaseClass
+{
+    public static $noCsrfValidationRoutes;
+
+    public static function setUpBeforeClass(): void
+    {
+        self::$noCsrfValidationRoutes = \Yii::app()->request->noCsrfValidationRoutes;
+    }
+
+    /**
+     * Testing that rest routes skip CSRF validation.
+     */
+    public function testRestRoutesSkipCsrfValidation()
+    {
+        // This test is skipped since there are no rest routes on v5, hence not expected to skip CSRF validation.
+        $this->markTestSkipped();
+        $routes = array(
+            'rest/v1/actionOnItemById/15',
+            'rest/v1/action',
+            '/rest/v1/actionOnItemById/25',
+            '/rest/v1/action',
+        );
+
+        foreach ($routes as $route) {
+            $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[0]
+            );
+
+            $this->assertSame(
+                1,
+                $skipValidation,
+                'CSRF validation should be skipped since the route ' . $route . ' is a rest route.'
+            );
+        }
+    }
+
+    /**
+     * Testing that remotecontrol routes skip CSRF validation.
+     */
+    public function testRemoteControlRoutesSkipCsrfValidation()
+    {
+        $routes = array(
+            'admin/remotecontrol/actionOnItemById/15',
+            'admin/remotecontrol/action',
+            '/admin/remotecontrol/actionOnItemById/25',
+            '/admin/remotecontrol',
+        );
+
+        foreach ($routes as $route) {
+            $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[0]
+            );
+
+            $this->assertSame(
+                1,
+                $skipValidation,
+                'CSRF validation should be skipped since the route ' . $route . ' is a remote control route.'
+            );
+        }
+    }
+
+    /**
+     * Testing that plugins/unsecure routes skip CSRF validation.
+     */
+    public function testPluginsUnsecureRoutesSkipCsrfValidation()
+    {
+        $routes = array(
+            'plugins/unsecure/action',
+            'plugins/unsecure/actionOnItemById/15',
+            '/plugins/unsecure/action',
+            '/plugins/unsecure/actionOnItemById/25',
+        );
+
+        foreach ($routes as $route) {
+            $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[1]
+            );
+
+            $this->assertSame(
+                1,
+                $skipValidation,
+                'CSRF validation should be skipped since the route ' . $route . ' is a plugins/unsecure route.'
+            );
+        }
+    }
+
+    /**
+     * Testing that rest-like routes don't skip CSRF validation.
+     */
+    public function testRestLikeRoutesDoNotSkipCsrfValidation()
+    {
+        // This test is skipped since there are no rest routes on v5, hence not expected to skip CSRF validation.
+        $this->markTestSkipped();
+        $routes = array(
+            'admin/menus/sa/restore',
+            '/admin/menus/sa/restore'
+        );
+
+        foreach ($routes as $route) {
+            $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[0]
+            );
+
+            $this->assertSame(
+                0,
+                $routeValidation,
+                'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.'
+            );
+        }
+    }
+
+    /**
+     * Testing that remote-like routes don't skip CSRF validation.
+     */
+    public function testRemoteLikeRoutesDoNotSkipCsrfValidation()
+    {
+        $routes = array(
+            'remote/action',
+            '/remote/action'
+        );
+
+        foreach ($routes as $route) {
+            $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[0]
+            );
+
+            $this->assertSame(
+                0,
+                $routeValidation,
+                'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.'
+            );
+        }
+    }
+
+    /**
+     * Testing that plugins/unsecure-like routes don't skip CSRF validation.
+     */
+    public function testPluginUnsecureLikeRoutesDoNotSkipCsrfValidation()
+    {
+        $routes = array(
+            'plugins/settings',
+            '/plugins/settings'
+        );
+
+        foreach ($routes as $route) {
+            $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
+                $route,
+                self::$noCsrfValidationRoutes[1]
+            );
+
+            $this->assertSame(
+                0,
+                $routeValidation,
+                'CSRF validation should not be skipped since the route ' . $route . ' is not a remote control route.'
+            );
+        }
+    }
+}