Per Drupal secure coding standards at

md5 and sha1 should not be used any place in Drupal core since 7.0, but are re-introduced in the Views module for 8.x:

298:      $this->resultsKey = $this->view->storage->get('name') . ':' . $this->displayHandler->display['id'] . ':results:' . md5(serialize($key_data));
322:      $this->outputKey = $this->view->storage->get('name') . ':' . $this->displayHandler->display['id'] . ':output:' . md5(serialize($key_data));

133:      $cid = 'unpackOptions:' . md5(serialize(array($this->options, $options)));

559:                $grouping = md5(serialize($grouping));

1416:    $this->dom_id = !empty($this->dom_id) ? $this->dom_id : md5($this->storage->get('name') . REQUEST_TIME . rand());
#1 1884828-1.patch3.64 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 49,607 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


pwolanin’s picture

3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 49,607 pass(es). View

Also, rand() is slow and unreliable - mt_rand() should be used instead per the PHP docs.

pwolanin’s picture

Status: Active » Needs review
pwolanin’s picture

Assigned: Unassigned » pwolanin
tim.plunkett’s picture

Issue tags: +VDC


dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, and I don't think performance matters here as it's not executed really often.

+1 for mt_rand()

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I always found these kinds of changes to just protect Drupal from brain-dead security scanners that even flag "md5" in *comments* and *text files* as security issues kinda silly, but OTOH I can attest that the sec. team gets bogus bug reports about this all the time, so it's nice to eliminate these. Thanks!

Committed and pushed to 8.x. Thanks!

dawehner’s picture

Anyone know whether this might be worth to backport to D7?

pwolanin’s picture

Yes, I would consider this for backport - ideally all 7.x modules should have stopped using md5.

dawehner’s picture

Project: Drupal core » Views
Version: 8.x-dev » 7.x-3.x-dev
Component: views.module » Code
Status: Fixed » Patch (to be ported)
merlinofchaos’s picture

In at least some cases in D7, the 32 byte length of the hash is the reason we're using it, not any security reason; sha256 keys are longer, I believe, so can't be used in that situation. If md5 is really to be considered evil (and boy, I dislike that) we would need to come up with another solution.

pwolanin’s picture

@merlinofchaos in the block patch at #1884826: Regression - replace md5 in Block module calls with sha2 hashes I did:

substr(hash('sha256', $delta), 0, 32);

though happily that code was't needed at all.

Probably better (more bits) is:

$hash = substr(drupal_hash_base64($delta), 0, 32);

Which we use in facetapi for block deltas:

merlinofchaos’s picture

An update should also clear caches and other things where we're using these MD5s for later lookups to avoid confusion.

Also changing those will unexpectedly change people's block deltas. While it seems highly unlikely that anyone is using a silly md5 for their CSS on their view blocks...and yet, I've seen people do weirder things. So we could potentially hurt those sites.

dawehner’s picture

That is a good point!

Another point is that the blocks are currently stored in the block table with it's md5-deltas, so if you would use sha2 the block positions for example
would get lost without an update function, but as blocks are even stored in features you simply don't have a way to fix all of them.

greggles’s picture

Title: Replace md5 calls with sha2 in Views in core » Replace md5 calls with sha256 in Views in core
Issue summary: View changes

I found this recently when searching and just wanted to update at least the title to say sha256 since that is what I believe is the goal and not just any of the sha2 functions (see this explainer about the differences).