From 98faef1b064acb61b714843451e4dd4407598e87 Mon Sep 17 00:00:00 2001 From: Pepijn Over Date: Sun, 31 Jan 2016 00:20:52 +0100 Subject: [PATCH] fixes #237: adding CSRF token to all forms and requires now on POST --- CHANGELOG.rst | 2 +- composer.json | 4 +- composer.lock | 103 +++++++++++++++++- src/psm/Module/AbstractController.php | 24 ++++ .../Config/Controller/ConfigController.php | 1 + src/psm/Module/ControllerInterface.php | 6 + .../Install/Controller/InstallController.php | 1 + .../Server/Controller/ServerController.php | 1 + .../User/Controller/LoginController.php | 1 + .../User/Controller/ProfileController.php | 1 + .../Module/User/Controller/UserController.php | 1 + src/psm/Router.php | 97 +++++++++++++++-- src/templates/default/main/macros.tpl.html | 3 + .../default/module/config/config.tpl.html | 2 + .../module/install/config_new.tpl.html | 2 + .../module/install/config_new_user.tpl.html | 2 + .../module/server/server/update.tpl.html | 2 + .../default/module/user/login/forgot.tpl.html | 2 + .../default/module/user/login/login.tpl.html | 2 + .../default/module/user/login/reset.tpl.html | 2 + .../default/module/user/profile.tpl.html | 2 + .../default/module/user/user/update.tpl.html | 2 + 22 files changed, 247 insertions(+), 16 deletions(-) create mode 100644 src/templates/default/main/macros.tpl.html diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e0c84da9..b6580e93 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -10,7 +10,7 @@ not yet released * #169: Increased server ip char limit to 500. * #164: Added support for FreeVoipDeal SMS gateway . * #181: Added blank index files to prevent directory listing. - +* #237: Adding CSRF protection. v3.1.1 (released November 6, 2014) ---------------------------------- diff --git a/composer.json b/composer.json index 43582197..b307dc44 100755 --- a/composer.json +++ b/composer.json @@ -16,7 +16,9 @@ "symfony/event-dispatcher": "2.8.*", "symfony/http-foundation": "2.8.*", "php-pushover/php-pushover": "dev-master", - "twig/twig": "1.*" + "twig/twig": "1.*", + "paragonie/random_compat" : "1.1.6", + "indigophp/hash-compat" : "1.1.0" }, "autoload": { "files": [ diff --git a/composer.lock b/composer.lock index 657860d9..1126411a 100644 --- a/composer.lock +++ b/composer.lock @@ -4,9 +4,60 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "hash": "f30b2f3412fa5a525947648bb15618e8", - "content-hash": "3fe8378a69be2b650919ad9cff79690d", + "hash": "8f27400edd82e99aa35998a3e01fc23e", + "content-hash": "3d1c36ee7e11634bc149bfc9a250e4ae", "packages": [ + { + "name": "indigophp/hash-compat", + "version": "v1.1.0", + "source": { + "type": "git", + "url": "https://github.com/indigophp/hash-compat.git", + "reference": "43a19f42093a0cd2d11874dff9d891027fc42214" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/indigophp/hash-compat/zipball/43a19f42093a0cd2d11874dff9d891027fc42214", + "reference": "43a19f42093a0cd2d11874dff9d891027fc42214", + "shasum": "" + }, + "require": { + "php": ">=5.3" + }, + "require-dev": { + "phpunit/phpunit": "~4.4" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.2-dev" + } + }, + "autoload": { + "files": [ + "src/hash_equals.php", + "src/hash_pbkdf2.php" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Márk Sági-Kazár", + "email": "mark.sagikazar@gmail.com" + } + ], + "description": "Backports hash_* functionality to older PHP versions", + "homepage": "https://indigophp.com", + "keywords": [ + "hash", + "hash_equals", + "hash_pbkdf2" + ], + "time": "2015-08-22 07:03:35" + }, { "name": "ircmaxell/password-compat", "version": "v1.0.4", @@ -49,6 +100,54 @@ ], "time": "2014-11-20 16:49:30" }, + { + "name": "paragonie/random_compat", + "version": "1.1.6", + "source": { + "type": "git", + "url": "https://github.com/paragonie/random_compat.git", + "reference": "e6f80ab77885151908d0ec743689ca700886e8b0" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/paragonie/random_compat/zipball/e6f80ab77885151908d0ec743689ca700886e8b0", + "reference": "e6f80ab77885151908d0ec743689ca700886e8b0", + "shasum": "" + }, + "require": { + "php": ">=5.2.0" + }, + "require-dev": { + "phpunit/phpunit": "4.*|5.*" + }, + "suggest": { + "ext-libsodium": "Provides a modern crypto API that can be used to generate random bytes." + }, + "type": "library", + "autoload": { + "files": [ + "lib/random.php" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Paragon Initiative Enterprises", + "email": "security@paragonie.com", + "homepage": "https://paragonie.com" + } + ], + "description": "PHP 5.x polyfill for random_bytes() and random_int() from PHP 7", + "keywords": [ + "csprng", + "pseudorandom", + "random" + ], + "time": "2016-01-29 16:19:52" + }, { "name": "php-pushover/php-pushover", "version": "dev-master", diff --git a/src/psm/Module/AbstractController.php b/src/psm/Module/AbstractController.php index 0c3e8566..c4a44a25 100644 --- a/src/psm/Module/AbstractController.php +++ b/src/psm/Module/AbstractController.php @@ -67,6 +67,11 @@ abstract class AbstractController extends ContainerAware implements ControllerIn */ protected $add_menu = true; + /** + * @var string $csrf_key + */ + protected $csrf_key; + /** * Messages to show the user * @var array $messages @@ -470,4 +475,23 @@ abstract class AbstractController extends ContainerAware implements ControllerIn public function getUser() { return $this->container->get('user'); } + + /** + * Get custom key for CSRF validation + * @return string + */ + public function getCSRFKey() { + return $this->csrf_key; + } + + /** + * Set CSRF key for validation + * @param string $key + * @return \psm\Module\ControllerInterface + */ + protected function setCSRFKey($key) { + $this->csrf_key = $key; + $this->twig->addGlobal('csrf_key', $key); + return $this; + } } diff --git a/src/psm/Module/Config/Controller/ConfigController.php b/src/psm/Module/Config/Controller/ConfigController.php index c51e6e72..9179301b 100644 --- a/src/psm/Module/Config/Controller/ConfigController.php +++ b/src/psm/Module/Config/Controller/ConfigController.php @@ -70,6 +70,7 @@ class ConfigController extends AbstractController { parent::__construct($db, $twig); $this->setMinUserLevelRequired(PSM_USER_ADMIN); + $this->setCSRFKey('config'); $this->setActions(array( 'index', 'save', diff --git a/src/psm/Module/ControllerInterface.php b/src/psm/Module/ControllerInterface.php index 25843a1d..ba821adf 100644 --- a/src/psm/Module/ControllerInterface.php +++ b/src/psm/Module/ControllerInterface.php @@ -44,4 +44,10 @@ interface ControllerInterface extends ContainerAwareInterface { * @return int */ public function getMinUserLevelRequired(); + + /** + * Get custom key for CSRF validation + * @return string + */ + public function getCSRFKey(); } diff --git a/src/psm/Module/Install/Controller/InstallController.php b/src/psm/Module/Install/Controller/InstallController.php index 06524091..dad31b14 100644 --- a/src/psm/Module/Install/Controller/InstallController.php +++ b/src/psm/Module/Install/Controller/InstallController.php @@ -48,6 +48,7 @@ class InstallController extends AbstractController { parent::__construct($db, $twig); $this->setMinUserLevelRequired(PSM_USER_ANONYMOUS); + $this->setCSRFKey('install'); $this->addMenu(false); $this->path_config = PSM_PATH_SRC . '../config.php'; diff --git a/src/psm/Module/Server/Controller/ServerController.php b/src/psm/Module/Server/Controller/ServerController.php index a7f59d8f..cbf50343 100644 --- a/src/psm/Module/Server/Controller/ServerController.php +++ b/src/psm/Module/Server/Controller/ServerController.php @@ -44,6 +44,7 @@ class ServerController extends AbstractServerController { $this->server_id = isset($_GET['id']) ? intval($_GET['id']) : 0; + $this->setCSRFKey('server'); $this->setActions(array( 'index', 'edit', 'save', 'delete', 'view', ), 'index'); diff --git a/src/psm/Module/User/Controller/LoginController.php b/src/psm/Module/User/Controller/LoginController.php index d8cfc13e..1df8e518 100644 --- a/src/psm/Module/User/Controller/LoginController.php +++ b/src/psm/Module/User/Controller/LoginController.php @@ -36,6 +36,7 @@ class LoginController extends AbstractController { parent::__construct($db, $twig); $this->setMinUserLevelRequired(PSM_USER_ANONYMOUS); + $this->setCSRFKey('login'); $this->setActions(array( 'login', 'forgot', 'reset', diff --git a/src/psm/Module/User/Controller/ProfileController.php b/src/psm/Module/User/Controller/ProfileController.php index f189868c..f7165104 100644 --- a/src/psm/Module/User/Controller/ProfileController.php +++ b/src/psm/Module/User/Controller/ProfileController.php @@ -43,6 +43,7 @@ class ProfileController extends AbstractController { $this->setActions(array( 'index', 'save', ), 'index'); + $this->setCSRFKey('profile'); } /** diff --git a/src/psm/Module/User/Controller/UserController.php b/src/psm/Module/User/Controller/UserController.php index e4b3a4b2..e7d093f2 100644 --- a/src/psm/Module/User/Controller/UserController.php +++ b/src/psm/Module/User/Controller/UserController.php @@ -40,6 +40,7 @@ class UserController extends AbstractController { parent::__construct($db, $twig); $this->setMinUserLevelRequired(PSM_USER_ADMIN); + $this->setCSRFKey('user'); $this->setActions(array( 'index', 'edit', 'delete', 'save', diff --git a/src/psm/Router.php b/src/psm/Router.php index 8e2226be..e0983f23 100644 --- a/src/psm/Router.php +++ b/src/psm/Router.php @@ -28,6 +28,7 @@ namespace psm; use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Config\FileLocator; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Loader\XmlFileLoader; @@ -51,6 +52,7 @@ class Router { public function __construct() { $this->container = $this->buildServiceContainer(); + $this->buildTwigEnvironment(); $mods = $this->container->getParameter('modules'); @@ -72,8 +74,6 @@ class Router { * @throws \LogicException */ public function run($mod) { - $user = $this->container->get('user'); - if(strpos($mod, '_') !== false) { list($mod, $controller) = explode('_', $mod); } else { @@ -81,18 +81,21 @@ class Router { } $controller = $this->getController($mod, $controller); - - // get min required level for this controller and make sure the user matches - $min_lvl = $controller->getMinUserLevelRequired(); $action = null; - if($min_lvl < PSM_USER_ANONYMOUS) { - // if user is not logged in, load login module - if(!$user->isUserLoggedIn()) { - $controller = $this->getController('user', 'login'); - } elseif($user->getUserLevel() > $min_lvl) { - $controller = $this->getController('error'); - $action = '401'; + try { + $this->validateRequest($controller); + } catch (\InvalidArgumentException $ex) { + switch($ex->getMessage()) { + case 'login_required': + $controller = $this->getController('user', 'login'); + break; + case 'invalid_csrf_token': + case 'invalid_user_level': + default: + $controller = $this->getController('error'); + $action = '401'; + break; } } @@ -145,6 +148,48 @@ class Router { return $this->container->get($id); } + /** + * Validate requets before heading to a controller + * @param \psm\Module\ControllerInterface $controller + * @throws \InvalidArgumentException + */ + protected function validateRequest(\psm\Module\ControllerInterface $controller) { + $request = Request::createFromGlobals(); + + if($request->getMethod() == 'POST') { + // require CSRF token for all POST calls + $session = $this->container->get('user')->getSession(); + $token_in = $request->request->get('csrf', ''); + $csrf_key = $controller->getCSRFKey(); + + if(empty($csrf_key)) { + if(!hash_equals($session->get('csrf_token'), $token_in)) { + throw new \InvalidArgumentException('invalid_csrf_token'); + } + } else { + if(!hash_equals( + hash_hmac('sha256', $csrf_key, $session->get('csrf_token2')), + $token_in + )) { + throw new \InvalidArgumentException('invalid_csrf_token'); + } + } + } + + // get min required level for this controller and make sure the user matches + $min_lvl = $controller->getMinUserLevelRequired(); + + if($min_lvl < PSM_USER_ANONYMOUS) { + // if user is not logged in, load login module + if(!$this->container->get('user')->isUserLoggedIn()) { + throw new \InvalidArgumentException('login_required'); + } elseif($this->container->get('user')->getUserLevel() > $min_lvl) { + throw new \InvalidArgumentException('invalid_user_level'); + } + } + } + + /** * Build a new service container * @return \Symfony\Component\DependencyInjection\ContainerBuilder @@ -157,4 +202,32 @@ class Router { return $builder; } + + /** + * Prepare twig environment + * @return \Twig_Environment + */ + protected function buildTwigEnvironment() { + $twig = $this->container->get('twig'); + $session = $this->container->get('user')->getSession(); + if(!$session->has('csrf_token')) { + $session->set('csrf_token', bin2hex(random_bytes(32))); + } + if(!$session->has('csrf_token2')) { + $session->set('csrf_token2', random_bytes(32)); + } + + $twig->addFunction( + new \Twig_SimpleFunction( + 'csrf_token', + function($lock_to = null) use ($session) { + if(empty($lock_to)) { + return $session->get('csrf_token'); + } + return hash_hmac('sha256', $lock_to, $session->get('csrf_token2')); + } + ) + ); + return $twig; + } } \ No newline at end of file diff --git a/src/templates/default/main/macros.tpl.html b/src/templates/default/main/macros.tpl.html new file mode 100644 index 00000000..47fb8fcc --- /dev/null +++ b/src/templates/default/main/macros.tpl.html @@ -0,0 +1,3 @@ +{% macro csrf_input() %} + +{% endmacro %} \ No newline at end of file diff --git a/src/templates/default/module/config/config.tpl.html b/src/templates/default/module/config/config.tpl.html index 6ff9093d..f05eaec6 100644 --- a/src/templates/default/module/config/config.tpl.html +++ b/src/templates/default/module/config/config.tpl.html @@ -1,3 +1,4 @@ +{% import 'main/macros.tpl.html' as macro %}
  • {{ label_general }}
    • @@ -225,4 +226,5 @@ + {{ macro.csrf_input() }} \ No newline at end of file diff --git a/src/templates/default/module/install/config_new.tpl.html b/src/templates/default/module/install/config_new.tpl.html index 1d0373eb..64fe07ab 100644 --- a/src/templates/default/module/install/config_new.tpl.html +++ b/src/templates/default/module/install/config_new.tpl.html @@ -1,5 +1,6 @@ {% extends "module/install/main.tpl.html" %} {% use "module/install/results.tpl.html" %} +{% import 'main/macros.tpl.html' as macro %} {% block install %}
    @@ -8,6 +9,7 @@
    + {{ macro.csrf_input() }}

    Please enter your database info:

    diff --git a/src/templates/default/module/install/config_new_user.tpl.html b/src/templates/default/module/install/config_new_user.tpl.html index 4c75a323..baa61311 100644 --- a/src/templates/default/module/install/config_new_user.tpl.html +++ b/src/templates/default/module/install/config_new_user.tpl.html @@ -1,5 +1,6 @@ {% extends "module/install/main.tpl.html" %} {% use "module/install/results.tpl.html" %} +{% import 'main/macros.tpl.html' as macro %} {% block install %}

    Sweet, your database connection is up and running!

    @@ -9,6 +10,7 @@
    + {{ macro.csrf_input() }}
    diff --git a/src/templates/default/module/server/server/update.tpl.html b/src/templates/default/module/server/server/update.tpl.html index 61bf5fc2..dd693640 100644 --- a/src/templates/default/module/server/server/update.tpl.html +++ b/src/templates/default/module/server/server/update.tpl.html @@ -1,4 +1,6 @@ +{% import 'main/macros.tpl.html' as macro %} + {{ macro.csrf_input() }}
    {{ titlemode }}
    diff --git a/src/templates/default/module/user/login/forgot.tpl.html b/src/templates/default/module/user/login/forgot.tpl.html index 035f00ab..d9e8bba4 100644 --- a/src/templates/default/module/user/login/forgot.tpl.html +++ b/src/templates/default/module/user/login/forgot.tpl.html @@ -1,5 +1,7 @@ +{% import 'main/macros.tpl.html' as macro %}
    + {{ macro.csrf_input() }} diff --git a/src/templates/default/module/user/login/login.tpl.html b/src/templates/default/module/user/login/login.tpl.html index b12810ee..e252030e 100644 --- a/src/templates/default/module/user/login/login.tpl.html +++ b/src/templates/default/module/user/login/login.tpl.html @@ -1,5 +1,7 @@ +{% import 'main/macros.tpl.html' as macro %}
    + {{ macro.csrf_input() }} diff --git a/src/templates/default/module/user/login/reset.tpl.html b/src/templates/default/module/user/login/reset.tpl.html index 15b6a7dd..8e18c806 100644 --- a/src/templates/default/module/user/login/reset.tpl.html +++ b/src/templates/default/module/user/login/reset.tpl.html @@ -1,5 +1,7 @@ +{% import 'main/macros.tpl.html' as macro %}
    + {{ macro.csrf_input() }} diff --git a/src/templates/default/module/user/profile.tpl.html b/src/templates/default/module/user/profile.tpl.html index b8d26be2..a5f390d5 100644 --- a/src/templates/default/module/user/profile.tpl.html +++ b/src/templates/default/module/user/profile.tpl.html @@ -1,4 +1,6 @@ +{% import 'main/macros.tpl.html' as macro %} + {{ macro.csrf_input() }}
    diff --git a/src/templates/default/module/user/user/update.tpl.html b/src/templates/default/module/user/user/update.tpl.html index 93769d0c..7b4ef56d 100644 --- a/src/templates/default/module/user/user/update.tpl.html +++ b/src/templates/default/module/user/user/update.tpl.html @@ -1,4 +1,6 @@ +{% import 'main/macros.tpl.html' as macro %} + {{ macro.csrf_input() }}
    {{ titlemode }}