From 37c1171451d003a726eab0c51eb70343ad231585 Mon Sep 17 00:00:00 2001 From: =?utf8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Wed, 20 Nov 2019 02:21:06 +0100 Subject: [PATCH] Promote warnings to exceptions in password_*() functions --- ext/sodium/sodium_pwhash.c | 10 ++-- ext/standard/basic_functions.stub.php | 2 +- ext/standard/basic_functions_arginfo.h | 2 +- ext/standard/password.c | 28 ++++----- .../password/password_bcrypt_errors.phpt | 23 ++++---- .../tests/password/password_hash_error.phpt | 10 ++-- .../password/password_hash_error_argon2.phpt | 58 ++++++++++++------- 7 files changed, 79 insertions(+), 54 deletions(-) diff --git a/ext/sodium/sodium_pwhash.c b/ext/sodium/sodium_pwhash.c index 3ff304c452..e58a9514cc 100644 --- a/ext/sodium/sodium_pwhash.c +++ b/ext/sodium/sodium_pwhash.c @@ -50,7 +50,7 @@ static inline int get_options(zend_array *options, size_t *memlimit, size_t *ops zend_long smemlimit = zval_get_long(opt); if ((smemlimit < 0) || (smemlimit < crypto_pwhash_MEMLIMIT_MIN >> 10) || (smemlimit > (crypto_pwhash_MEMLIMIT_MAX >> 10))) { - php_error_docref(NULL, E_WARNING, "Memory cost is outside of allowed memory range"); + zend_value_error("Memory cost is outside of allowed memory range"); return FAILURE; } *memlimit = smemlimit << 10; @@ -58,12 +58,12 @@ static inline int get_options(zend_array *options, size_t *memlimit, size_t *ops if ((opt = zend_hash_str_find(options, "time_cost", strlen("time_cost")))) { *opslimit = zval_get_long(opt); if ((*opslimit < crypto_pwhash_OPSLIMIT_MIN) || (*opslimit > crypto_pwhash_OPSLIMIT_MAX)) { - php_error_docref(NULL, E_WARNING, "Time cost is outside of allowed time range"); + zend_value_error("Time cost is outside of allowed time range"); return FAILURE; } } if ((opt = zend_hash_str_find(options, "threads", strlen("threads"))) && (zval_get_long(opt) != 1)) { - php_error_docref(NULL, E_WARNING, "A thread value other than 1 is not supported by this implementation"); + zend_value_error("A thread value other than 1 is not supported by this implementation"); return FAILURE; } return SUCCESS; @@ -74,7 +74,7 @@ static zend_string *php_sodium_argon2_hash(const zend_string *password, zend_arr zend_string *ret; if ((ZSTR_LEN(password) >= 0xffffffff)) { - php_error_docref(NULL, E_WARNING, "Password is too long"); + zend_value_error("Password is too long"); return NULL; } @@ -84,7 +84,7 @@ static zend_string *php_sodium_argon2_hash(const zend_string *password, zend_arr ret = zend_string_alloc(crypto_pwhash_STRBYTES - 1, 0); if (crypto_pwhash_str_alg(ZSTR_VAL(ret), ZSTR_VAL(password), ZSTR_LEN(password), opslimit, memlimit, alg)) { - php_error_docref(NULL, E_WARNING, "Unexpected failure hashing password"); + zend_value_error("Unexpected failure hashing password"); zend_string_release(ret); return NULL; } diff --git a/ext/standard/basic_functions.stub.php b/ext/standard/basic_functions.stub.php index 75eeaffedf..d05b155dab 100755 --- a/ext/standard/basic_functions.stub.php +++ b/ext/standard/basic_functions.stub.php @@ -1131,7 +1131,7 @@ function unpack(string $format, string $data, int $offset = 0): array|false {} function password_get_info(string $hash): ?array {} -function password_hash(string $password, $algo, array $options = []): ?string {} +function password_hash(string $password, $algo, array $options = []): string {} function password_needs_rehash(string $hash, $algo, array $options = []): bool {} diff --git a/ext/standard/basic_functions_arginfo.h b/ext/standard/basic_functions_arginfo.h index 0ffd3e7ee6..2fa47cb704 100755 --- a/ext/standard/basic_functions_arginfo.h +++ b/ext/standard/basic_functions_arginfo.h @@ -1741,7 +1741,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_password_get_info, 0, 1, IS_ARRA ZEND_ARG_TYPE_INFO(0, hash, IS_STRING, 0) ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_password_hash, 0, 2, IS_STRING, 1) +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_password_hash, 0, 2, IS_STRING, 0) ZEND_ARG_TYPE_INFO(0, password, IS_STRING, 0) ZEND_ARG_INFO(0, algo) ZEND_ARG_TYPE_INFO(0, options, IS_ARRAY, 0) diff --git a/ext/standard/password.c b/ext/standard/password.c index 17896e77ee..56908b4f8a 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -83,20 +83,20 @@ static zend_string* php_password_make_salt(size_t length) /* {{{ */ zend_string *ret, *buffer; if (length > (INT_MAX / 3)) { - php_error_docref(NULL, E_WARNING, "Length is too large to safely generate"); + zend_value_error("Length is too large to safely generate"); return NULL; } buffer = zend_string_alloc(length * 3 / 4 + 1, 0); if (FAILURE == php_random_bytes_silent(ZSTR_VAL(buffer), ZSTR_LEN(buffer))) { - php_error_docref(NULL, E_WARNING, "Unable to generate salt"); + zend_value_error("Unable to generate salt"); zend_string_release_ex(buffer, 0); return NULL; } ret = zend_string_alloc(length, 0); if (php_password_salt_to64(ZSTR_VAL(buffer), ZSTR_LEN(buffer), length, ZSTR_VAL(ret)) == FAILURE) { - php_error_docref(NULL, E_WARNING, "Generated salt too short"); + zend_value_error("Generated salt too short"); zend_string_release_ex(buffer, 0); zend_string_release_ex(ret, 0); return NULL; @@ -193,7 +193,7 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a } if (cost < 4 || cost > 31) { - php_error_docref(NULL, E_WARNING, "Invalid bcrypt cost parameter specified: " ZEND_LONG_FMT, cost); + zend_value_error("Invalid bcrypt cost parameter specified: " ZEND_LONG_FMT, cost); return NULL; } @@ -316,7 +316,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a } if (memory_cost > ARGON2_MAX_MEMORY || memory_cost < ARGON2_MIN_MEMORY) { - php_error_docref(NULL, E_WARNING, "Memory cost is outside of allowed memory range"); + zend_value_error("Memory cost is outside of allowed memory range"); return NULL; } @@ -325,7 +325,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a } if (time_cost > ARGON2_MAX_TIME || time_cost < ARGON2_MIN_TIME) { - php_error_docref(NULL, E_WARNING, "Time cost is outside of allowed time range"); + zend_value_error("Time cost is outside of allowed time range"); return NULL; } @@ -334,7 +334,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a } if (threads > ARGON2_MAX_LANES || threads == 0) { - php_error_docref(NULL, E_WARNING, "Invalid number of threads"); + zend_value_error("Invalid number of threads"); return NULL; } @@ -374,7 +374,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a if (status != ARGON2_OK) { zend_string_efree(encoded); - php_error_docref(NULL, E_WARNING, "%s", argon2_error_message(status)); + zend_value_error("%s", argon2_error_message(status)); return NULL; } @@ -650,7 +650,7 @@ PHP_FUNCTION(password_verify) } /* }}} */ -/* {{{ proto string|null password_hash(string password, mixed algo[, array options = array()]) +/* {{{ proto string password_hash(string password, mixed algo[, array options = array()]) Hash a password */ PHP_FUNCTION(password_hash) { @@ -669,15 +669,17 @@ PHP_FUNCTION(password_hash) algo = php_password_algo_find_zval(zalgo); if (!algo) { zend_string *algostr = zval_get_string(zalgo); - php_error_docref(NULL, E_WARNING, "Unknown password hashing algorithm: %s", ZSTR_VAL(algostr)); + zend_value_error("Unknown password hashing algorithm: %s", ZSTR_VAL(algostr)); zend_string_release(algostr); - RETURN_NULL(); + return; } digest = algo->hash(password, options); if (!digest) { - /* algo->hash should have raised an error. */ - RETURN_NULL(); + if (!EG(exception)) { + zend_throw_error(NULL, "Password hashing failed for unknown reason"); + } + return; } RETURN_NEW_STR(digest); diff --git a/ext/standard/tests/password/password_bcrypt_errors.phpt b/ext/standard/tests/password/password_bcrypt_errors.phpt index 64496744cb..10c3483f5a 100644 --- a/ext/standard/tests/password/password_bcrypt_errors.phpt +++ b/ext/standard/tests/password/password_bcrypt_errors.phpt @@ -3,15 +3,18 @@ Test error operation of password_hash() with bcrypt hashing --FILE-- 3)); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -var_dump(password_hash("foo", PASSWORD_BCRYPT, array("cost" => 3))); - -var_dump(password_hash("foo", PASSWORD_BCRYPT, array("cost" => 32))); - +try { + var_dump(password_hash("foo", PASSWORD_BCRYPT, array("cost" => 32))); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} ?> ---EXPECTF-- -Warning: password_hash(): Invalid bcrypt cost parameter specified: 3 in %s on line %d -NULL - -Warning: password_hash(): Invalid bcrypt cost parameter specified: 32 in %s on line %d -NULL +--EXPECT-- +Invalid bcrypt cost parameter specified: 3 +Invalid bcrypt cost parameter specified: 32 diff --git a/ext/standard/tests/password/password_hash_error.phpt b/ext/standard/tests/password/password_hash_error.phpt index cb50654905..a0d8c6461e 100644 --- a/ext/standard/tests/password/password_hash_error.phpt +++ b/ext/standard/tests/password/password_hash_error.phpt @@ -10,7 +10,11 @@ try { echo $e->getMessage(), "\n"; } -var_dump(password_hash("foo", array())); +try { + password_hash("foo", array()); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} try { var_dump(password_hash("foo", 19, new StdClass)); @@ -35,9 +39,7 @@ try { password_hash() expects at least 2 parameters, 1 given Warning: Array to string conversion in %s on line %d - -Warning: password_hash(): Unknown password hashing algorithm: Array in %s on line %d -NULL +Unknown password hashing algorithm: Array password_hash() expects parameter 3 to be array, object given password_hash() expects parameter 3 to be array, string given password_hash() expects parameter 1 to be string, array given diff --git a/ext/standard/tests/password/password_hash_error_argon2.phpt b/ext/standard/tests/password/password_hash_error_argon2.phpt index 2ea6b93627..96f029aba9 100644 --- a/ext/standard/tests/password/password_hash_error_argon2.phpt +++ b/ext/standard/tests/password/password_hash_error_argon2.phpt @@ -7,28 +7,46 @@ if (!defined('PASSWORD_ARGON2ID')) die('skip password_hash not built with Argon2 ?> --FILE-- 0])); -var_dump(password_hash('test', PASSWORD_ARGON2I, ['time_cost' => 0])); -var_dump(password_hash('test', PASSWORD_ARGON2I, ['threads' => 0])); -var_dump(password_hash('test', PASSWORD_ARGON2ID, ['memory_cost' => 0])); -var_dump(password_hash('test', PASSWORD_ARGON2ID, ['time_cost' => 0])); -var_dump(password_hash('test', PASSWORD_ARGON2ID, ['threads' => 0])); -?> ---EXPECTF-- -Warning: password_hash(): Memory cost is outside of allowed memory range in %s on line %d -NULL +try { + password_hash('test', PASSWORD_ARGON2I, ['memory_cost' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -Warning: password_hash(): Time cost is outside of allowed time range in %s on line %d -NULL +try { + password_hash('test', PASSWORD_ARGON2I, ['time_cost' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -Warning: password_hash(): %sthread%s -NULL +try { + password_hash('test', PASSWORD_ARGON2I, ['threads' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -Warning: password_hash(): Memory cost is outside of allowed memory range in %s on line %d -NULL +try { + password_hash('test', PASSWORD_ARGON2ID, ['memory_cost' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -Warning: password_hash(): Time cost is outside of allowed time range in %s on line %d -NULL +try { + password_hash('test', PASSWORD_ARGON2ID, ['time_cost' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -Warning: password_hash(): %sthread%s -NULL +try { + password_hash('test', PASSWORD_ARGON2ID, ['threads' => 0]); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} +?> +--EXPECT-- +Memory cost is outside of allowed memory range +Time cost is outside of allowed time range +Invalid number of threads +Memory cost is outside of allowed memory range +Time cost is outside of allowed time range +Invalid number of threads -- 2.40.0