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
Comment | File | Size | Author |
---|---|---|---|
#22 | 2830596-8.2.x-22.patch | 2.9 KB | pwolanin |
#18 | 2830596-18.patch | 2.95 KB | YesCT |
#18 | interdiff.2830596.16.18.txt | 493 bytes | YesCT |
#16 | increment-2830596-16.txt | 581 bytes | pwolanin |
#16 | 2830596-16.patch | 2.95 KB | pwolanin |
Comments
Comment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedrolling a patch for this.
I think this will also cause the service container to be rebuilt?
Comment #3
alexpottThe service container is no longer compiled to disk :)
Comment #4
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedsimple 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.
Comment #5
alexpottCrypt::randomBytesBase64(10);
no?Comment #6
alexpottOr actually... \Drupal\Component\Utility\Crypt::randomBytesBase64(7)
Comment #7
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commented@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.
Comment #8
alexpott@pwolanin I guess it's immaterial :) since the length will be shorter than the eventual filename
Comment #9
joelpittetChanging component, it probably should be this or base system from a quick issue queue search.
Comment #10
alexpottThis 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
Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedWhy do we need that? If Drupal doesn't find the expected file (cache miss) it generates it.
Comment #12
alexpottBecause leaving around a whole load of files that aren't used is super confusing for people.
Comment #13
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedok, 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.
Comment #14
alexpottNope 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.
Comment #15
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedok, so put the same post-update in both patches?
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedAdding 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.
Comment #17
YesCT CreditAttribution: YesCT commentedI'm reviewing this now.
Comment #18
YesCT CreditAttribution: YesCT commentedgrep'd and capital PHP is what we do.
without patch
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
none with the patch
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?)
Comment #20
catchCommitted/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.
Comment #21
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedDoes not hurt anything, and would start mitigating the windows issue?
Comment #22
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedJust add the post update in the right place.
Comment #23
YesCT CreditAttribution: YesCT commentedI'll give this a check.
Comment #24
YesCT CreditAttribution: YesCT commentedread 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?
Comment #25
YesCT CreditAttribution: YesCT commentedComment #27
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedsporadic fail?
Comment #29
catchI 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.
Comment #33
xjmIssue tags got cloned on this incorrectly.
Comment #34
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedSince this is not going to 8.2, it's fixed, right?
Comment #35
cilefen CreditAttribution: cilefen commentedYes I think so.