From be7cae44087188edb3c9b77dfb2a1d8f7dbe8ed2 Mon Sep 17 00:00:00 2001 From: Duncan Sommerville Date: Fri, 20 Feb 2015 13:12:49 -0500 Subject: [PATCH 1/2] Fixed CSRF prevention checks for REST calls, moved CSRF initialization to Bootstrap --- airtime_mvc/application/Bootstrap.php | 17 ++++++++++++++++- .../controllers/LoginController.php | 3 +++ .../controllers/PluploadController.php | 5 +++-- .../controllers/plugins/Acl_plugin.php | 19 +++++++++++-------- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/airtime_mvc/application/Bootstrap.php b/airtime_mvc/application/Bootstrap.php index b4a4fb43a..787bc5d03 100644 --- a/airtime_mvc/application/Bootstrap.php +++ b/airtime_mvc/application/Bootstrap.php @@ -65,13 +65,28 @@ class Bootstrap extends Zend_Application_Bootstrap_Bootstrap } $view->headScript()->appendScript("var userType = '$userType';"); } + + /** + * Create a global namespace to hold a session token for CSRF prevention + */ + protected function _initCsrfNamespace() { + $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); + // Check if the token exists + if (!$csrf_namespace->authtoken) { + // If we don't have a token, regenerate it and set a 2 hour timeout + // Should we log the user out here if the token is expired? + $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); + $csrf_namespace->setExpirationSeconds(2*60*60); + } + } /** * Ideally, globals should be written to a single js file once * from a php init function. This will save us from having to * reinitialize them every request */ - private function _initTranslationGlobals($view) { + protected function _initTranslationGlobals() { + $view = $this->getResource('view'); $view->headScript()->appendScript("var PRODUCT_NAME = '" . PRODUCT_NAME . "';"); $view->headScript()->appendScript("var USER_MANUAL_URL = '" . USER_MANUAL_URL . "';"); $view->headScript()->appendScript("var COMPANY_NAME = '" . COMPANY_NAME . "';"); diff --git a/airtime_mvc/application/controllers/LoginController.php b/airtime_mvc/application/controllers/LoginController.php index c808c2aee..7566adb6f 100644 --- a/airtime_mvc/application/controllers/LoginController.php +++ b/airtime_mvc/application/controllers/LoginController.php @@ -98,6 +98,9 @@ class LoginController extends Zend_Controller_Action { $auth = Zend_Auth::getInstance(); $auth->clearIdentity(); + // Unset all session variables relating to CSRF prevention on logout + $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); + $csrf_namespace->unsetAll(); $this->_redirect('showbuilder/index'); } diff --git a/airtime_mvc/application/controllers/PluploadController.php b/airtime_mvc/application/controllers/PluploadController.php index f1197ec2b..6aae736ae 100644 --- a/airtime_mvc/application/controllers/PluploadController.php +++ b/airtime_mvc/application/controllers/PluploadController.php @@ -32,8 +32,9 @@ class PluploadController extends Zend_Controller_Action } $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); - $csrf_namespace->setExpirationSeconds(5*60*60); - $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); + /* Moved to be globally set in Bootstrap */ + // $csrf_namespace->setExpirationSeconds(5*60*60); + // $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); $csrf_element = new Zend_Form_Element_Hidden('csrf'); $csrf_element->setValue($csrf_namespace->authtoken)->setRequired('true')->removeDecorator('HtmlTag')->removeDecorator('Label'); diff --git a/airtime_mvc/application/controllers/plugins/Acl_plugin.php b/airtime_mvc/application/controllers/plugins/Acl_plugin.php index bd803ec89..fbf783131 100644 --- a/airtime_mvc/application/controllers/plugins/Acl_plugin.php +++ b/airtime_mvc/application/controllers/plugins/Acl_plugin.php @@ -148,17 +148,22 @@ class Zend_Controller_Plugin_Acl extends Zend_Controller_Plugin_Abstract } } } else { //We have a session/identity. - // If we have an identity and we're making a RESTful request, // we need to check the CSRF token - if ($request->_action != "get" && $request->getModuleName() == "rest") { - $tokenValid = $this->verifyCSRFToken($request->getParam("csrf_token")); + if ($_SERVER['REQUEST_METHOD'] != "GET" && $request->getModuleName() == "rest") { + $token = $request->getParam("csrf_token"); + $tokenValid = $this->verifyCSRFToken($token); if (!$tokenValid) { + $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); + $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); + + Logging::warn("Invalid CSRF token: $token"); $this->getResponse() ->setHttpResponseCode(401) - ->appendBody("ERROR: CSRF token mismatch."); - return; + ->appendBody("ERROR: CSRF token mismatch.") + ->sendResponse(); + die(); } } @@ -202,9 +207,7 @@ class Zend_Controller_Plugin_Acl extends Zend_Controller_Plugin_Abstract $current_namespace = new Zend_Session_Namespace('csrf_namespace'); $observed_csrf_token = $token; $expected_csrf_token = $current_namespace->authtoken; - Logging::error("Observed: " . $observed_csrf_token); - Logging::error("Expected: " . $expected_csrf_token); - + return ($observed_csrf_token == $expected_csrf_token); } From 73e5fb938f7a5afa74eabe57072d1d5c5765e32f Mon Sep 17 00:00:00 2001 From: Albert Santoni Date: Tue, 24 Feb 2015 11:13:39 -0500 Subject: [PATCH 2/2] Use more secure random number generation for CSRF auth tokens * Also cleaned up pull request --- airtime_mvc/application/controllers/PluploadController.php | 6 +++--- airtime_mvc/application/controllers/plugins/Acl_plugin.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/airtime_mvc/application/controllers/PluploadController.php b/airtime_mvc/application/controllers/PluploadController.php index 6aae736ae..7c808140c 100644 --- a/airtime_mvc/application/controllers/PluploadController.php +++ b/airtime_mvc/application/controllers/PluploadController.php @@ -31,10 +31,10 @@ class PluploadController extends Zend_Controller_Action $this->view->quotaLimitReached = true; } + //Because uploads are done via AJAX (and we're not using Zend form for those), we manually add the CSRF + //token in here. $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); - /* Moved to be globally set in Bootstrap */ - // $csrf_namespace->setExpirationSeconds(5*60*60); - // $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); + //The CSRF token is generated in Bootstrap.php $csrf_element = new Zend_Form_Element_Hidden('csrf'); $csrf_element->setValue($csrf_namespace->authtoken)->setRequired('true')->removeDecorator('HtmlTag')->removeDecorator('Label'); diff --git a/airtime_mvc/application/controllers/plugins/Acl_plugin.php b/airtime_mvc/application/controllers/plugins/Acl_plugin.php index fbf783131..d4577e471 100644 --- a/airtime_mvc/application/controllers/plugins/Acl_plugin.php +++ b/airtime_mvc/application/controllers/plugins/Acl_plugin.php @@ -156,7 +156,7 @@ class Zend_Controller_Plugin_Acl extends Zend_Controller_Plugin_Abstract if (!$tokenValid) { $csrf_namespace = new Zend_Session_Namespace('csrf_namespace'); - $csrf_namespace->authtoken = sha1(uniqid(rand(),1)); + $csrf_namespace->authtoken = sha1(openssl_random_pseudo_bytes(128)); Logging::warn("Invalid CSRF token: $token"); $this->getResponse()