From 4b76bc5b32b27e3a6a810be4d535cde53c268671 Mon Sep 17 00:00:00 2001 From: irdc Date: Sun, 18 Sep 2022 02:23:41 +0200 Subject: [PATCH] Use a HMAC to sign and validate CSRF tokens, instead of generating random ones and storing them in the session data * Use a HMAC to sign and validate CSRF tokens, instead of generating random ones and storing them in the session data. Reduces number of writes to sessions table and fixes kanboard issue #4942. * Added missing CSRF check for starting/stopping subtask timers. Co-authored-by: Willemijn Coene --- app/Controller/SubtaskStatusController.php | 1 + app/Core/Security/Token.php | 90 ++++++++++++++++------ app/Helper/SubtaskHelper.php | 12 ++- tests/units/Core/Security/TokenTest.php | 9 ++- 4 files changed, 84 insertions(+), 28 deletions(-) diff --git a/app/Controller/SubtaskStatusController.php b/app/Controller/SubtaskStatusController.php index c5542b3f6..36a1b136c 100644 --- a/app/Controller/SubtaskStatusController.php +++ b/app/Controller/SubtaskStatusController.php @@ -43,6 +43,7 @@ class SubtaskStatusController extends BaseController */ public function timer() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $subtask = $this->getSubtask($task); $timer = $this->request->getStringParam('timer'); diff --git a/app/Core/Security/Token.php b/app/Core/Security/Token.php index 5efc62013..c42fb9bcf 100644 --- a/app/Core/Security/Token.php +++ b/app/Core/Security/Token.php @@ -12,6 +12,11 @@ use Kanboard\Core\Base; */ class Token extends Base { + protected static $KEY_LENGTH = 32; + protected static $NONCE_LENGTH = 16; + protected static $HMAC_ALGO = 'sha256'; + protected static $HMAC_LENGTH = 16; + /** * Generate a random token with different methods: openssl or /dev/urandom or fallback to uniqid() * @@ -19,9 +24,9 @@ class Token extends Base * @access public * @return string Random token */ - public static function getToken() + public static function getToken($length = 30) { - return bin2hex(random_bytes(30)); + return bin2hex(random_bytes($length)); } /** @@ -55,35 +60,76 @@ class Token extends Base */ public function validateCSRFToken($token) { - $tokens = session_get('csrf'); - if (isset($tokens[$token])) { - unset($tokens[$token]); - session_set('csrf', $tokens); - return true; - } - - return false; + return $this->validateSessionToken('csrf', $token); } + /** + * Check if the token exists as a reusable CSRF token + * + * @access public + * @param string $token CSRF token + * @return bool + */ public function validateReusableCSRFToken($token) { - $tokens = session_get('pcsrf'); - if (isset($tokens[$token])) { - return true; - } - - return false; + return $this->validateSessionToken('pcsrf', $token); } - protected function createSessionToken($key) + /** + * Generate a session token of the given type + * + * @access protected + * @param string $type Token type + * @return string Random token + */ + protected function createSessionToken($type) { - if (! session_exists($key)) { - session_set($key, []); + $nonce = self::getToken(self::$NONCE_LENGTH); + return $nonce . $this->signSessionToken($type, $nonce); + } + + /** + * Check a session token of the given type + * + * @access protected + * @param string $type Token type + * @param string $token Session token + * @return bool + */ + protected function validateSessionToken($type, $token) + { + if (!is_string($token)) { + return false; } - $nonce = self::getToken(); - session_merge($key, [$nonce => true]); + if (strlen($token) != (self::$NONCE_LENGTH + self::$HMAC_LENGTH) * 2) { + return false; + } - return $nonce; + $nonce = substr($token, 0, self::$NONCE_LENGTH * 2); + $hmac = substr($token, self::$NONCE_LENGTH * 2, self::$HMAC_LENGTH * 2); + + return hash_equals($this->signSessionToken($type, $nonce), $hmac); + } + + /** + * Sign a nonce with the key belonging to the given type + * + * @access protected + * @param string $type Token type + * @param string $nonce Nonce to sign + * @return string + */ + protected function signSessionToken($type, $nonce) + { + if (!session_exists($type . '_key')) { + session_set($type . '_key', self::getToken(self::$KEY_LENGTH)); + } + + $data = $nonce . '-' . session_id(); + $key = session_get($type . '_key'); + $hmac = hash_hmac(self::$HMAC_ALGO, $data, $key, true); + + return bin2hex(substr($hmac, 0, self::$HMAC_LENGTH)); } } diff --git a/app/Helper/SubtaskHelper.php b/app/Helper/SubtaskHelper.php index aa3a47655..318ab1e3a 100644 --- a/app/Helper/SubtaskHelper.php +++ b/app/Helper/SubtaskHelper.php @@ -80,12 +80,20 @@ class SubtaskHelper extends Base public function renderTimer(array $task, array $subtask) { $html = ''; + $params = array( + 'task_id' => $subtask['task_id'], + 'subtask_id' => $subtask['id'], + 'timer' => '', + 'csrf_token' => $this->token->getReusableCSRFToken(), + ); if ($subtask['is_timer_started']) { - $html .= $this->helper->url->icon('pause', t('Stop timer'), 'SubtaskStatusController', 'timer', array('timer' => 'stop', 'task_id' => $subtask['task_id'], 'subtask_id' => $subtask['id']), false, 'js-subtask-toggle-timer'); + $params['timer'] = 'stop'; + $html .= $this->helper->url->icon('pause', t('Stop timer'), 'SubtaskStatusController', 'timer', $params, false, 'js-subtask-toggle-timer'); $html .= ' (' . $this->helper->dt->age($subtask['timer_start_date']) .')'; } else { - $html .= $this->helper->url->icon('play-circle-o', t('Start timer'), 'SubtaskStatusController', 'timer', array('timer' => 'start', 'task_id' => $subtask['task_id'], 'subtask_id' => $subtask['id']), false, 'js-subtask-toggle-timer'); + $params['timer'] = 'start'; + $html .= $this->helper->url->icon('play-circle-o', t('Start timer'), 'SubtaskStatusController', 'timer', $params, false, 'js-subtask-toggle-timer'); } $html .= ''; diff --git a/tests/units/Core/Security/TokenTest.php b/tests/units/Core/Security/TokenTest.php index dbb7bd1a7..b9adea38c 100644 --- a/tests/units/Core/Security/TokenTest.php +++ b/tests/units/Core/Security/TokenTest.php @@ -20,10 +20,11 @@ class TokenTest extends Base public function testCSRFTokens() { $token = new Token($this->container); - $t1 = $token->getCSRFToken(); - $this->assertNotEmpty($t1); - $this->assertTrue($token->validateCSRFToken($t1)); - $this->assertFalse($token->validateCSRFToken($t1)); + $csrf = $token->getCSRFToken(); + $this->assertTrue($token->validateCSRFToken($csrf)); + + $pcsrf = $token->getReusableCSRFToken(); + $this->assertTrue($token->validateReusableCSRFToken($pcsrf)); } }