php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #74961 "Disallow non-crypto hashes in HMAC and PBKDF2" implemented poorly
Submitted: 2017-07-21 09:10 UTC Modified: 2017-08-10 11:42 UTC
From: bluebaroncanada at gmail dot com Assigned:
Status: Open Package: *Encryption and hash functions
PHP Version: 7.2.0beta1 OS:
Private report: No CVE-ID:
Have you experienced this issue?
Rate the importance of this bug to you:

 [2017-07-21 09:10 UTC] bluebaroncanada at gmail dot com
Description:
------------
Commit https://github.com/php/php-src/commit/d89d149edf39cf4ce9ab41979f246e82510d43a5#commitcomment-23109176 is a poor implementation of its goal.
It salted the code and added a new is_crypto function which wasn't necessary and is also insufficient.
It should provide a new hash_algos_hmac to extend http://php.net/manual/en/function.hash-algos.php
The documentation now says "Returns FALSE when algo is unknown." and 7.2.0	Usage of non-cryptographic hash functions (adler32, crc32, crc32b, fnv132, fnv1a32, fnv164, fnv1a64, joaat) was disabled.  However it does not say that it will return false only when providing one of these non-cryptographic functions, which is what it actually does.  There could be future deprecation of more functions.
I suggest that either the is_crypto function is extended to the users, but better than that, we should remove the is_crypto function and create the hash_algos_hmac to extend the hash_algos function for the same reason its predecessor exits and also so that we can use that function internally to decide if the hashing algorithm should be available and thus easily allow future deprecation and pedantic decisions on available algorithms without further damaging the code.
The reason that this bothers me so is that phpseclib extends this functionality and it cannot properly test with this implementation or properly extend this functionality to pass on to its users.  In the future, if there is further deprecation, users would be treated with an undetectable error.
This commit should be removed and rewritten.

Test script:
---------------
if (!hash_hmac('crc32', 'The quick brown fox jumped over the lazy dog.', 'secret'))
die('run for your lives');


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-07-23 18:39 UTC] pollita@php.net
I'm a little unclear on what you're looking for when you say "rewritten".

I see the request for hash_algos_hmac() which is entirely reasonable (or a variant thereof), but I'm not sure what the rest of the request is...
 [2017-07-27 19:07 UTC] bluebaroncanada at gmail dot com
The rest of the request is to undo some of these changes that are cumbersome, difficult to maintain, and hard to research to build new code on top.  It's certainly unnecessary and highly-coupled to put is_crypto functions that are only used once inside the hash functions.  That's just an extra layer of coupling for no reason.  It's going to make it more likely to have a bug.
Instead, let's have a canonical function that tells all that both the user and the developer can rely on.
 [2017-08-10 03:23 UTC] pollita@php.net
I'm confused why you care how the flag (flag mind you, not function) is implemented.  Why does it matter that the algo has a bit set on its definition rather than having a completely separate lookup table?  The way it looks from userspace is the same.
 [2017-08-10 03:40 UTC] bluebaroncanada at gmail dot com
TBH, I really don't care in the end.  All I'm saying is that now there's the potential for more problems.

"abchash doesn't implement is_crypto = true"
"xyzhash doesn't implement is_crypto = false"
"Oh.  My bad.  I put it in the function that lists all available functions. I'll add that."
"Oh.  Should I check both places?  Why do we have two places?"

"We just invented this new class of hashing functions.  Should it have is_crypto? 
 Hmm.  Well maybe yes, maybe no.  It doesn't really fit that paradigm."

Not to mention it's simply extraneous if there's going to be a lookup function and is_crypto.  It's a make-work addition.
 [2017-08-10 03:42 UTC] bluebaroncanada at gmail dot com
The more we add things like this, too, the heavier the code gets.  The more we have to remember to implement new stuff.  The more cumbersome and time consuming it is.  I think, just generally, we should have high prejudice against things like that.
 [2017-08-10 09:12 UTC] nikic@php.net
There are two reasons why this was moved into the hash structure, rather than having a secondary structure:
 * To make sure that there is a "single source of truth" regarding hash function metadata. This makes it impossible to forget to specify whether or not the function is cryptographic when a new hash is added.
 * To avoid double-lookups. It's not good to scan through lists multiple times, especially if it involves case-insensitive string comparisons. I've recently done a similar change in mbstring (https://github.com/php/php-src/commit/633a471ba0c9acc6d1cca04880c1e69e7b2dc18e), because it turned out that scanning those lists was dominating the runtime of some functions.
 [2017-08-10 11:42 UTC] bluebaroncanada at gmail dot com
Hmm, nikic.  That is a pretty good reason.  Then perhaps this should be extended to the user, and the docs updated to reflect that change.  Unless the user has access to that function, the user will still have to perform that lookup.  Perhaps there's a way to get this to constant time with a hash table or hash-name variable constants?
 [2017-08-10 11:55 UTC] bluebaroncanada at gmail dot com
Err.  Constants. ... variable constants.  Brilliant.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC