From 298c7870e6fe4332d8aa1757a9c8d79f841389ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Skrzypczak?= Date: Tue, 18 Jan 2022 12:25:46 +0100 Subject: [PATCH] Improved CSRF protection --- app/Controller/Action.php | 4 +--- app/Controller/Base.php | 25 ++++--------------------- app/Request.php | 4 ++-- config/ConfigTemplates.php | 6 ++++++ config/Security.php | 3 +++ config/csrf_config.php | 6 ++++-- config/version.php | 4 ++-- include/main/WebUI.php | 4 +--- install/views/Index.php | 3 --- modules/Users/views/Login.php | 3 --- 10 files changed, 23 insertions(+), 39 deletions(-) diff --git a/app/Controller/Action.php b/app/Controller/Action.php index 88a23f299ac3..c9278fe0a5cd 100644 --- a/app/Controller/Action.php +++ b/app/Controller/Action.php @@ -7,6 +7,7 @@ * @copyright YetiForce Sp. z o.o * @license YetiForce Public License 4.0 (licenses/LicenseEN.txt or yetiforce.com) * @author Mariusz Krzaczkowski + * @author Radosław Skrzypczak */ namespace App\Controller; @@ -16,9 +17,6 @@ */ abstract class Action extends Base { - /** {@inheritdoc} */ - public $csrfActive = false; - /** * Process action. * diff --git a/app/Controller/Base.php b/app/Controller/Base.php index 0cadef3d7a4d..b8f99f2f510c 100644 --- a/app/Controller/Base.php +++ b/app/Controller/Base.php @@ -7,6 +7,7 @@ * @copyright YetiForce Sp. z o.o * @license YetiForce Public License 4.0 (licenses/LicenseEN.txt or yetiforce.com) * @author Mariusz Krzaczkowski + * @author Radosław Skrzypczak */ namespace App\Controller; @@ -18,12 +19,6 @@ abstract class Base { /** @var \App\Headers Headers instance. */ public $headers; - /** - * CSRF is active?. - * - * @var bool - */ - public $csrfActive = true; /** * Activated language locale. @@ -31,12 +26,6 @@ abstract class Base * @var bool */ protected static $activatedLocale = false; - /** - * Activated csrf. - * - * @var bool - */ - protected static $activatedCsrf = false; /** * Constructor. @@ -48,15 +37,9 @@ public function __construct() \App\Language::initLocale(); self::$activatedLocale = true; } - if (!self::$activatedCsrf) { - if ($this->csrfActive && \App\Config::security('csrfActive')) { - require_once 'config/csrf_config.php'; - \CsrfMagic\Csrf::init(); - $this->csrfActive = true; - } else { - $this->csrfActive = false; - } - self::$activatedCsrf = true; + if (\App\Config::security('csrfActive')) { + require_once 'config/csrf_config.php'; + \CsrfMagic\Csrf::init(); } } diff --git a/app/Request.php b/app/Request.php index 706c2daf6e38..65f19f14b83d 100644 --- a/app/Request.php +++ b/app/Request.php @@ -710,8 +710,8 @@ public function validateWriteAccess($skipRequestTypeCheck = false) throw new \App\Exceptions\Csrf('Invalid request - validate Write Access'); } $this->validateReadAccess(); - if (class_exists('CSRFConfig') && !\CsrfMagic\Csrf::check(false)) { - throw new \App\Exceptions\Csrf('Unsupported request'); + if (\App\Config::security('csrfActive')) { + \CsrfMagic\Csrf::check(); } } diff --git a/config/ConfigTemplates.php b/config/ConfigTemplates.php index c72c6f67cc03..9ccec497caa2 100644 --- a/config/ConfigTemplates.php +++ b/config/ConfigTemplates.php @@ -1154,6 +1154,12 @@ 'validation' => '\App\Validator::bool', 'sanitization' => '\App\Purifier::bool' ], + 'csrfLifetimeToken' => [ + 'default' => 28800, + 'description' => 'Default expire time of CSRF token in seconds', + 'validation' => '\App\Validator::naturalNumber', + 'sanitization' => '\App\Purifier::naturalNumber' + ], 'csrfFrameBreaker' => [ 'default' => true, 'description' => 'Enable verified frame protection, used in CSRF', diff --git a/config/Security.php b/config/Security.php index 94ea1484519f..8ef8586946e8 100644 --- a/config/Security.php +++ b/config/Security.php @@ -151,6 +151,9 @@ class Security /** Enable CSRF protection */ public static $csrfActive = true; + /** Default expire time of CSRF token in seconds */ + public static $csrfLifetimeToken = 28800; + /** Enable verified frame protection, used in CSRF */ public static $csrfFrameBreaker = true; diff --git a/config/csrf_config.php b/config/csrf_config.php index d36be8412054..d6a1bc03059c 100644 --- a/config/csrf_config.php +++ b/config/csrf_config.php @@ -6,6 +6,7 @@ * The Initial Developer of the Original Code is vtiger. * Portions created by vtiger are Copyright (C) vtiger. * All Rights Reserved. + * Contributor(s): YetiForce Sp. z o.o * ****************************************************************************** */ class CSRFConfig @@ -16,14 +17,15 @@ class CSRFConfig public static function startup() { //Override the default expire time of token - \CsrfMagic\Csrf::$expires = 259200; + \CsrfMagic\Csrf::$expires = \App\Config::security('csrfLifetimeToken', 7200); \CsrfMagic\Csrf::$callback = function ($tokens) { - throw new \App\Exceptions\AppException('Invalid request - Response For Illegal Access', 403); + throw new \App\Exceptions\Csrf('Invalid request - Response For Illegal Access', 403); }; $js = 'vendor/yetiforce/csrf-magic/src/Csrf.min.js'; if (!IS_PUBLIC_DIR) { $js = 'public_html/' . $js; } + \CsrfMagic\Csrf::$defer = true; \CsrfMagic\Csrf::$dirSecret = __DIR__; \CsrfMagic\Csrf::$rewriteJs = $js; \CsrfMagic\Csrf::$cspToken = \App\Session::get('CSP_TOKEN'); diff --git a/config/version.php b/config/version.php index f8377d50abf7..ad7aa939e895 100644 --- a/config/version.php +++ b/config/version.php @@ -1,7 +1,7 @@ '6.3.42', - 'patchVersion' => '2022.01.17', + 'appVersion' => '6.3.43', + 'patchVersion' => '2022.01.18', 'lib_roundcube' => '0.2.10', ]; diff --git a/include/main/WebUI.php b/include/main/WebUI.php index f0405baca82f..e02d33cd59b3 100644 --- a/include/main/WebUI.php +++ b/include/main/WebUI.php @@ -168,9 +168,7 @@ public function process(App\Request $request) \App\Log::error("HandlerClass: $handlerClass", 'Loader'); throw new \App\Exceptions\AppException('LBL_HANDLER_NOT_FOUND', 405); } - if ($handler->csrfActive) { - $handler->validateRequest($request); - } + $handler->validateRequest($request); if ($handler->loginRequired() && $this->checkLogin($request)) { return true; } diff --git a/install/views/Index.php b/install/views/Index.php index bdc1249f2c7c..228c60eb64e2 100644 --- a/install/views/Index.php +++ b/install/views/Index.php @@ -14,9 +14,6 @@ class Install_Index_View extends \App\Controller\View\Base { use \App\Controller\ExposeMethod; - /** {@inheritdoc} */ - public $csrfActive = false; - /** * @var bool */ diff --git a/modules/Users/views/Login.php b/modules/Users/views/Login.php index 82335d61683e..670a39ef318a 100644 --- a/modules/Users/views/Login.php +++ b/modules/Users/views/Login.php @@ -11,9 +11,6 @@ class Users_Login_View extends \App\Controller\View\Base { - /** {@inheritdoc} */ - public $csrfActive = false; - /** {@inheritdoc} */ public function __construct() {