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 <willemijn@irdc.nl>

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');

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');
+        return $this->validateSessionToken('csrf', $token);
-        if (isset($tokens[$token])) {
-            unset($tokens[$token]);
-            session_set('csrf', $tokens);
-            return true;
-        }
-
-        return false;
     }
 
+    /**
+     * 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');
+        return $this->validateSessionToken('pcsrf', $token);
-        if (isset($tokens[$token])) {
-            return true;
-        }
-
-        return false;
     }
 
-    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)) {
+        $nonce = self::getToken(self::$NONCE_LENGTH);
-            session_set($key, []);
+        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();
+        if (strlen($token) != (self::$NONCE_LENGTH + self::$HMAC_LENGTH) * 2) {
-        session_merge($key, [$nonce => true]);
+            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));
     }
 }

app/Helper/SubtaskHelper.php

@@ -80,12 +80,20 @@ class SubtaskHelper extends Base
     public function renderTimer(array $task, array $subtask)
     {
         $html = '<span class="subtask-timer-toggle">';
+        $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 .= '</span>';

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);
+        $csrf = $token->getCSRFToken();
-        $this->assertTrue($token->validateCSRFToken($t1));
+        $this->assertTrue($token->validateCSRFToken($csrf));
-        $this->assertFalse($token->validateCSRFToken($t1));
+
+        $pcsrf = $token->getReusableCSRFToken();
+        $this->assertTrue($token->validateReusableCSRFToken($pcsrf));
     }
 }