Problem/Motivation

Discovered in #2606772: Long Twig cache directories can cause failures on some filesystems and we should be ensuring that filenames are not unnecessarily long. That issue proposed truncating the hmac_hash() however that results in less security. Given that this is about allowing the server to write PHP we shouldn't be taking any unnecessary risks.

Proposed resolution

Use \Drupal\Component\Utility\Crypt::hmacBase64() instead of hash_hmac(). As this will result in shorter hashes...

>>> hash_hmac('sha256', 'a', 'a');
=> "3ecf5388e220da9e0f919485deb676d8bee3aec046a779353b463418511ee622"
>>> \Drupal\Component\Utility\Crypt::hmacBase64('a', 'a');
=> "Ps9TiOIg2p4PkZSF3rZ22L7jrsBGp3k1O0Y0GFEe5iI"

This will cause all twig files to be regenerated.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

pwolanin’s picture

rolling a patch for this.

I think this will also cause the service container to be rebuilt?

alexpott’s picture

The service container is no longer compiled to disk :)

pwolanin’s picture

Status: Active » Needs review
FileSize
2.38 KB

simple conversion - also makes the tempnam function use secure random bytes instead of the one-off of shuffling the hash of microtime().

Guess my local install has some left-over cruft of compiled containers - let me rm that to reduce my own confusion.

alexpott’s picture

+++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
@@ -225,7 +227,7 @@ protected function getUncachedMTime($directory) {
-      $path = $directory . '/' . $prefix . substr(str_shuffle(hash('sha256', microtime())), 0, 10);
+      $path = $directory . '/' . $prefix . Crypt::randomBytesBase64(20);

Crypt::randomBytesBase64(10); no?

alexpott’s picture

Or actually... \Drupal\Component\Utility\Crypt::randomBytesBase64(7)

pwolanin’s picture

@alexpott - why make it so short? Then it's more likely to collide. If we are going to call this real random function it might as well be to get at least 16 bytes.

alexpott’s picture

@pwolanin I guess it's immaterial :) since the length will be shorter than the eventual filename

joelpittet’s picture

Component: theme system » file system

Changing component, it probably should be this or base system from a quick issue queue search.

alexpott’s picture

Status: Needs review » Needs work

This change needs an empty post-update to clear caches since all twig files in php/twig are going to change their names. Especially if this gets in before #2606772: Long Twig cache directories can cause failures on some filesystems

pwolanin’s picture

Why do we need that? If Drupal doesn't find the expected file (cache miss) it generates it.

alexpott’s picture

Because leaving around a whole load of files that aren't used is super confusing for people.

pwolanin’s picture

Status: Needs work » Needs review

ok, well I'm going to make #2606772: Long Twig cache directories can cause failures on some filesystems postponed on this - this is a trivial patch and can go in 1st, and then the other can have the post-update. I don't think we need it 2x for essentially the same thing.

alexpott’s picture

Nope but the fist patch in should have the post update in case the other does not make it into 8.3.0 or for that matter the next 8.2.x release since I think these bug fixes are eligible.

pwolanin’s picture

ok, so put the same post-update in both patches?

pwolanin’s picture

Adding the same post-update function as #2606772: Long Twig cache directories can cause failures on some filesystems so we can be sure it's present if either go in.

YesCT’s picture

I'm reviewing this now.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
493 bytes
2.95 KB

grep'd and capital PHP is what we do.

without patch

$ ag "hash_hmac" core
core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php
133:    return $directory . '/' . hash_hmac('sha256', $name, $this->secret . $directory_mtime) . '.php';

core/lib/Drupal/Component/Utility/Crypt.php
92:    $hmac = base64_encode(hash_hmac('sha256', $data, $key, TRUE));

core/lib/Drupal/Core/Site/Settings.php
174:      return 'drupal.' . $identifier . '.' . \Drupal::VERSION . '.' . static::get('deployment_identifier') . '.' . hash_hmac('sha256', $identifier, static::get('hash_salt') . '.' . $root . '/' . $site_path);

core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php
80:    $expected_filename = $expected_directory . '/' . hash_hmac('sha256', $name, $this->secret . $directory_mtime) . '.php';

with patch

$ ag "hash_hmac" core
core/lib/Drupal/Component/Utility/Crypt.php
92: $hmac = base64_encode(hash_hmac('sha256', $data, $key, TRUE));

core/lib/Drupal/Core/Site/Settings.php
174: return 'drupal.' . $identifier . '.' . \Drupal::VERSION . '.' . static::get('deployment_identifier') . '.' . hash_hmac('sha256', $identifier, static::get('hash_salt') . '.' . $root . '/' . $site_path);

there are a bunch of hash('sha256' in core, but only one in MTime without the patch

$ ag "hash..sha256" core | grep MT
core/lib/Drupal/Component/PhpStorage/MTimeProtectedFastFileStorage.php:228:      $path = $directory . '/' . $prefix . substr(str_shuffle(hash('sha256', microtime())), 0, 10);

none with the patch

$ ag "hash..sha256" core | grep MT

So, considering the scope of this issue, I think this is fine.

rtbc cause the new patch is just with a nit. check the interdiff to make sure..

(doesn't apply to 8.2.x (system.post_update.php) you want that in this issue?)

  • catch committed a251c2d on 8.3.x
    Issue #2830596 by pwolanin, YesCT, alexpott:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

@alexpott mentioned this going into 8.2.x too. It's eligible since it's a straight bugfix, but not sure how I feel about it.

pwolanin’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Fixed » Patch (to be ported)

Does not hurt anything, and would start mitigating the windows issue?

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.9 KB

Just add the post update in the right place.

YesCT’s picture

I'll give this a check.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

read the whole patch, just to make sure no unintended things got in in the git diff. looks great.

also redid the greps, and same as 8.3.x

rtbc from me.

@catch what is your concern about putting this in 8.2.x?

YesCT’s picture

Assigned: Unassigned » catch

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2830596-8.2.x-22.patch, failed testing.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

sporadic fail?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2830596-8.2.x-22.patch, failed testing.

catch’s picture

I don't have a specific concerns about getting this into 8.2, but it's the sort of change that if there is an issue it'd be mostly likely found after upgrading a real site.

  • catch committed a251c2d on 8.4.x
    Issue #2830596 by pwolanin, YesCT, alexpott:...

  • catch committed a251c2d on 8.4.x
    Issue #2830596 by pwolanin, YesCT, alexpott:...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: -rc target, -Triaged core major

Issue tags got cloned on this incorrectly.

pwolanin’s picture

Status: Needs work » Fixed

Since this is not going to 8.2, it's fixed, right?

cilefen’s picture

Yes I think so.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.