PR for Password Hash RFC #191

ircmaxell · 2012-09-12T15:58:49Z

This is the pull request for the Password Hash RFC which was accepted.

Please review the changes, and if all looks good I will merge the PR in.

Thanks

Anthony

…nt overflow on allocation

Conflicts: ext/standard/crypt.c

* upstream/master: (34 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 ...

* upstream/master: (393 commits) forked two tests for windows Fixed bug #50997 (SOAP Error when trying to submit 2nd Element of a choice) Fixed bug #50997 (SOAP Error when trying to submit 2nd Element of a choice). Fixed bug #50997 (SOAP Error when trying to submit 2nd Element of a choice). Fixed bug #50997 (SOAP Error when trying to submit 2nd Element of a choice) Fixed bug #50997 (SOAP Error when trying to submit 2nd Element of a choice) Bug #49510: Boolean validation fails with FILTER_NULL_ON_FAILURE with empty string or false Implemented ReflectionFunction::isGenerator() Allow null as a default value for length in mb_substr() and mb_strcut() Allow null as a default value for length in mb_substr() and mb_strcut() folder Initializing optional argument description in assert() Initializing optional argument description in assert() Fix test failed due to new Token T_YIELD fix NEWS Fix leak when yielding array as key Drop obsolete test Remove extra blank in notice message, should act as same as vm Fixed bug #62987 (Assigning to ArrayObject[null][something] overrides all undefined variables) assert() user message ...

ircmaxell · 2012-09-13T14:34:23Z

I've just added a commit to switch the algorithms definition to an ENUM (cleaner and allows for better type checking in the algos functions...

nikic · 2012-09-17T14:20:36Z

ext/standard/php_password.h

+typedef enum {
+	PASSWORD_UNKNOWN,
+	PASSWORD_BCRYPT
+} php_password_algos;


I'd rather call this enum php_password_algo (singular) as it fits in better with parameters and return values. Also I don't see why you removed the PHP_ prefix for the enum values. Those are global, so they should be prefixed too. (Only C++11 has proper non-global enum class values.)

Fixed by the next push.

nikic · 2012-09-17T15:01:59Z

ext/standard/basic_functions.c

+ZEND_BEGIN_ARG_INFO_EX(arginfo_password_get_info, 0, 0, 1)
+	ZEND_ARG_INFO(0, hash)
+ZEND_END_ARG_INFO()
+ZEND_BEGIN_ARG_INFO_EX(arginfo_password_needs_rehash, 0, 0, 1)


Here again the last arg should be 2 (as the algo is required too).

nikic · 2012-09-17T15:25:54Z

ext/standard/password.c

+			{
+				int newCost = PHP_PASSWORD_BCRYPT_COST, cost = 0;
+
+				if (options && zend_symtable_find(options, "cost", 5, (void **) &option_buffer) == SUCCESS) {


Use of the symtable function is not necessary here (only required when you're taking user input as the key), could be just zend_hash_find. The 5 would be better written as sizeof("cost"), to clarify its meaning.

nikic · 2012-09-17T15:46:09Z

ext/standard/password.c

+				if (options && zend_symtable_find(options, "cost", 5, (void **) &option_buffer) == SUCCESS) {
+					convert_to_long_ex(option_buffer);
+					newCost = Z_LVAL_PP(option_buffer);
+					zval_ptr_dtor(option_buffer);


This zval_ptr_dtor isn't right here. zend_symtable_find does not add an additional ref. Though I'm a bit confused now how this should be properly done when convert_to_long_ex is used...

An example what would usually happen:

==7601== Invalid read of size 4 ==7601== at 0x8250A1E: _zval_ptr_dtor (zend.h:404) ==7601== by 0x826235E: _zval_ptr_dtor_wrapper (zend_variables.c:180) ==7601== by 0x8276453: zend_hash_destroy (zend_hash.c:560) ==7601== by 0x8261EFB: _zval_dtor_func (zend_variables.c:43) ==7601== by 0x82A6E58: zend_do_fcall_common_helper_SPEC (zend_variables.h:35) ==7601== by 0x82AE0B9: ZEND_DO_FCALL_SPEC_CONST_HANDLER (zend_vm_execute.h:2447) ==7601== by 0x82A453E: execute_ex (zend_vm_execute.h:440) ==7601== by 0x82A4614: execute (zend_vm_execute.h:464) ==7601== by 0x826665C: zend_execute_scripts (zend.c:1309) ==7601== by 0x81C861B: php_execute_script (main.c:2459) ==7601== by 0x83C5358: do_cli (php_cli.c:988) ==7601== by 0x83C6893: main (php_cli.c:1364) ==7601== Address 0x43ebe68 is 8 bytes inside a block of size 20 free'd ==7601== at 0x402B06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==7601== by 0x822C56A: _efree (zend_alloc.c:2433) ==7601== by 0x8250AFD: _zval_ptr_dtor (zend_execute_API.c:440) ==7601== by 0x81BDC94: zif_password_needs_rehash (password.c:250) ==7601== by 0x82A5FBE: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:655)

=> The value is first freed here and later in the hash dtor.

All memory leaks that I could find have been rectified. Please test again...

nikic · 2012-10-06T15:35:36Z

I'm still getting a lot of valgrind warnings, e.g.

=29932== Invalid read of size 1
==29932==    at 0x8261B61: _zval_dtor_func (zend_variables.c:35)
==29932==    by 0x82507FE: _zval_ptr_dtor (zend_variables.h:35)
==29932==    by 0x826208A: _zval_ptr_dtor_wrapper (zend_variables.c:180)
==29932==    by 0x827617F: zend_hash_destroy (zend_hash.c:560)
==29932==    by 0x8261C27: _zval_dtor_func (zend_variables.c:43)
==29932==    by 0x82A6B84: zend_do_fcall_common_helper_SPEC (zend_variables.h:35)
==29932==    by 0x82ADDE5: ZEND_DO_FCALL_SPEC_CONST_HANDLER (zend_vm_execute.h:2447)
==29932==    by 0x82A426A: execute_ex (zend_vm_execute.h:440)
==29932==    by 0x82A4340: execute (zend_vm_execute.h:464)
==29932==    by 0x8266388: zend_execute_scripts (zend.c:1309)
==29932==    by 0x81C8347: php_execute_script (main.c:2459)
==29932==    by 0x83C5084: do_cli (php_cli.c:988)
==29932==  Address 0x43ee77b is 3 bytes inside a block of size 4 free'd
==29932==    at 0x402B06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==29932==    by 0x822C296: _efree (zend_alloc.c:2433)
==29932==    by 0x825953C: convert_to_long_base (zend_operators.c:394)
==29932==    by 0x82593FD: convert_to_long (zend_operators.c:364)
==29932==    by 0x81BDF0E: zif_password_hash (password.c:332)
==29932==    by 0x82A5CEA: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:655)
==29932==    by 0x82ADDE5: ZEND_DO_FCALL_SPEC_CONST_HANDLER (zend_vm_execute.h:2447)
==29932==    by 0x82A426A: execute_ex (zend_vm_execute.h:440)
==29932==    by 0x82A4340: execute (zend_vm_execute.h:464)
==29932==    by 0x8266388: zend_execute_scripts (zend.c:1309)
==29932==    by 0x81C8347: php_execute_script (main.c:2459)
==29932==    by 0x83C5084: do_cli (php_cli.c:988)

==29932== Invalid free() / delete / delete[] / realloc()
==29932==    at 0x402B06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==29932==    by 0x822C296: _efree (zend_alloc.c:2433)
==29932==    by 0x8261BCA: _zval_dtor_func (zend_variables.c:36)
==29932==    by 0x82507FE: _zval_ptr_dtor (zend_variables.h:35)
==29932==    by 0x826208A: _zval_ptr_dtor_wrapper (zend_variables.c:180)
==29932==    by 0x827617F: zend_hash_destroy (zend_hash.c:560)
==29932==    by 0x8261C27: _zval_dtor_func (zend_variables.c:43)
==29932==    by 0x82A6B84: zend_do_fcall_common_helper_SPEC (zend_variables.h:35)
==29932==    by 0x82ADDE5: ZEND_DO_FCALL_SPEC_CONST_HANDLER (zend_vm_execute.h:2447)
==29932==    by 0x82A426A: execute_ex (zend_vm_execute.h:440)
==29932==    by 0x82A4340: execute (zend_vm_execute.h:464)
==29932==    by 0x8266388: zend_execute_scripts (zend.c:1309)
==29932==  Address 0x43ee778 is 0 bytes inside a block of size 4 free'd
==29932==    at 0x402B06C: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==29932==    by 0x822C296: _efree (zend_alloc.c:2433)
==29932==    by 0x825953C: convert_to_long_base (zend_operators.c:394)
==29932==    by 0x82593FD: convert_to_long (zend_operators.c:364)
==29932==    by 0x81BDF0E: zif_password_hash (password.c:332)
==29932==    by 0x82A5CEA: zend_do_fcall_common_helper_SPEC (zend_vm_execute.h:655)
==29932==    by 0x82ADDE5: ZEND_DO_FCALL_SPEC_CONST_HANDLER (zend_vm_execute.h:2447)
==29932==    by 0x82A426A: execute_ex (zend_vm_execute.h:440)
==29932==    by 0x82A4340: execute (zend_vm_execute.h:464)
==29932==    by 0x8266388: zend_execute_scripts (zend.c:1309)
==29932==    by 0x81C8347: php_execute_script (main.c:2459)
==29932==    by 0x83C5084: do_cli (php_cli.c:988)
==29932==

nikic · 2012-10-06T15:40:20Z

Also from a quick look at the new code, you're using INIT_PZVAL_COPY to copy the zval, but that macro doesn't invoke the copy constructor. You should use MAKE_COPY_ZVAL instead. (This might be related to the invalid reads above. The lack of copy_ctor probably causes a double free on a string.)

ircmaxell · 2012-10-06T16:17:39Z

Can you provide the test code, so I can integrate it with the unit tests?
On Oct 6, 2012 4:40 PM, "nikic" [email protected] wrote:

Also from a quick look at the new code, you're using INIT_PZVAL_COPY to
copy the zval, but that macro doesn't invoke the copy constructor. You
should use MAKE_COPY_ZVAL instead. (This might be related to the invalid
reads above. The lack of copy_ctor probably causes a double free on a
string.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/191#issuecomment-9199240.

nikic · 2012-10-06T16:24:40Z

@ircmaxell I get that when running ext/standard/tests/password/password_bcrypt_errors.phpt. I compiled using --disable-all --enable-debug --enable-maintainer-zts.

Anthony Ferrara and others added 30 commits June 24, 2012 22:44

Base structure for passsword_create and password_make_salt

c77f2c2

Actually complete password_create()

7e41980

Implement password_verify

6574028

Fix memory leak on branch

f7097d9

Basic random generator added to make_salt

18d3bd9

More error checking, and some cleaning up for password.c

618f262

Implement openssl support for make_salt

41d7374

Refactor salt generation, rename password_create to password_hash

2d4b7cb

Implement php.ini setting password.bcrypt_cost

232da90

Add tests for password hashing

e505316

Update tests to check ini setting

2b9591f

Add tests and error checking for large salt requested values to preve…

5f44be0

…nt overflow on allocation

Fix formatting issues in password.c

0dd2f16

Refactor crypt to use an external working function

6bb3865

Refactor password.c a bit, add different error checking

da3d8bf

Merge remote branch 'upstream/master' into hash_password

9e18e57

Conflicts: ext/standard/crypt.c

More refactoring of crypt into php_crypt, and fixing memory allocation

9c1445c

Update password.c to use safe_emalloc in sensitive places

f53112f

Remove php.ini setting for default bcrypt cost

6cc3c65

Some more refactoring, make algo no longer optional

6943f2a

Update signature info for changing algo to an ordinal

886527d

Implement password_needs_rehash() function

5160dc1

Fix issue with int vs long parameter

db86d54

Implement password_get_info() function

ee7e799

Cleanup whitespace issues

9d3630b

Switch second parameter to password_make_salt to be a flag

707c907

Remove password_make_salt() from the implementation

e05413c

Refactoring to use size_t instead of int most places

db41f9f

Switch to using an ENUM for algorithms instead of a constant

83cfff4

nikic reviewed Sep 17, 2012
View reviewed changes

ircmaxell added 2 commits September 17, 2012 10:52

A bunch of naming convention fixes. No functionality changes

e034a46

Fix ucwords error casing

44c2624

nikic reviewed Sep 17, 2012
View reviewed changes

Fix arg info for required params passed to needs_rehash

6fd5ba5

nikic reviewed Sep 17, 2012
View reviewed changes

Refactor slightly to enable cleaner readability

8bd79d1

nikic reviewed Sep 17, 2012
View reviewed changes

ircmaxell added 3 commits October 5, 2012 15:31

Fix some double free issues, and more cleanup work

4a7d18c

Fix issue with possible memory leak

25b2d36

Really fix leaks, add test cases to prove it...

1751d5f

ircmaxell added 3 commits October 6, 2012 12:33

fix allocation and copy issue

76e83f7

Clean up unreported memory leak by switching to zval_ptr_dtor

37b2207

Refactor to using a stack based zval instead of dynamic allocation

0bc9ca3

php-pulls merged commit 0bc9ca3 into php:master Oct 16, 2012

dunglas mentioned this pull request Sep 17, 2023

Segmentation fault when building Symfony cache on Alpine #12234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR for Password Hash RFC #191

PR for Password Hash RFC #191

ircmaxell commented Sep 12, 2012

ircmaxell commented Sep 13, 2012

nikic Sep 17, 2012

ircmaxell Sep 17, 2012

nikic Sep 17, 2012

nikic Sep 17, 2012

nikic Sep 17, 2012

nikic Sep 17, 2012

ircmaxell Oct 6, 2012

nikic commented Oct 6, 2012

nikic commented Oct 6, 2012

ircmaxell commented Oct 6, 2012

nikic commented Oct 6, 2012

PR for Password Hash RFC #191

PR for Password Hash RFC #191

Conversation

ircmaxell commented Sep 12, 2012

ircmaxell commented Sep 13, 2012

nikic Sep 17, 2012

Choose a reason for hiding this comment

ircmaxell Sep 17, 2012

Choose a reason for hiding this comment

nikic Sep 17, 2012

Choose a reason for hiding this comment

nikic Sep 17, 2012

Choose a reason for hiding this comment

nikic Sep 17, 2012

Choose a reason for hiding this comment

nikic Sep 17, 2012

Choose a reason for hiding this comment

ircmaxell Oct 6, 2012

Choose a reason for hiding this comment

nikic commented Oct 6, 2012

nikic commented Oct 6, 2012

ircmaxell commented Oct 6, 2012

nikic commented Oct 6, 2012