Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens

We should make sure, that this doesn't have any negative performance implications.

Quick benchmarking shows sha256 to be quite a bit slower:

crc32b: 6.5723259449005
sha256: 16.812311887741

<?php
function microtime_float()
{
      list($usec, $sec) = explode(" ", microtime());
      return ((float)$usec + (float)$sec);
}

$string = file_get_contents('http://www.drupal.org');
foreach (['crc32b', 'sha256'] as $method) {
  $start = microtime_float();
  for ($i = 0; $i < 100000; $i++) {
    $hash = hash($method, $string);
  }
  print $method . ': ' . (microtime_float() - $start) . "\n";
}

Note, I have no idea if this is a realistic benchmark in terms of string size, or times this would be called in a page load.

Well, someone should write a patch and then see whether it actually matters and whether it sums up enough FOR cached pages, where it actually matters.

I found 2 places this might be an issue, here's the patch.

Fixes tests.

Did some basic benchmarking using 10 nodes on the homepage (60 total nodes), logged in as user 1.

With cold caches, time spent in PlaceholderGenerator::createPlaceholder():

Current 8.0.x: 152 microseconds
With patch: 166 microseconds

So, not much change. With warmed and fully primed caches, there were 0 calls to this method.

Is there anything more to do / discuss here, or can we mark this rtbc? Seems rather important to guarantee no collisions on placeholders.

Re-roll.

So sha1 is just pretending and we should not use it anywhere.

crc32b does not pretend, and I think all of our use-cases in core really are not going to have hash-collisions, however there is a chance that people use crc32 via bad copy and paste of the pattern and crc32 is lol in PHP. Also you have to carefully review all the places it's used to see if they're collision-sensitive or not which is not ideal.

sha256 looks fine, but there is no documentation as to why we're picking that over anything else.

So I think what I'd prefer to do here is add a specific helper for non-cryptographic hashing, which we use in all these places. Then we can use sha256 for now but if we think of something else, change that centrally, and contrib can then rely on it too.

@pwolanin you prefer something like this right ?

sry the interdiff is not a patch ....

same patch #23

See also #2562341-4: Add non-HTML placeholder generation to PlaceholderGenerator or a new service.

Discussed with @alexpott, @Cottser, @lauriii, and @joelpittet at DrupalCon New Orleans. We agreed that this is a major bug since there is a small/theoretical but non-infinitesimal risk here of accidental disclosure in case of a placeholder collision.

Do placeholders end up in the cache? If so, we probably want an empty update hook to force a cache invalidation for this change?

Aside from that question, I think the current patch just needs review. I think #18 somewhat addresses @catch's concerns (and that makes sense to me).

Queued #27 for re-testing, because it is A) more than 6 months old, B) predates the addition of BigPipe to Drupal core, which I am pretty certain will also need some tests to be updated.

I've rerolled the patch from #35 to apply cleanly on 8.3.x. It doesn't pass testing, though.

Fixed the remanining bugs in core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php and core/modules/big_pipe/src/Tests/BigPipeTest.php related to the hash function.

Still getting some other test failures for the big pipe module, but it could just be something with my db setup - for example, I keep getting "Incorrect string value..." because of an em-dash in core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php (which ultimately gets inserted into simpletest.function, an ascii column).

Also getting an error "Data too long for column 'function' at row 6" - the offending value is

'Drupal\Tests\big_pipe\Unit\Render\BigPipeResponseAttachmentsProcessorTest->testHtmlResponse with data set "all official attachment types, with random assigned values, even if technically not valid, to prove BigPipeResponseAttachmentsProcessor is a perfect decorator"()'

I'm wondering whether the right approach here would be to introduce a InsecureHash method which allows people to create a hash, which is not used in the area of security, but rather just for hashing stuff, like cache or here these placeholders. In there we could then either use crypt or just use some of the other ones, but at least we never have to argue about it anymore.

Yeah, I was thinking that as well - I think that might actually be covered by #2562341: Add non-HTML placeholder generation to PlaceholderGenerator or a new service, but I'm not entirely sure

This is looking good. But I thin since the scope now appears to be beyond just PlaceholderGenerator, the issue title and summary could use a quick updating.

From #31:

Do placeholders end up in the cache? If so, we probably want an empty update hook to force a cache invalidation for this change?

I think at least some of these get cached, so an empty update hook should be added.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -683,8 +683,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
-      // https://www.drupal.org/node/2562341.
-      $placeholder = 'form_action_' . hash('crc32b', __METHOD__);
+      // https://www.drupal.org/node/2562341. The placholder uses a fixed string
+      // that is Crypt::hashBase64('Drupal\Core\Form\FormBuilder::prepareForm');
+      $placeholder = 'form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM';

This bit is still really confusing to me. I wonder if that was supposed to originally be a form class method being hashed? That would at least vary the placeholder per-form...

Aside from that, I think this looks good.

Assuming the code snippet in #51 is for better cachibility (as the code comment mentions), then I think hard-coding this is fine for now (since it is effectively hardcoded anyway given that the method doesn't change).

