Bug #74961 [Opn->Csd]: "Disallow non-crypto hashes in HMAC and PBKDF2" implemented poorly
Edit report at https://bugs.php.net/bug.php?id=74961&edit=1
ID: 74961
Updated by: cmb@php.net
Reported by: bluebaroncanada at gmail dot com
Summary: "Disallow non-crypto hashes in HMAC and PBKDF2"
implemented poorly
-Status: Open
+Status: Closed
Type: Bug
Package: *Encryption and hash functions
PHP Version: 7.2.0beta1
-Assigned To:
+Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
hash_hmac_algos() is available as of PHP 7.2.0, and the
documentation of hash_hmac(_file)() has been improved regarding
non-cryptographic algos[1], so it seems this ticket can be closed.
[1] <http://svn.php.net/viewvc?view=revision&revision=344350>
Previous Comments:
------------------------------------------------------------------------
[2017-08-10 11:55:17] bluebaroncanada at gmail dot com
Err. Constants. ... variable constants. Brilliant.
------------------------------------------------------------------------
[2017-08-10 11:42:56] 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 09:12:04] 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 03:42:49] 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 03:40:16] 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.
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=74961
--
Edit this bug report at https://bugs.php.net/bug.php?id=74961&edit=1
Thread (10 messages)