From 40b1007aabcfb23db0368f5c7471c5bd8559556b Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 8 Dec 2020 15:15:45 -0800
Subject: [PATCH] CRM_Core_Key - Provide more debugging hints about `qfKey`s

Overview
--------

The `qfKey` parameter is a security mechanism (CSRF). The content of `qfKey` is also inscrutable - so when there's a problem
with `qfKey`, it can be difficult to determine the origin/nature of the problem.

Before
------

The `qfKey` provides a digital signature based on (1) session ID, (2) the
form being processed, (3) a random private-key (unique to the user/session).
In some cases, the `qfKey` also has a random nonce appended to distinguish
between concurrent tabs that work with the same form.

* Ex: `2abc4b7f23d9ae4dfcdbb0c692cc666e5f11256fe84ace7662b0e075834b81958` (w/o nonce)
* Ex: `2abc4b7f23d9ae4dfcdbb0c692cc666e5f11256fe84ace7662b0e075834b81958_9527` (w/nonce)

If there is a logic problem where the `qfKey` of form A gets mixed-up with
form B, then it's extremely hard to understand the mismatch All you see are
two long random codes.

* Inputted qfKey: `2abc4b7f23d9ae4dfcdbb0c692cc666e5f11256fe84ace7662b0e075834b81958`
* Expected fqKey: `89874b7a23192e4d6aab10c622ca369e5e11226f784a5c7652b95075536b81a5e`

After
-----

The `qfKey` has a prefix to indicate the token's intended usage (as well as a digital signature).

* Ex: `CRMContactControllerSearch154eitbf74v3ko0sw94k08gogo8448asdfw4goggkkwgkww08c` (w/o nonce)
* Ex: `CRMContactControllerSearch154eitbf74v3ko0sw94k08gogo8448asdfw4goggkkwgkww08c_9355` (w/ nonce)

If there is a logic problem where the `qfKey` of form A gets mixed-up with
form B, then the prefix can help you understand. Compare:

* Inputted qfKey: `CRMContactControllerSearch154eitbf74v3ko0sw94k08gogo8448asdfw4goggkkwgkww08c`
* Expected qfKey: `CRMContributeFormContribution5o2due205mp0384080sgc8omw8kwggoksw47sswchs80gw0kgs`

This tells you that there's logical mismatch - it's trying to render the contribution
screen using the key for the contact-search screen.

Comments
--------

* The identity of the `formname` is not sensitive information.
* The `qfKey` might look prettier with more delimeters (`{formname}_{signature}_{nonce}`). However, I belive there are random bits of existing code
  which use `explode('_')` to split apart the signature and the nonce. This formula seems to be drop-in/interoperable.
* I switched the encoding of the signature from hex (0-9a-f) to base-36 (0-9a-z) to make it a bit shorter.
---
 CRM/Core/Key.php | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/CRM/Core/Key.php b/CRM/Core/Key.php
index 688c0f8cff..505bd3588d 100644
--- a/CRM/Core/Key.php
+++ b/CRM/Core/Key.php
@@ -32,10 +32,10 @@ class CRM_Core_Key {
   const HASH_ALGO = 'sha256';
 
   /**
-   * The length of a generated signature/digest (expressed in hex digits).
+   * The minimum length of a generated signature/digest (expressed in base36 digits).
    * @var int
    */
-  const HASH_LENGTH = 64;
+  const HASH_LENGTH = 25;
 
   public static $_key = NULL;
 
@@ -121,7 +121,8 @@ class CRM_Core_Key {
       $k = $key;
     }
 
-    if (!hash_equals($k, self::sign($name))) {
+    $expected = self::sign($name);
+    if (!hash_equals($k, $expected)) {
       return NULL;
     }
     return $key;
@@ -141,7 +142,7 @@ class CRM_Core_Key {
    */
   public static function valid($key) {
     // ensure that hash is a hex number (of expected length)
-    return preg_match('#[0-9a-f]{' . self::HASH_LENGTH . '}#i', $key) ? TRUE : FALSE;
+    return preg_match('#^[0-9a-zA-Z]{' . self::HASH_LENGTH . ',}+(_\d+)?$#', $key) ? TRUE : FALSE;
   }
 
   /**
@@ -157,8 +158,10 @@ class CRM_Core_Key {
     if (strpos($sessionID, $delim) !== FALSE || strpos($name, $delim) !== FALSE) {
       throw new \RuntimeException("Failed to generate signature. Malformed session-id or form-name.");
     }
+    // The "prefix" gives some advisory details to help with debugging.
+    $prefix = preg_replace('/[^a-zA-Z0-9]/', '', $name);
     // Note: Unsure why $sessionID is included, but it's always been there, and it doesn't seem harmful.
-    return hash_hmac(self::HASH_ALGO, $sessionID . $delim . $name, $privateKey);
+    return $prefix . base_convert(hash_hmac(self::HASH_ALGO, $sessionID . $delim . $name, $privateKey), 16, 36);
 
   }
 
-- 
2.25.1