[REF] Use random_bytes instead of uniqid/rand for random hex strings

Using random_bytes is both faster and more secure than md5(uniqid(rand(), TRUE)). It is possibly also easier to read, in the sense that it is more obvious that it returns hexadecimal encoded random bytes.

I did not find an instance where guessing the random identifier would result in a security vulnerability. So this change does not have direct security impact as far as I know. It's more of a best practice thing and I hope people copy paste the new, secure way of generating random bytes when creating identifiers for security-sensitive stuff, instead of copying the old, insecure way.

In some test files the lengths of the random strings are one character longer. E.g. I replaced `substr(sha1(rand()), 0, 7)` with `bin2hex(random_bytes(4))`. The length did not seem very important here, so I don't think this matters.

I haven't tested all changed code. I rely on unit tests, and that the code generates a random hex string of a certain length before and after I replaced it.

I also looked into the SQL statements that use MD5(RAND()). These should be replaced by HEX(RANDOM_BYTES()), but this is only available starting in MariaDB 10.10, and we require 10.2.

CRM/Contact/BAO/Contact.php

@@ -191,7 +191,7 @@ public static function add(&$params) {
     // Fixed in 1.5 by making hash optional, only do this in create mode, not update.
     if ((!isset($contact->hash) || !$contact->hash) && !$contact->id) {
       $allNull = FALSE;
-      $contact->hash = md5(uniqid(rand(), TRUE));
+      $contact->hash = bin2hex(random_bytes(16));
     }
 
     // Even if we don't need $employerId, it's important to call getFieldValue() before

CRM/Contact/BAO/Contact/Utils.php

@@ -165,7 +165,7 @@ public static function generateChecksum($entityId, $ts = NULL, $live = NULL, $ha
       // to avoid breaking things elsewhere
       // See lab issue #5541
       do {
-        $hash = md5(uniqid(rand(), TRUE));
+        $hash = bin2hex(random_bytes(16));
         if ($hashSize) {
           $hash = substr($hash, 0, $hashSize);
         }

CRM/Contact/Form/Task.php

@@ -447,7 +447,7 @@ public function createHiddenGroup() {
     if ($searchParams['radio_ts'] == 'ts_sel') {
       // Create a static group.
       // groups require a unique name
-      $randID = md5(time() . rand(1, 1000));
+      $randID = bin2hex(random_bytes(16));
       $grpTitle = "Hidden Group {$randID}";
       $grpID = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Group', $grpTitle, 'id', 'title');
 

CRM/Contribute/Form/AbstractEditPayment.php

@@ -272,7 +272,7 @@ public function __get($name) {
    */
   public function getInvoiceID(): string {
     if (!$this->invoiceID) {
-      $this->invoiceID = md5(uniqid(mt_rand(), TRUE));
+      $this->invoiceID = bin2hex(random_bytes(16));
     }
     return $this->invoiceID;
   }

CRM/Contribute/Form/Contribution.php

@@ -1234,7 +1234,7 @@ protected function processCreditCard($submittedValues, $lineItem, $contactID) {
     CRM_Contribute_Form_AdditionalInfo::postProcessCommon($params, $this->_params, $this);
 
     if (empty($this->_params['invoice_id'])) {
-      $this->_params['invoiceID'] = md5(uniqid(rand(), TRUE));
+      $this->_params['invoiceID'] = bin2hex(random_bytes(16));
     }
     else {
       $this->_params['invoiceID'] = $this->_params['invoice_id'];

CRM/Contribute/Form/Contribution/Confirm.php

@@ -1669,7 +1669,7 @@ private function processSecondaryFinancialTransaction($contactID, $tempParams, $
     $financialType->id = $financialTypeID;
     $financialType->find(TRUE);
     $tempParams['amount'] = $minimumFee;
-    $tempParams['invoiceID'] = md5(uniqid(rand(), TRUE));
+    $tempParams['invoiceID'] = bin2hex(random_bytes(16));
     $isRecur = $tempParams['is_recur'] ?? NULL;
 
     //assign receive date when separate membership payment
@@ -1850,7 +1850,7 @@ public static function submit($params) {
     //this way the mocked up controller ignores the session stuff
     $_SERVER['REQUEST_METHOD'] = 'GET';
     $form->controller = new CRM_Contribute_Controller_Contribution();
-    $params['invoiceID'] = md5(uniqid(rand(), TRUE));
+    $params['invoiceID'] = bin2hex(random_bytes(16));
 
     $paramsProcessedForForm = $form->_params = self::getFormParams($params['id'], $params);
 

CRM/Contribute/Form/Contribution/Main.php

@@ -1252,7 +1252,7 @@ public function submit($params) {
     $this->set('amount', $this->getMainContributionAmount());
 
     // generate and set an invoiceID for this transaction
-    $invoiceID = md5(uniqid(rand(), TRUE));
+    $invoiceID = bin2hex(random_bytes(16));
     $this->set('invoiceID', $invoiceID);
     $params['invoiceID'] = $invoiceID;
     $title = !empty($this->_values['frontend_title']) ? $this->_values['frontend_title'] : $this->_values['title'];

CRM/Core/Error.php

@@ -509,7 +509,7 @@ public static function debug($name, $variable = NULL, $log = TRUE, $html = TRUE,
 
     if ($log) {
       // Log the output to error_log with a unique reference.
-      $unique = substr(md5(random_bytes(32)), 0, 12);
+      $unique = bin2hex(random_bytes(6));
       error_log("errorID:$unique\n$out");
 
       if (!$checkPermission) {

CRM/Core/Payment/AuthorizeNetIPN.php

@@ -161,7 +161,7 @@ public function getInput(): array {
     // Per CRM-17611 it would also not be passed back for a decline.
     elseif ($this->isSuccess()) {
       $input['is_test'] = 1;
-      $input['trxn_id'] = $this->transactionID ?: md5(uniqid(mt_rand(), TRUE));
+      $input['trxn_id'] = $this->transactionID ?: bin2hex(random_bytes(16));
     }
     $this->transactionID = $input['trxn_id'];
 

CRM/Core/Payment/PayPalProIPN.php

@@ -261,7 +261,7 @@ public function recur(array $input): void {
           // In future moving to create pending & then complete, but this OK for now.
           // Also consider accepting 'Failed' like other processors.
           $input['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
-          $input['invoice_id'] = md5(uniqid(rand(), TRUE));
+          $input['invoice_id'] = bin2hex(random_bytes(16));
           $input['original_contribution_id'] = $this->getContributionID();
           $input['contribution_recur_id'] = $this->getContributionRecurID();
 

CRM/Event/Form/Participant.php

@@ -2044,7 +2044,7 @@ public function getLineItems(): array {
    */
   public function getInvoiceID(): string {
     if (!$this->invoiceID) {
-      $this->invoiceID = md5(uniqid(rand(), TRUE));
+      $this->invoiceID = bin2hex(random_bytes(16));
     }
     return $this->invoiceID;
   }

CRM/Event/Form/Registration/Register.php

@@ -833,7 +833,7 @@ public function postProcess() {
       $this->set('amount_level', $params['amount_level']);
 
       // generate and set an invoiceID for this transaction
-      $invoiceID = md5(uniqid(rand(), TRUE));
+      $invoiceID = bin2hex(random_bytes(16));
       $this->set('invoiceID', $invoiceID);
 
       if ($this->_paymentProcessor) {

CRM/Import/Form/Preview.php

@@ -52,7 +52,7 @@ public function buildQuickForm() {
   public function setStatusUrl() {
     $statusID = $this->get('statusID');
     if (!$statusID) {
-      $statusID = md5(uniqid(rand(), TRUE));
+      $statusID = bin2hex(random_bytes(16));
       $this->set('statusID', $statusID);
     }
     $statusUrl = CRM_Utils_System::url('civicrm/ajax/status', "id={$statusID}", FALSE, NULL, FALSE);

CRM/Utils/Chart.php

@@ -270,7 +270,7 @@ public static function buildChart(&$params, $chart) {
         }
 
         // generate unique id for this chart instance
-        $uniqueId = md5(uniqid(rand(), TRUE));
+        $uniqueId = bin2hex(random_bytes(16));
 
         $theChart["chart_{$uniqueId}"]['size'] = ['xSize' => $xSize, 'ySize' => $ySize];
         $theChart["chart_{$uniqueId}"]['object'] = $chartObj;

CRM/Utils/File.php

@@ -458,7 +458,7 @@ public static function cleanFileName($name) {
    * @return string
    */
   public static function makeFileName($name, bool $unicode = FALSE) {
-    $uniqID = md5(uniqid(rand(), TRUE));
+    $uniqID = bin2hex(random_bytes(16));
     $info = pathinfo($name);
     $basename = substr($info['basename'],
       0, -(strlen($info['extension'] ?? '') + (($info['extension'] ?? '') == '' ? 0 : 1))
@@ -509,7 +509,7 @@ public static function makeFilenameWithUnicode(string $input, string $replacemen
    */
   public static function duplicate($filePath) {
     $oldName = pathinfo($filePath, PATHINFO_FILENAME);
-    $uniqID = md5(uniqid(rand(), TRUE));
+    $uniqID = bin2hex(random_bytes(16));
     $newName = preg_replace('/(_[\w]{32})$/', '', $oldName) . '_' . $uniqID;
     $newPath = str_replace($oldName, $newName, $filePath);
     copy($filePath, $newPath);
@@ -730,9 +730,6 @@ public static function relativize($directory, $basePath) {
    * @see tempnam
    */
   public static function tempnam($prefix = 'tmp-') {
-    // $config = CRM_Core_Config::singleton();
-    // $nonce = md5(uniqid() . $config->dsn . $config->userFrameworkResourceURL);
-    // $fileName = "{$config->configAndLogDir}" . $prefix . $nonce . $suffix;
     $fileName = tempnam(sys_get_temp_dir(), $prefix);
     return $fileName;
   }

CRM/Utils/SQL/TempTable.php

@@ -92,7 +92,7 @@ class CRM_Utils_SQL_TempTable {
   public static function build() {
     $t = new CRM_Utils_SQL_TempTable();
     $t->category = NULL;
-    $t->id = md5(uniqid('', TRUE));
+    $t->id = bin2hex(random_bytes(16));
     // The constant CIVICRM_TEMP_FORCE_DURABLE is for local debugging.
     $t->durable = CRM_Utils_Constant::value('CIVICRM_TEMP_FORCE_DURABLE', FALSE);
     $t->utf8 = TRUE;

Civi/Test/ContactTestTrait.php

@@ -154,7 +154,7 @@ private function sampleContact(string $contact_type, int $seq = 0, bool $random
     foreach ($samples[$contact_type] as $key => $values) {
       $params[$key] = $values[$seq % count($values)];
       if ($random) {
-        $params[$key] .= substr(sha1(mt_rand()), 0, 5);
+        $params[$key] .= bin2hex(random_bytes(3));
       }
     }
     if ($contact_type === 'Individual') {

api/v3/Contribution/Transact.php

@@ -55,7 +55,7 @@ function civicrm_api3_contribution_transact($params) {
   }
 
   // Some payment processors expect a unique invoice_id - generate one if not supplied
-  $params['invoice_id'] ??= md5(uniqid(rand(), TRUE));
+  $params['invoice_id'] ??= bin2hex(random_bytes(16));
 
   $paymentProcessor = CRM_Financial_BAO_PaymentProcessor::getPayment($params['payment_processor'], $params['payment_processor_mode']);
   $params = $paymentProcessor['object']->doPayment($params);

bin/ContributionProcessor.php

@@ -436,7 +436,7 @@ public static function processAPIContribution($params) {
       // errors due to invoice ID. See:
       // ./CRM/Core/Payment/PayPalIPN.php:200
       if ($recurring->id) {
-        $params['invoice_id'] = md5(uniqid(rand(), TRUE));
+        $params['invoice_id'] = bin2hex(random_bytes(16));
       }
 
       $recurring->copyValues($params);
@@ -496,7 +496,7 @@ public static function _fillCommonParams(&$params, $type = 'paypal') {
     }
     else {
       // generate a new transaction id, if not already exist
-      $transaction['trxn_id'] = md5(uniqid(rand(), TRUE));
+      $transaction['trxn_id'] = bin2hex(random_bytes(16));
     }
 
     if (!isset($transaction['financial_type_id'])) {

ext/afform/mock/tests/phpunit/E2E/AfformMock/MockPublicFormTest.php

@@ -50,7 +50,7 @@ public function testGetPage() {
   public function testPublicCreateAllowed() {
     $initialMaxId = CRM_Core_DAO::singleValueQuery('SELECT max(id) FROM civicrm_contact');
 
-    $r = md5(random_bytes(16));
+    $r = bin2hex(random_bytes(16));
 
     $me = [0 => ['fields' => []]];
     $me[0]['fields']['first_name'] = 'Firsty' . $r;
@@ -75,7 +75,7 @@ public function testPublicEditDisallowed() {
       ->execute()
       ->first();
 
-    $r = md5(random_bytes(16));
+    $r = bin2hex(random_bytes(16));
 
     $me = [0 => ['fields' => []]];
     $me[0]['fields']['id'] = $contact['id'];

ext/authx/Civi/Authx/AuthxRequestBuilder.php

@@ -156,7 +156,7 @@ protected function credPass($cid) {
   }
 
   public function credApikey($cid) {
-    $api_key = md5(\random_bytes(16));
+    $api_key = bin2hex(\random_bytes(16));
     \civicrm_api3('Contact', 'create', [
       'id' => $cid,
       'api_key' => $api_key,

ext/eventcart/CRM/Event/Cart/Form/Checkout/Payment.php

@@ -474,7 +474,7 @@ public function postProcess() {
     }
 
     $params['now'] = date('YmdHis');
-    $params['invoiceID'] = md5(uniqid(rand(), TRUE));
+    $params['invoiceID'] = bin2hex(random_bytes(16));
     $params['amount'] = $this->total;
     $params['financial_type_id'] = $this->financial_type_id;
     if ($this->payment_required && empty($params['is_pay_later'])) {

ext/ewaysingle/CRM/Core/Payment/eWAY.php

@@ -214,7 +214,7 @@ public function doPayment(&$params, $component = 'contribute') {
     //----------------------------------------------------------------------------------------------------
     // We use CiviCRM's param's 'invoiceID' as the unique transaction token to feed to eWAY
     // Trouble is that eWAY only accepts 16 chars for the token, while CiviCRM's invoiceID is an 32.
-    // As its made from a "$invoiceID = md5(uniqid(rand(), true));" then using the fierst 16 chars
+    // As its made from a "$invoiceID = bin2hex(random_bytes(16))" then using the first 16 chars
     // should be alright
     //----------------------------------------------------------------------------------------------------
     $uniqueTrnxNum = substr($params['invoiceID'], 0, 16);

tests/phpunit/CRM/Contact/BAO/ContactType/ContactSearchTest.php

@@ -83,7 +83,7 @@ class CRM_Contact_BAO_ContactType_ContactSearchTest extends CiviUnitTestCase {
 
   public function setUp(): void {
     parent::setUp();
-    $students = 'indivi_student' . substr(sha1(rand()), 0, 7);
+    $students = 'indivi_student' . bin2hex(random_bytes(4));
     $params = [
       'label' => $students,
       'name' => $students,
@@ -94,7 +94,7 @@ public function setUp(): void {
     CRM_Contact_BAO_ContactType::writeRecord($params);
     $this->student = $params['name'];
 
-    $parents = 'indivi_parent' . substr(sha1(rand()), 0, 7);
+    $parents = 'indivi_parent' . bin2hex(random_bytes(4));
     $params = [
       'label' => $parents,
       'name' => $parents,
@@ -105,7 +105,7 @@ public function setUp(): void {
     CRM_Contact_BAO_ContactType::writeRecord($params);
     $this->parent = $params['name'];
 
-    $organizations = 'org_sponsor' . substr(sha1(rand()), 0, 7);
+    $organizations = 'org_sponsor' . bin2hex(random_bytes(4));
     $params = [
       'label' => $organizations,
       'name' => $organizations,

tests/phpunit/CRM/Contact/Form/Search/SearchContactTest.php

@@ -49,7 +49,7 @@ public function testContactSubtype(): void {
   protected function searchContacts($contactSubType) {
     // create contact
     $params = [
-      'first_name' => 'Peter' . substr(sha1(rand()), 0, 4),
+      'first_name' => 'Peter' . bin2hex(random_bytes(2)),
       'last_name' => 'Lastname',
       'contact_type' => 'Individual',
       'contact_sub_type' => $contactSubType,

tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php

@@ -790,7 +790,7 @@ public function setUp(): void {
       'id' => $customField['id'],
       'token' => sprintf('{contact.custom_%s}', $customField['id']),
       'name' => sprintf('custom_%s', $customField['id']),
-      'value' => 'text ' . substr(sha1(mt_rand()), 0, 7),
+      'value' => 'text ' . bin2hex(random_bytes(4)),
     ];
 
     $this->fixtures['sched_on_custom_date'] = [

tests/phpunit/CRM/Core/BAO/NavigationTest.php

@@ -93,7 +93,7 @@ public function testUpdateExistingReportMenuLink(): void {
    * Test that a navigation item can be retrieved by it's url.
    */
   public function testGetNavItemByUrl(): void {
-    $random_string = substr(sha1(rand()), 0, 7);
+    $random_string = bin2hex(random_bytes(4));
     $name = "Test Menu Link {$random_string}";
     $url = "civicrm/test/{$random_string}";
     $url_params = "reset=1";
@@ -120,7 +120,7 @@ public function testGetNavItemByUrl(): void {
    * that is part of the navigation but not the instance.
    */
   public function testGetNavItemByUrlWildcard(): void {
-    $random_string = substr(sha1(rand()), 0, 7);
+    $random_string = bin2hex(random_bytes(4));
     $name = "Test Menu Link {$random_string}";
     $url = "civicrm/test/{$random_string}";
     $url_params = "reset=1&output=criteria";

tests/phpunit/CRM/Core/DAOTest.php

@@ -379,8 +379,8 @@ public function testDBOptions(): void {
     $contactIDs = [];
     for ($i = 0; $i < 10; $i++) {
       $contactIDs[] = $this->individualCreate([
-        'first_name' => 'Alan' . substr(sha1(rand()), 0, 7),
-        'last_name' => 'Smith' . substr(sha1(rand()), 0, 4),
+        'first_name' => 'Alan' . bin2hex(random_bytes(4)),
+        'last_name' => 'Smith' . bin2hex(random_bytes(2)),
       ]);
     }
 

tests/phpunit/CRM/Mailing/BAO/MailingTest.php

@@ -174,7 +174,7 @@ public function testGetMailingDisabledGroup(): void {
     $this->hookClass->setHook('civicrm_aclGroup', [$this, 'hook_civicrm_aclGroup']);
     CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM', 'edit groups'];
     // Create dummy group and assign 2 contacts
-    $name = 'Test static group ' . substr(sha1(rand()), 0, 7);
+    $name = 'Test static group ' . bin2hex(random_bytes(4));
     $groupID = $this->groupCreate([
       'name' => $name,
       'title' => $name,

tests/phpunit/CRM/Mailing/BAO/SpoolTest.php

@@ -29,17 +29,17 @@ public function tearDown(): void {
    */
   public function testSend(): void {
     $contact_params_1 = [
-      'first_name' => substr(sha1(rand()), 0, 7),
+      'first_name' => bin2hex(random_bytes(4)),
       'last_name' => 'Anderson',
-      'email' => substr(sha1(rand()), 0, 7) . '@example.org',
+      'email' => bin2hex(random_bytes(4)) . '@example.org',
       'contact_type' => 'Individual',
     ];
     $contact_id_1 = $this->individualCreate($contact_params_1);
 
     $contact_params_2 = [
-      'first_name' => substr(sha1(rand()), 0, 7),
+      'first_name' => bin2hex(random_bytes(4)),
       'last_name' => 'Xylophone',
-      'email' => substr(sha1(rand()), 0, 7) . '@example.org',
+      'email' => bin2hex(random_bytes(4)) . '@example.org',
       'contact_type' => 'Individual',
     ];
     $contact_id_2 = $this->individualCreate($contact_params_2);

tests/phpunit/CRM/Price/Form/FieldTest.php

@@ -76,7 +76,7 @@ public function testLargeFloatOptionValue($thousandSeparator) {
     $this->setCurrencySeparators($thousandSeparator);
     $thousands = Civi::settings()->get('monetaryThousandSeparator');
     $decimal = Civi::settings()->get('monetaryDecimalPoint');
-    $paramsSet['title'] = 'Price Set' . substr(sha1(rand()), 0, 7);
+    $paramsSet['title'] = 'Price Set' . bin2hex(random_bytes(4));
     $paramsSet['name'] = CRM_Utils_String::titleToVar($paramsSet['title']);
     $paramsSet['is_active'] = TRUE;
     $paramsSet['financial_type_id'] = 'Event Fee';

tests/phpunit/api/v3/ActionScheduleTest.php

@@ -53,7 +53,7 @@ public function testSimpleActionScheduleCreate($version) {
     $oldCount = CRM_Core_DAO::singleValueQuery('select count(*) from civicrm_action_schedule');
     $activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate');
     $assigneeID = CRM_Utils_Array::key('Activity Assignees', $activityContacts);
-    $title = 'simpleActionSchedule' . substr(sha1(rand()), 0, 7);
+    $title = 'simpleActionSchedule' . bin2hex(random_bytes(4));
     $params = [
       'title' => $title,
       'recipient' => $assigneeID,
@@ -101,7 +101,7 @@ public function testActionScheduleWithScheduledDatesCreate($version) {
     $oldCount = CRM_Core_DAO::singleValueQuery('select count(*) from civicrm_action_schedule');
     $activityContacts = CRM_Activity_BAO_ActivityContact::buildOptions('record_type_id', 'validate');
     $assigneeID = CRM_Utils_Array::key('Activity Assignees', $activityContacts);
-    $title = 'simpleActionSchedule' . substr(sha1(rand()), 0, 7);
+    $title = 'simpleActionSchedule' . bin2hex(random_bytes(4));
     $params = [
       'title' => $title,
       'recipient' => $assigneeID,

tests/phpunit/api/v3/ActivityTest.php

@@ -754,7 +754,7 @@ public function testActivityGetTagFilter(): void {
   public function testJoinOnTags(): void {
     $tagName = 'act_tag_nm_' . mt_rand();
     $tagDescription = 'act_tag_ds_' . mt_rand();
-    $tagColor = '#' . substr(md5(mt_rand()), 0, 6);
+    $tagColor = '#' . bin2hex(random_bytes(3));
     $tag = $this->callAPISuccess('Tag', 'create', ['name' => $tagName, 'color' => $tagColor, 'description' => $tagDescription, 'used_for' => 'Activities']);
     $activity = $this->callAPISuccess('Activity', 'Create', $this->_params);
     $this->callAPISuccess('EntityTag', 'create', ['entity_table' => 'civicrm_activity', 'tag_id' => $tag['id'], 'entity_id' => $activity['id']]);