Optimize hash_pbkdf2() by reusing HMAC contexts #21175
Conversation
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Since this is security-sensitive code: Can you please split the individual optimizations into separate commits for ease of review?
- Migration from float to integer operations to calculate the digest lengths.
- HMAC context reuse
- SHA-256 special case
- CommonCrypto SHA-256 special case
- Possibly other individually useful migration steps that I missed
|
Follow-up with exact commit references for easier navigation:
And |
gregharmon
force-pushed
the
codex/pbkdf2-hmac-context-reuse
branch
from
daa2d0f to
1e94d0b
Compare
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Thank you for splitting the commits. Some more superficial review. I'll need to take a deeper look at the logic when it isn't almost midnight.
| /* {{{ php_hash_sha256_final32_from_context | ||
| Compute SHA256(base_ctx || data[32]) where base_ctx already hashed one full 64-byte block. | ||
| */ | ||
| void php_hash_sha256_final32_from_context(unsigned char digest[32], const PHP_SHA256_CTX *context, const unsigned char data[32]) |
There was a problem hiding this comment.
I would expect this to be static and to sit in hash.c then.
| digest[i * 4] = (unsigned char) ((state[i] >> 24) & 0xff); | ||
| digest[(i * 4) + 1] = (unsigned char) ((state[i] >> 16) & 0xff); | ||
| digest[(i * 4) + 2] = (unsigned char) ((state[i] >> 8) & 0xff); | ||
| digest[(i * 4) + 3] = (unsigned char) (state[i] & 0xff); |
There was a problem hiding this comment.
For easier scanning:
| digest[i * 4] = (unsigned char) ((state[i] >> 24) & 0xff); | |
| digest[(i * 4) + 1] = (unsigned char) ((state[i] >> 16) & 0xff); | |
| digest[(i * 4) + 2] = (unsigned char) ((state[i] >> 8) & 0xff); | |
| digest[(i * 4) + 3] = (unsigned char) (state[i] & 0xff); | |
| digest[(i * 4) + 0] = (unsigned char) ((state[i] >> 24) & 0xff); | |
| digest[(i * 4) + 1] = (unsigned char) ((state[i] >> 16) & 0xff); | |
| digest[(i * 4) + 2] = (unsigned char) ((state[i] >> 8) & 0xff); | |
| digest[(i * 4) + 3] = (unsigned char) ((state[i] >> 0) & 0xff); |
| zend_string *returnval; | ||
| unsigned char *digest, *result, *K = NULL, counter[4]; | ||
| zend_long loops, i, j, digest_length = 0; | ||
| const size_t digest_size = 32; | ||
| const size_t block_size = 64; | ||
| PHP_SHA256_CTX inner_context, outer_context, context; |
There was a problem hiding this comment.
Can you also pull these declarations down as appropriate (particularly the loop variables and the returnval)?
| zend_string *returnval; | ||
| unsigned char *derived = NULL; | ||
| zend_long derived_len = 0; |
There was a problem hiding this comment.
Ditto, the returnval should not be sitting around uninitialized, this just invites refactoring mistakes.
2 participants
Fixes #9604.
Optimize
hash_pbkdf2()inext/hashby reusing precomputed HMAC contexts instead of rehashing key blocks each round.Changes
ext/hash/tests/gh9604.phpt.Behavior
No API or semantic changes intended: validation, return formats, and output bytes are unchanged.
Tests
make test TESTS="ext/hash/tests/hash_pbkdf2_basic.phpt ext/hash/tests/hash_pbkdf2_error.phpt ext/hash/tests/gh9604.phpt"make test TESTS="ext/hash/tests"Benchmark
Setup:
hash_pbkdf2('sha256', 'password', 'salt', 200000, 64, false)hrtime(true), median reportedObserved on this machine:
135.47 msmedian28.28 msmedian-of-12 (sample medians27.68–28.46 ms)~4.79x