[Cache] Make directory hashing case insensitive #20453
Conversation
| @@ -147,7 +147,7 @@ protected function doSave(array $values, $lifetime) | |||
| private function getFile($id, $mkdir = false) | |||
| { | |||
| $hash = str_replace('/', '-', base64_encode(md5($id, true))); | |||
There was a problem hiding this comment.
Why does it base64_encode an md5 hash? Seems pointless to me.
There was a problem hiding this comment.
There is a constraint on the length of paths (at least on Windows).
strlen(md5('', false))=> 32strlen(base64_encode(md5('', true)))=> 22 (padding=removed)
That's the reason.
There was a problem hiding this comment.
Using md5 which has known collisions with just 64 bytes is already unsafe. But ignoring the case of base64_encode will make it even worse. This basically means it unsafe to use untrusted cache keys for the cache component. This needs to be highlighted.
There was a problem hiding this comment.
MD5 weaknesses make it not suitable for any security related job (signatures, etc.). Here, we only need to be reasonably sure that keys won't collide. Making two bytes case insensitive won't invalidate this.
As far as security is concerned, if a collision occurs, it will be caught by the unhashed key comparison done when opening files.
There was a problem hiding this comment.
Hum, my bad, I'm wrong here! Give me lunch time to give the correct number so we can talk with numbers...
There was a problem hiding this comment.
Whatever the probably is, it just means, you cannot trust cache keys. E.g. it's not safe to use the current request info as cache key. That should be documented.
There was a problem hiding this comment.
As far as security is concerned, if a collision occurs, it will be caught by the unhashed key comparison done when opening files.
This doesn't solve the security problem. When a cache key doesn't exist which it looks like when a collision happens with the key comparison, then clients usually write the cache with the collided key. So it will overwrite the cache of another key. This basically means anybody can prune the cache by using collisions.
There was a problem hiding this comment.
So for the proba part, worse case on windows: the 128bits MD5 being turned to case insensitive base64 means 38^21*4 = 6e+33 key space cardinality. On NTFS that handles at most 2^32-1 files per partition, this means that filling at 100% has 1/1.4e+24 chance of generating a collision. I'd say this is enough space for any use cases here.
There was a problem hiding this comment.
So, considering that I still think having short paths is desired to make things work seamlessly on Windows, considering also that truncating a sha256 looks fine for the use case, I replaced md5 by a truncated sha256. Case insensitivity is here equivalent to truncating a bit more the hash on Windows, which seems safe also for the use case (see previous comment). OK for you?
f64d931 to
d892be1
Compare
d892be1 to
6d4a658
Compare
|
Status: Reviewed |
|
Thank you @nicolas-grekas. |
…rekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Make directory hashing case insensitive | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20450 | License | MIT | Doc PR | - Fixes this situation: a Windows OS, running a Debian via VirtualBox and Vagrant. So the cache folder is on a Windows partition while the PHP command is running inside Debian... Not testable without this specific setup... Commits ------- 6d4a658 [Cache] Make directory hashing case insensitive
|
Btw, PHP 7.1 might have fixed the Windows PHP length limit.
https://github.com/php/php-src/blob/php-7.1.0RC5/UPGRADING I didn't try it. But might we worth considering to decrease collision probability. |
Fixes this situation: a Windows OS, running a Debian via VirtualBox and Vagrant. So the cache folder is on a Windows partition while the PHP command is running inside Debian...
Not testable without this specific setup...