Skip to content

Promote warnings to exceptions in password_*() functions #4994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Promote warnings to exceptions in password_*() functions #4994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ext/sodium/sodium_pwhash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,20 @@ 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;
}
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;
Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}

Expand Down
2 changes: 1 addition & 1 deletion ext/standard/basic_functions_arginfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
28 changes: 15 additions & 13 deletions ext/standard/password.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)
{
Expand All @@ -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);
Expand Down
23 changes: 13 additions & 10 deletions ext/standard/tests/password/password_bcrypt_errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@ Test error operation of password_hash() with bcrypt hashing
--FILE--
<?php
//-=-=-=-
try {
password_hash("foo", PASSWORD_BCRYPT, array("cost" => 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
10 changes: 6 additions & 4 deletions ext/standard/tests/password/password_hash_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
58 changes: 38 additions & 20 deletions ext/standard/tests/password/password_hash_error_argon2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,46 @@ if (!defined('PASSWORD_ARGON2ID')) die('skip password_hash not built with Argon2
?>
--FILE--
<?php
var_dump(password_hash('test', PASSWORD_ARGON2I, ['memory_cost' => 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