Create hash_pbkdf2 function addition #105

Merged
merged 8 commits into from Jul 10, 2012

Conversation

Projects
None yet
4 participants
@ircmaxell
Contributor

ircmaxell commented Jun 12, 2012

This pull request adds a new function to the hash package: hash_pbkdf2(), providing the ability to natively hash content using the PKCS5 approved PBKDF2 algorithm. See Wikipedia and RSA for more information about the algorithm.

This patch refactors the internal implementation of hash_hmac() to allow code reuse between it and the new hash_pbkdf2() function. No internal APIs were changed, and the only API addition is the PHP function hash_pbkdf2(). A few static inline functions were either added, or extracted from the inside of hash_hmac and its implementation. These refactorings should have no public impact, since they are static to the extension.

NEWS
@@ -42,6 +42,9 @@ PHP NEWS
still exists for backward compatibility but is doing nothing). (Pierrick)
. Fixed bug #54995 (Missing CURLINFO_RESPONSE_CODE support). (Pierrick)
+- Hash
+ . Added support for PBKDF2 via hash_pbkdf2()F

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

The F looks strange :) Also I'm missing your name there :)

@nikic

nikic Jun 12, 2012

Member

The F looks strange :) Also I'm missing your name there :)

@scottmac

This comment has been minimized.

Show comment
Hide comment
@scottmac

scottmac Jun 12, 2012

Should we make this more generic so that other derived key hash functions can be added? Like scrypt too?

Should we make this more generic so that other derived key hash functions can be added? Like scrypt too?

ext/hash/hash.c
+ /* Reduce the key first */
+ ops->hash_init(context);
+ ops->hash_update(context, (unsigned char *) key, key_len);
+ ops->hash_final((unsigned char *) K, context);

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

Are the two unsigned char * cast necessary here? Look to me like they are already declared to be of that type.

@nikic

nikic Jun 12, 2012

Member

Are the two unsigned char * cast necessary here? Look to me like they are already declared to be of that type.

ext/hash/hash.c
+ } else {
+ memcpy(K, key, key_len);
+ }
+ /* XOR the key with 0x36 to get the ipad) */

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

There's a leftover ) at the end of the comment :)

@nikic

nikic Jun 12, 2012

Member

There's a leftover ) at the end of the comment :)

@ircmaxell

This comment has been minimized.

Show comment
Hide comment
@ircmaxell

ircmaxell Jun 12, 2012

Contributor

@scottmac As far as that's concerned, I'm not sure how that would look. The API between PBKDF2 and SCrypt are very different. One takes a hash algorithm, a key (password), a salt, an iteration count, and a length. The other takes a key (password), a salt, and 3 integer parameters: N, p, r. So without having a generic "options" array (which I wouldn't care for).

Instead, why not add a second function: hash_scrypt()?

Contributor

ircmaxell commented Jun 12, 2012

@scottmac As far as that's concerned, I'm not sure how that would look. The API between PBKDF2 and SCrypt are very different. One takes a hash algorithm, a key (password), a salt, an iteration count, and a length. The other takes a key (password), a salt, and 3 integer parameters: N, p, r. So without having a generic "options" array (which I wouldn't care for).

Instead, why not add a second function: hash_scrypt()?

ext/hash/hash.c
+ }
+
+ if (length < 0) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Length Must Be Greater Than Or Equal To 0: %ld", length);

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

This and the previous error message Have All Words Capitalized. PHP doesn't usually make use of that scheme :)

@nikic

nikic Jun 12, 2012

Member

This and the previous error message Have All Words Capitalized. PHP doesn't usually make use of that scheme :)

ext/hash/hash.c
+ temp = emalloc(ops->digest_size);
+
+ /* Setup Keys that will be used for all hmac rounds */
+ memset(K2, 0, ops->block_size);

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

Not that it makes much of a difference, but is this memset needed? If I see it right then you are fully overwriting K2 anyways in the php_hash_string_xor_char call below.

@nikic

nikic Jun 12, 2012

Member

Not that it makes much of a difference, but is this memset needed? If I see it right then you are fully overwriting K2 anyways in the php_hash_string_xor_char call below.

ext/hash/hash.c
+ if (length == 0) {
+ length = ops->digest_size;
+ }
+ digest_length = length;

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

This line looks incorrectly indented. Maybe using spaces here?

@nikic

nikic Jun 12, 2012

Member

This line looks incorrectly indented. Maybe using spaces here?

@ircmaxell

This comment has been minimized.

Show comment
Hide comment
@ircmaxell

ircmaxell Jun 12, 2012

Contributor

@nikic Thanks! I've updated all of the issues you've identified. The reason for the casts, was that before the refactor it was needed, but I never changed the argument after I extracted the method. You are correct that the second memset is not needed. I've removed that as well.

I'll push the changes once make test passes to ensure I didn't bork anything.

Thanks!

Contributor

ircmaxell commented Jun 12, 2012

@nikic Thanks! I've updated all of the issues you've identified. The reason for the casts, was that before the refactor it was needed, but I never changed the argument after I extracted the method. You are correct that the second memset is not needed. I've removed that as well.

I'll push the changes once make test passes to ensure I didn't bork anything.

Thanks!

ext/hash/hash.c
+ /* result += temp */
+ memcpy(result + ((i - 1) * ops->digest_size), temp, ops->digest_size);
+ }
+ /* Zero potentiall sensitive variables */

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

nit: potentiall should probably be potentially

@nikic

nikic Jun 12, 2012

Member

nit: potentiall should probably be potentially

ext/hash/hash.c
@@ -202,10 +203,45 @@ static void php_hash_do_hash(INTERNAL_FUNCTION_PARAMETERS, int isfilename, zend_
}
/* }}} */
+static inline void php_hash_string_xor_char(unsigned char *out, const unsigned char *in, const unsigned char xor_with, const int length) {
+ int i;
+ for(i=0; i < length; i++) {

This comment has been minimized.

@nikic

nikic Jun 12, 2012

Member

nit: This for loop (and the one below it) need a space after the keyword :)

@nikic

nikic Jun 12, 2012

Member

nit: This for loop (and the one below it) need a space after the keyword :)

ircmaxell added some commits Jun 12, 2012

Merge remote branch 'upstream/master' into hash_pbkdf2
* upstream/master: (101 commits)
  Fixed Bug #62500 (Segfault in DateInterval class when extended)
  Fixed test bug #62312 (warnings changed one more time)
  fix valgrind warning
  fix valgrind warning
  fixed #62433 test for win
  update NEWS
  Fixed bug #62499 (curl_setopt($ch, CURLOPT_COOKIEFILE, "") returns false)
  appease MSVC (doesnt like unary minus of unsigned ints)
  appease MSVC (doesnt like unary minus of unsigned ints)
  appease MSVC (doesnt like unary minus of unsigned ints)
  - Fixed bug #62507 (['REQUEST_TIME'] under mod_php5 returns miliseconds instead of seconds)
  Fixed Bug #62500 (Segfault in DateInterval class when extended)
  Added in NEWS and UPGRADING for feature 55218
  Fix two issues with run-tests.php
  Fix potential integer overflow in nl2br
  Fix potential integer overflow in bin2hex
  This wil be PHP 5.3.16
  Revert change 3f3ad30: There shouldn't be new features in 5.3, especially not if they aren't in 5.4, too.
  fix (signed) integer overflow (part of bug #52550
  fix (signed) integer overflow (part of bug #52550
  ...

@php-pulls php-pulls merged commit 731c6fd into php:master Jul 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment