X-Git-Url: https://vcs.fsf.org/?a=blobdiff_plain;f=CRM%2FCore%2FKey.php;h=3e7826f2c7172ae2691330defc261b5606da096a;hb=43fd2d50323d1162d47578162c25d3dc070ceb79;hp=3e4ee1235b1cff4be2c0ee1aeb6d0adf21e73fa9;hpb=5f3863929569f1515668185c490c7b34757b432c;p=civicrm-core.git

diff --git a/CRM/Core/Key.php b/CRM/Core/Key.php
index 3e4ee1235b..3e7826f2c7 100644
--- a/CRM/Core/Key.php
+++ b/CRM/Core/Key.php
@@ -15,6 +15,28 @@
  * @copyright CiviCRM LLC https://civicrm.org/licensing
  */
 class CRM_Core_Key {
+
+  /**
+   * The length of the randomly-generated, per-session signing key.
+   *
+   * Expressed as number of bytes. (Ex: 128 bits = 16 bytes)
+   *
+   * @var int
+   */
+  const PRIVATE_KEY_LENGTH = 16;
+
+  /**
+   * @var string
+   * @see hash_hmac_algos()
+   */
+  const HASH_ALGO = 'sha256';
+
+  /**
+   * The minimum length of a generated signature/digest (expressed in base36 digits).
+   * @var int
+   */
+  const HASH_LENGTH = 25;
+
   public static $_key = NULL;
 
   public static $_sessionID = NULL;
@@ -30,7 +52,7 @@ class CRM_Core_Key {
       $session = CRM_Core_Session::singleton();
       self::$_key = $session->get('qfPrivateKey');
       if (!self::$_key) {
-        self::$_key = md5(uniqid(mt_rand(), TRUE)) . md5(uniqid(mt_rand(), TRUE));
+        self::$_key = base64_encode(random_bytes(self::PRIVATE_KEY_LENGTH));
         $session->set('qfPrivateKey', self::$_key);
       }
     }
@@ -64,12 +86,10 @@ 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
+      // now generate a random number between 1 and 10000 and add it to the key
       // so that we can have forms in mutiple tabs etc
       $key = $key . '_' . mt_rand(1, 10000);
     }
@@ -101,28 +121,46 @@ class CRM_Core_Key {
       $k = $key;
     }
 
-    $privateKey = self::privateKey();
-    $sessionID = self::sessionID();
-    if ($k != md5($sessionID . $name . $privateKey)) {
+    $expected = self::sign($name);
+    if (!hash_equals($k, $expected)) {
       return NULL;
     }
     return $key;
   }
 
   /**
-   * The original version of this function, added circa 2010 and untouched
-   * since then, seemed intended to check for a 32-digit hex string followed
-   * optionally by an underscore and 4-digit number. But it had a bug where
-   * the optional part was never checked ever. So have decided to remove that
-   * second check to keep it simple since it seems like pseudo-security.
+   * Check that the key is well-formed. This does not check that the key is
+   * currently a key that is in use or belongs to a real form/session.
    *
    * @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 key is an alphanumeric string of at least HASH_LENGTH with
+    // an optional underscore+digits at the end.
+    return preg_match('#^[0-9a-zA-Z]{' . self::HASH_LENGTH . ',}+(_\d+)?$#', $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.");
+    }
+    // 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 $prefix . base_convert(hash_hmac(self::HASH_ALGO, $sessionID . $delim . $name, $privateKey), 16, 36);
+
   }
 
 }