@dawehner - no, I do not think that is a viable approach and there is no reason to prefer other hashes. Please don't try to micro-optimize away the auditability of Drupal core.

I think you are coming from a total biased argument background :p
All I really just care about is readability, and by that better education about the problem space. Here is an example of what I thought about SecureHash::hash() vs Crypt::hashBase64()
This is for me all about the readability, nothing whatever historic fight you had in the past.

This does not improve security. It at most improves security perception, by not getting alarming results out of poor security audits.

I think we need a followup to add a coding standard to warn against the use of the hash function and to tell people to use Crypt::hashBase64() as the preferred method of generating a hash.

+++ b/core/modules/system/system.install
@@ -1728,5 +1728,12 @@ function system_update_8201() {
 /**
+ * Clear caches due to changes to render placeholders.
+ */
+function system_update_8301() {
+  // Empty update to cause a cache rebuild.
+}
+
+/**

Should be a post-update - given we just added one we don't need to add another - however we should add comments about all the stuff it is expected to cause a cache clear for. And change it's name system_post_update_clear_twig_cache to system_post_update_clear_8_3

I spent sometime pondering the feedback of @dawehner and @catch about adding a specific method for non-cryptographic hashing. Initially I agreed with them - but then I noticed that https://secure.php.net/manual/en/function.hash.php is part of PHP's cryptography extension system. I don't think we add clarity by mudding the water and making people think about cryptographic hashes and non-cryptographic hashes.

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TwigExtensionPass.php
@@ -26,7 +27,7 @@ public function process(ContainerBuilder $container) {
-    $container->setParameter('twig_extension_hash', hash('crc32b', $twig_extension_hash));
+    $container->setParameter('twig_extension_hash', Crypt::hashBase64($twig_extension_hash));

I just don't think this change is right. Over in #2606772: Long Twig cache directories can cause failures on some filesystems we've discussed filename lengths at length. This change adds nothing but pain for windows users.

Also I think #59 points to the need for a short hash function in Crypt based on hashBase64 because this is not the only place where using lengthy hashes is non-sensical.

1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
   @@ -66,6 +83,16 @@ public function __construct($root, CacheBackendInterface $cache, $twig_extension
      /**
   +   * Get the cache prefixed used by \Drupal\Core\Template\TwigPhpStorageCache
   +   *
   +   * @return string
   +   *   The file cache prefix, or empty string if the cache is disabled.
   +   */
   +  public function getTwigCachePrefix() {
   +    return $this->twigCachePrefix;
   +  }
   

   This seems unnecessary? If this is just for a test then we should use reflection. However - the prefix was publicly available before as it was stored as a container parameter and therefore available to all services. So maybe this is a good enough replacement. That said, I think we should consider the twig cache directories as internal to the twig system so I'd rather come up with a valid non-theoretical use-case first.

2. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -158,9 +159,8 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   -      // Hash the settings into a string. crc32 is the fastest way to hash
   -      // something for non-cryptographic purposes.
   -      $parts[] = hash('crc32b', serialize($settings));
   +      // Hash the settings into a string.
   +      $parts[] = Crypt::hashBase64(serialize($settings));
   

   This method is one of the most important for Drupal performance - we should definitely have a look at the impact of this change on a site with a complex layout involving lots of bundles with lots of fields.

I think I would like a change record for this, besides that this looks RTBC to me.

I was first confused about the twig changes, but the uniqid() definitely makes sense.

One question though:

What happens in a race-condition of several competing requests to write their new uniqid() to the state?

I think in the worst we just write some cache items we will never need anymore, right?

@Fabianx thanks for bringing up the race condition concern. I think you are right about there being a window. I also think you are right about the worst that will happen is some twig files will be written and used once. They would be cleaned up on a cache clear.

@pwolanin
I think more important the CR should contain the information that you should not use 'crc32b' or 'sha1' but rather Crypt::hashBase64

Thanks, pwolanin.

The reason why I think a change record is needed again (thanks for writing) is the many core contributors that did not use this best-practice (and getting that code committed).

There clearly was a lack of this mindset:

'even if this is not security related we need to use a strong hash regardless'

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -158,9 +159,9 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
-      // Hash the settings into a string. crc32 is the fastest way to hash
-      // something for non-cryptographic purposes.
-      $parts[] = hash('crc32b', serialize($settings));
+      // Include the settings serialized as JSON as part of the key. The JSON is
+      // a shorter string than the serialized form, so array access is faster.
+      $parts[] = json_encode($settings);

This method is key for typed data performance. There are performance considerations on the issue but not any numbers that I think we should see. This method will be called for every typed data object used.

Doing a simple xhprof without and with the patch shows that on standard there's very little difference. Which is great so we're not dealing with a massive change. All the differences are well with the normal variation of repeated runs.

So I think I've satisfied my performance concerns.

Committed 94f0aac and pushed to 8.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/TypedData/TypedDataManager.php b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
index e0a1b58..b1c295d 100644
--- a/core/lib/Drupal/Core/TypedData/TypedDataManager.php
+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Core\TypedData;
 
-use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Plugin\Exception\PluginException;
 use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\DependencyInjection\ClassResolverInterface;

removed unused use on commit.