Skip to content

Commit e93f089

Browse files
sgolemonsgolemon
committed
Refactor password_hash()
Pull salt generation out to a helper. Merge options/hash into single switch. Restore php_error->php_error_docref from last diff. (Error messages matter)
1 parent 7165e28 commit e93f089

File tree

1 file changed

+120
-119
lines changed

1 file changed

+120
-119
lines changed

‎ext/standard/password.c

Lines changed: 120 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ static php_password_algo php_password_determine_algo(const zend_string *hash)
7979
const size_t len = ZSTR_LEN(hash);
8080
if (len == 60 && h[0] == '$' && h[1] == '2' && h[2] == 'y') {
8181
return PHP_PASSWORD_BCRYPT;
82-
}
82+
}
8383
#if HAVE_ARGON2LIB
8484
if (len >= sizeof("$argon2i$")-1 && !memcmp(h, "$argon2i$", sizeof("$argon2i$")-1)) {
8585
return PHP_PASSWORD_ARGON2I;
@@ -135,20 +135,20 @@ static zend_string* php_password_make_salt(size_t length) /* {{{ */
135135
zend_string *ret, *buffer;
136136

137137
if (length > (INT_MAX / 3)) {
138-
php_error(E_WARNING, "Length is too large to safely generate");
138+
php_error_docref(NULL, E_WARNING, "Length is too large to safely generate");
139139
return NULL;
140140
}
141141

142142
buffer = zend_string_alloc(length * 3 / 4 + 1, 0);
143143
if (FAILURE == php_random_bytes_silent(ZSTR_VAL(buffer), ZSTR_LEN(buffer))) {
144-
php_error(E_WARNING, "Unable to generate salt");
144+
php_error_docref(NULL, E_WARNING, "Unable to generate salt");
145145
zend_string_release(buffer);
146146
return NULL;
147147
}
148148

149149
ret = zend_string_alloc(length, 0);
150150
if (php_password_salt_to64(ZSTR_VAL(buffer), ZSTR_LEN(buffer), length, ZSTR_VAL(ret)) == FAILURE) {
151-
php_error(E_WARNING, "Generated salt too short");
151+
php_error_docref(NULL, E_WARNING, "Generated salt too short");
152152
zend_string_release(buffer);
153153
zend_string_release(ret);
154154
return NULL;
@@ -341,22 +341,81 @@ PHP_FUNCTION(password_verify)
341341
}
342342
/* }}} */
343343

344+
static zend_string* php_password_get_salt(zval *return_value, int required_salt_len, HashTable *options) {
345+
zend_string *buffer;
346+
zval *option_buffer;
347+
348+
if (!options || !(option_buffer = zend_hash_str_find(options, "salt", sizeof("salt") - 1))) {
349+
buffer = php_password_make_salt(required_salt_len);
350+
if (!buffer) {
351+
RETVAL_FALSE;
352+
}
353+
return buffer;
354+
}
355+
356+
php_error_docref(NULL, E_DEPRECATED, "Use of the 'salt' option to password_hash is deprecated");
357+
358+
switch (Z_TYPE_P(option_buffer)) {
359+
case IS_STRING:
360+
buffer = zend_string_copy(Z_STR_P(option_buffer));
361+
break;
362+
case IS_LONG:
363+
case IS_DOUBLE:
364+
case IS_OBJECT:
365+
buffer = zval_get_string(option_buffer);
366+
break;
367+
case IS_FALSE:
368+
case IS_TRUE:
369+
case IS_NULL:
370+
case IS_RESOURCE:
371+
case IS_ARRAY:
372+
default:
373+
php_error_docref(NULL, E_WARNING, "Non-string salt parameter supplied");
374+
return NULL;
375+
}
376+
377+
/* XXX all the crypt related APIs work with int for string length.
378+
That should be revised for size_t and then we maybe don't require
379+
the > INT_MAX check. */
380+
if (ZSTR_LEN(buffer) > INT_MAX) {
381+
php_error_docref(NULL, E_WARNING, "Supplied salt is too long");
382+
zend_string_release(buffer);
383+
return NULL;
384+
}
385+
386+
if (ZSTR_LEN(buffer) < required_salt_len) {
387+
php_error_docref(NULL, E_WARNING, "Provided salt is too short: %zd expecting %zd", ZSTR_LEN(buffer), required_salt_len);
388+
zend_string_release(buffer);
389+
return NULL;
390+
}
391+
392+
if (php_password_salt_is_alphabet(ZSTR_VAL(buffer), ZSTR_LEN(buffer)) == FAILURE) {
393+
zend_string *salt = zend_string_alloc(required_salt_len, 0);
394+
if (php_password_salt_to64(ZSTR_VAL(buffer), ZSTR_LEN(buffer), required_salt_len, ZSTR_VAL(salt)) == FAILURE) {
395+
php_error_docref(NULL, E_WARNING, "Provided salt is too short: %zd", ZSTR_LEN(buffer));
396+
zend_string_release(salt);
397+
zend_string_release(buffer);
398+
return NULL;
399+
}
400+
zend_string_release(buffer);
401+
return salt;
402+
} else {
403+
zend_string *salt = zend_string_alloc(required_salt_len, 0);
404+
memcpy(ZSTR_VAL(salt), ZSTR_VAL(buffer), required_salt_len);
405+
zend_string_release(buffer);
406+
return salt;
407+
}
408+
}
409+
344410
/* {{{ proto string password_hash(string password, int algo[, array options = array()])
345411
Hash a password */
346412
PHP_FUNCTION(password_hash)
347413
{
348-
char hash_format[8];
349-
zend_long algo = 0;
350-
zend_string *password, *salt;
351-
size_t required_salt_len = 0, hash_format_len;
352-
HashTable *options = 0;
353-
zval *option_buffer;
414+
zend_string *password;
415+
zend_long algo = PHP_PASSWORD_DEFAULT;
416+
HashTable *options = NULL;
354417

355418
#if HAVE_ARGON2LIB
356-
size_t time_cost = PHP_PASSWORD_ARGON2_TIME_COST;
357-
size_t memory_cost = PHP_PASSWORD_ARGON2_MEMORY_COST;
358-
size_t threads = PHP_PASSWORD_ARGON2_THREADS;
359-
argon2_type type = Argon2_i;
360419
#endif
361420

362421
ZEND_PARSE_PARAMETERS_START(2, 3)
@@ -369,6 +428,10 @@ PHP_FUNCTION(password_hash)
369428
switch (algo) {
370429
case PHP_PASSWORD_BCRYPT:
371430
{
431+
char hash_format[10];
432+
size_t hash_format_len;
433+
zend_string *result, *hash, *salt;
434+
zval *option_buffer;
372435
zend_long cost = PHP_PASSWORD_BCRYPT_COST;
373436

374437
if (options && (option_buffer = zend_hash_str_find(options, "cost", sizeof("cost")-1)) != NULL) {
@@ -380,14 +443,46 @@ PHP_FUNCTION(password_hash)
380443
RETURN_NULL();
381444
}
382445

383-
required_salt_len = 22;
384-
sprintf(hash_format, "$2y$%02ld$", (long) cost);
385-
hash_format_len = 7;
446+
hash_format_len = snprintf(hash_format, sizeof(hash_format), "$2y$%02ld$", (long) cost);
447+
if (!(salt = php_password_get_salt(return_value, 22, options))) {
448+
return;
449+
}
450+
ZSTR_VAL(salt)[ZSTR_LEN(salt)] = 0;
451+
452+
hash = zend_string_alloc(ZSTR_LEN(salt) + hash_format_len, 0);
453+
sprintf(ZSTR_VAL(hash), "%s%s", hash_format, ZSTR_VAL(salt));
454+
ZSTR_VAL(hash)[hash_format_len + ZSTR_LEN(salt)] = 0;
455+
456+
zend_string_release(salt);
457+
458+
/* This cast is safe, since both values are defined here in code and cannot overflow */
459+
result = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1);
460+
zend_string_release(hash);
461+
462+
if (!result) {
463+
RETURN_FALSE;
464+
}
465+
466+
if (ZSTR_LEN(result) < 13) {
467+
zend_string_free(result);
468+
RETURN_FALSE;
469+
}
470+
471+
RETURN_STR(result);
386472
}
387473
break;
388474
#if HAVE_ARGON2LIB
389475
case PHP_PASSWORD_ARGON2I:
390476
{
477+
zval *boption_buffer;
478+
zend_string *salt, *out, *encoded;
479+
size_t time_cost = PHP_PASSWORD_ARGON2_TIME_COST;
480+
size_t memory_cost = PHP_PASSWORD_ARGON2_MEMORY_COST;
481+
size_t threads = PHP_PASSWORD_ARGON2_THREADS;
482+
argon2_type type = Argon2_i;
483+
size_t encoded_len;
484+
int status = 0;
485+
391486
if (options && (option_buffer = zend_hash_str_find(options, "memory_cost", sizeof("memory_cost")-1)) != NULL) {
392487
memory_cost = zval_get_long(option_buffer);
393488
}
@@ -405,7 +500,7 @@ PHP_FUNCTION(password_hash)
405500
php_error_docref(NULL, E_WARNING, "Time cost is outside of allowed time range", time_cost);
406501
RETURN_NULL();
407502
}
408-
503+
409504
if (options && (option_buffer = zend_hash_str_find(options, "threads", sizeof("threads")-1)) != NULL) {
410505
threads = zval_get_long(option_buffer);
411506
}
@@ -415,106 +510,11 @@ PHP_FUNCTION(password_hash)
415510
RETURN_NULL();
416511
}
417512

418-
required_salt_len = 16;
419-
}
420-
break;
421-
#endif
422-
case PHP_PASSWORD_UNKNOWN:
423-
default:
424-
php_error_docref(NULL, E_WARNING, "Unknown password hashing algorithm: " ZEND_LONG_FMT, algo);
425-
RETURN_NULL();
426-
}
427-
428-
if (options && (option_buffer = zend_hash_str_find(options, "salt", sizeof("salt")-1)) != NULL) {
429-
zend_string *buffer;
430-
431-
php_error_docref(NULL, E_DEPRECATED, "Use of the 'salt' option to password_hash is deprecated");
432-
433-
switch (Z_TYPE_P(option_buffer)) {
434-
case IS_STRING:
435-
buffer = zend_string_copy(Z_STR_P(option_buffer));
436-
break;
437-
case IS_LONG:
438-
case IS_DOUBLE:
439-
case IS_OBJECT:
440-
buffer = zval_get_string(option_buffer);
441-
break;
442-
case IS_FALSE:
443-
case IS_TRUE:
444-
case IS_NULL:
445-
case IS_RESOURCE:
446-
case IS_ARRAY:
447-
default:
448-
php_error_docref(NULL, E_WARNING, "Non-string salt parameter supplied");
449-
RETURN_NULL();
450-
}
451-
452-
/* XXX all the crypt related APIs work with int for string length.
453-
That should be revised for size_t and then we maybe don't require
454-
the > INT_MAX check. */
455-
if (ZSTR_LEN(buffer) > INT_MAX) {
456-
php_error_docref(NULL, E_WARNING, "Supplied salt is too long");
457-
RETURN_NULL();
458-
} else if (ZSTR_LEN(buffer) < required_salt_len) {
459-
php_error_docref(NULL, E_WARNING, "Provided salt is too short: %zd expecting %zd", ZSTR_LEN(buffer), required_salt_len);
460-
zend_string_release(buffer);
461-
RETURN_NULL();
462-
} else if (php_password_salt_is_alphabet(ZSTR_VAL(buffer), ZSTR_LEN(buffer)) == FAILURE) {
463-
salt = zend_string_alloc(required_salt_len, 0);
464-
if (php_password_salt_to64(ZSTR_VAL(buffer), ZSTR_LEN(buffer), required_salt_len, ZSTR_VAL(salt)) == FAILURE) {
465-
zend_string_release(salt);
466-
php_error_docref(NULL, E_WARNING, "Provided salt is too short: %zd", ZSTR_LEN(buffer));
467-
zend_string_release(buffer);
468-
RETURN_NULL();
469-
}
470-
} else {
471-
salt = zend_string_alloc(required_salt_len, 0);
472-
memcpy(ZSTR_VAL(salt), ZSTR_VAL(buffer), required_salt_len);
473-
}
474-
zend_string_release(buffer);
475-
} else {
476-
salt = php_password_make_salt(required_salt_len);
477-
if (!salt) {
478-
RETURN_FALSE;
479-
}
480-
}
481-
482-
switch (algo) {
483-
case PHP_PASSWORD_BCRYPT:
484-
{
485-
zend_string *result, *hash;
486-
ZSTR_VAL(salt)[ZSTR_LEN(salt)] = 0;
487-
488-
hash = zend_string_alloc(ZSTR_LEN(salt) + hash_format_len, 0);
489-
sprintf(ZSTR_VAL(hash), "%s%s", hash_format, ZSTR_VAL(salt));
490-
ZSTR_VAL(hash)[hash_format_len + ZSTR_LEN(salt)] = 0;
491-
492-
zend_string_release(salt);
493-
494-
/* This cast is safe, since both values are defined here in code and cannot overflow */
495-
result = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1);
496-
zend_string_release(hash);
497-
498-
if (!result) {
499-
RETURN_FALSE;
513+
if (!(salt = php_password_get_salt(return_value, 16, options))) {
514+
return;
500515
}
501516

502-
if (ZSTR_LEN(result) < 13) {
503-
zend_string_free(result);
504-
RETURN_FALSE;
505-
}
506-
507-
RETURN_STR(result);
508-
}
509-
break;
510-
#if HAVE_ARGON2LIB
511-
case PHP_PASSWORD_ARGON2I:
512-
{
513-
size_t encoded_len;
514-
int status = 0;
515-
zend_string *out = zend_string_alloc(32, 0);
516-
zend_string *encoded;
517-
517+
out = zend_string_alloc(32, 0);
518518
encoded_len = argon2_encodedlen(
519519
time_cost,
520520
memory_cost,
@@ -527,7 +527,6 @@ PHP_FUNCTION(password_hash)
527527
);
528528

529529
encoded = zend_string_alloc(encoded_len, 0);
530-
531530
status = argon2_hash(
532531
time_cost,
533532
memory_cost,
@@ -549,7 +548,7 @@ PHP_FUNCTION(password_hash)
549548

550549
if (status != ARGON2_OK) {
551550
zend_string_free(encoded);
552-
php_error(E_WARNING, "%s", argon2_error_message(status));
551+
php_error_docref(NULL, E_WARNING, "%s", argon2_error_message(status));
553552
RETURN_FALSE;
554553
}
555554

@@ -558,8 +557,10 @@ PHP_FUNCTION(password_hash)
558557
}
559558
break;
560559
#endif
560+
case PHP_PASSWORD_UNKNOWN:
561561
default:
562-
RETURN_FALSE;
562+
php_error_docref(NULL, E_WARNING, "Unknown password hashing algorithm: " ZEND_LONG_FMT, algo);
563+
RETURN_NULL();
563564
}
564565
}
565566
/* }}} */

0 commit comments

Comments
 (0)