From c9a7484e9d58ed6317f25c2412be72a54d9312bc Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Mon, 6 Apr 2020 01:07:12 -0700
Subject: [PATCH] CRM_Core_Key - Strengthen signature algorithm

This alters the qfKey signature algorithm, with a few aims:

1.  If someone wants to perform a brute-force to figure the per-session
    private-key, we want it go slow.  Therefore, use a slower hash (ie
    HMAC-SHA256 instead of MD5).

2.  If someone performs a timing attack aimed at figuring a passable qfKey,
    the execution-time for `validate()` should not provide any hints.

3.  If someone finds a way to manipulate one of the constituent parts
    ($sessionID, $name, $privateKey), we want it to be hard to create a
    collsion. So... (a) Use HMAC instead of a vanilla hash. (b) Use delimiters
    between the data sections ($sessionID, $name).
---
 CRM/Core/Key.php | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/CRM/Core/Key.php b/CRM/Core/Key.php
index ebc9cf6bc4..688c0f8cff 100644
--- a/CRM/Core/Key.php
+++ b/CRM/Core/Key.php
@@ -25,6 +25,18 @@ class CRM_Core_Key {
    */
   const PRIVATE_KEY_LENGTH = 16;
 
+  /**
+   * @var string
+   * @see hash_hmac_algos()
+   */
+  const HASH_ALGO = 'sha256';
+
+  /**
+   * The length of a generated signature/digest (expressed in hex digits).
+   * @var int
+   */
+  const HASH_LENGTH = 64;
+
   public static $_key = NULL;
 
   public static $_sessionID = NULL;
@@ -74,9 +86,7 @@ class CRM_Core_Key {
    *   valid formID
    */
   public static function get($name, $addSequence = FALSE) {
-    $privateKey = self::privateKey();
-    $sessionID = self::sessionID();
-    $key = md5($sessionID . $name . $privateKey);
+    $key = self::sign($name);
 
     if ($addSequence) {
       // now generate a random number between 1 and 100K and add it to the key
@@ -111,9 +121,7 @@ class CRM_Core_Key {
       $k = $key;
     }
 
-    $privateKey = self::privateKey();
-    $sessionID = self::sessionID();
-    if ($k != md5($sessionID . $name . $privateKey)) {
+    if (!hash_equals($k, self::sign($name))) {
       return NULL;
     }
     return $key;
@@ -129,10 +137,29 @@ class CRM_Core_Key {
    * @param string $key
    *
    * @return bool
+   *   TRUE if the signature ($key) is well-formed.
    */
   public static function valid($key) {
-    // ensure that key contains a 32 digit hex string
-    return (bool) preg_match('#[0-9a-f]{32}#i', $key);
+    // ensure that hash is a hex number (of expected length)
+    return preg_match('#[0-9a-f]{' . self::HASH_LENGTH . '}#i', $key) ? TRUE : FALSE;
+  }
+
+  /**
+   * @param string $name
+   *   The name of the form
+   * @return string
+   *   A signed digest of $name, computed with the per-session private key
+   */
+  private static function sign($name) {
+    $privateKey = self::privateKey();
+    $sessionID = self::sessionID();
+    $delim = chr(0);
+    if (strpos($sessionID, $delim) !== FALSE || strpos($name, $delim) !== FALSE) {
+      throw new \RuntimeException("Failed to generate signature. Malformed session-id or form-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);
+
   }
 
 }
-- 
2.25.1