ext/hash: improve PBKDF2 performance

[Sjord]

This is a security bug, since it makes PBKDF2 faster for attackers than defenders. So it should be fixed from PHP 8.2 on.

[LamentXU123]

Have you benchmarked the difference between it? Generally we treat performance improvements as a feature to be landed on master only and only vulnerbility fixes qualifies for 8.2. @php/release-managers-82 ?

[LamentXU123]

Feel free to add a UPGRADING entry.

[Sjord]

time sapi/cli/php -r 'hash_pbkdf2("sha256", "pass", "salt", 1e7);'

• PHP-8.2: 18 seconds
• this branch: 10 seconds

[ArtUkrainskiy]

I measured this on PHP-8.2 03e9593 (5db5b7a merges cleanly) with GCC 13.3, GCC 14.2, clang 18.1 and clang 20.1 on an i7-13700H, built with the flags of the official Docker image; PHP-8.2 does not build with --enable-werror on these compilers, so without it.

Change in CPU cycles per call with the PR, relative to PHP-8.2 (negative = fewer cycles, faster); median of five alternating runs, a fixed 28-byte password and 16-byte salt (~: within the noise of PHP-8.2 measured against itself):

call	PHP-8.2, CPU cycles per call (GCC 13.3)	GCC 13.3	GCC 14.2	clang 18.1	clang 20.1
hash_pbkdf2('md5', …, 1), 1 iteration	1,845	+5%	+4%	+2%	+4%
hash_pbkdf2('sha1', …, 1), 1 iteration	2,097	+5%	+4%	+3%	+4%
hash_pbkdf2('sha256', …, 1), 1 iteration	4,158	+2%	+2%	0% ~	+1%
hash_pbkdf2('sha512', …, 1), 1 iteration	5,534	+3%	+1%	+1%	+2%
hash_pbkdf2('sha3-256', …, 1), 1 iteration	6,378	0% ~	+2%	+1%	+1%
hash_pbkdf2('whirlpool', …, 1), 1 iteration	6,695	+1%	+2%	+4%	+2%
hash_pbkdf2('md5', …, 1000), 1000 iterations	1,298,394	−43%	−43%	−44%	−44%
hash_pbkdf2('sha1', …, 1000), 1000 iterations	1,424,314	−43%	−46%	−46%	−45%
hash_pbkdf2('sha256', …, 1000), 1000 iterations	3,504,329	−48%	−48%	−51%	−49%
hash_pbkdf2('sha512', …, 1000), 1000 iterations	4,865,934	−47%	−47%	−48%	−49%
hash_pbkdf2('sha3-256', …, 1000), 1000 iterations	5,422,488	−51%	−52%	−53%	−52%
hash_pbkdf2('whirlpool', …, 1000), 1000 iterations	7,168,768	−35%	−35%	−35%	−35%

• With 1000 iterations: 43–53% fewer cycles, 35% for whirlpool, on all four compilers; with 10 iterations 36–46% (32% for whirlpool), in the report. A round used to hash the key block and then the data; for md5 to sha3-256 the digest and its padding fit one block, so two compressions per round become one, while a whirlpool digest and its padding take two blocks, so three become two. The instruction counts move by similar amounts.
• With 1 iteration: 0–5% more cycles. The precomputation costs as much as the round it replaces, plus two allocations and two copies; with the iteration counts PBKDF2 is used with, it disappears.

Correctness: I compared the output with PHP-8.2 for all 44 algorithms of hash_hmac_algos(), with 1, 2, 3 and 5 iterations, hex and raw output, the default length, 200 hex characters and 100 raw bytes (several output blocks), random byte strings up to 300 bytes as passwords and as salts, half of them on or next to the block sizes 64 to 144: identical on 3,000 inputs with each of the four compilers and on 1,000 under ASan/UBSan. ext/hash/tests pass, and they and the differential test each catch all six deliberate mistakes I put into the new code (a round started from a fresh state, the inner state built from the opad block, the outer state without its key block, the inner round run from the outer state in the first block and in the iterations, the outer round left out), so the change needs no new test.

Code: one suggestion. K1, K2 and computed_salt are wiped with ZEND_SECURE_ZERO() before efree(), but inner_context and outer_context, which hold the hash states after absorbing K1 and K2 and are as good as the key for computing the HMAC, are freed without it:

ZEND_SECURE_ZERO(inner_context, ops->context_size);
ZEND_SECURE_ZERO(outer_context, ops->context_size);
efree(inner_context);
efree(outer_context);

The const added to php_hash_copy() and php_hash_copy_func_t is not needed for the optimisation; for PHP-8.2 that may matter to an extension that registers its own algorithm with a copy function of the old signature without a cast, which GCC 14 and clang reject by default. Whether this goes into PHP-8.2 is for the release managers.

About security: the speed-up does not make passwords stronger by itself. With the same iteration count an attacker's cost per guess is the same as before; the gain becomes strength only where an application raises the count. It also does nothing for existing hashes: PBKDF2 uses the password in every iteration, so a stored hash cannot be extended to more iterations without the original password, only recomputed with a higher count when the user next logs in. For applications concerned about password storage, I would recommend password_hash() (bcrypt by default, Argon2id where available) with password_needs_rehash() on login rather than hash_pbkdf2().

No correctness issues found. LGTM.

Full tables, scripts and data: https://github.com/ArtUkrainskiy/php-src-bench/tree/main/reports/pr-24188-pbkdf2

[Sjord]

Ping @adoy @saundefined @ramsey as PHP 8.2 release managers.

[LamentXU123]

I think this is correct. I agree to merge it to master at least but for lower branches I don't have strong opinions.