Skip to content

Make password salt generation algo-agnostic and isolate bcrypt-specific logic #24281

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

Open
narfbg wants to merge 5 commits into
base: PHP-8.4
Choose a base branch
from
+52 −73
Open

Make password salt generation algo-agnostic and isolate bcrypt-specific logic #24281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dc37a9
Make password salt generation algo-agnostic and isolate bcrypt-specif…
narfbg Oct 12, 2026
5a68cd9
Fix overflow bug, drop an unnecessary zend_string + CS fixes
narfbg Oct 13, 2026
b04d59f
Drop php_password_bcrypt_salt_to64() entirely
narfbg Oct 14, 2026
9bcde6f
Drop php_password_get_salt() helper
narfbg Oct 14, 2026
c06fec8
Further simplify salt handling
narfbg Oct 14, 2026
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
121 changes: 48 additions & 73 deletions ext/standard/password.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,71 +46,24 @@ void php_password_algo_unregister(const char *ident) {
zend_hash_str_del(&php_password_algos, ident, strlen(ident));
}

static int php_password_salt_to64(const char *str, const size_t str_len, const size_t out_len, char *ret) /* {{{ */
static bool php_password_make_salt(char *salt, size_t length) /* {{{ */
{
size_t pos = 0;
zend_string *buffer;
if ((int) str_len < 0) {
return FAILURE;
}
buffer = php_base64_encode((unsigned char*) str, str_len);
if (ZSTR_LEN(buffer) < out_len) {
/* Too short of an encoded string generated */
zend_string_release_ex(buffer, 0);
return FAILURE;
}
for (pos = 0; pos < out_len; pos++) {
if (ZSTR_VAL(buffer)[pos] == '+') {
ret[pos] = '.';
} else if (ZSTR_VAL(buffer)[pos] == '=') {
zend_string_free(buffer);
return FAILURE;
} else {
ret[pos] = ZSTR_VAL(buffer)[pos];
}
}
zend_string_free(buffer);
return SUCCESS;
}
/* }}} */

static zend_string* php_password_make_salt(size_t length) /* {{{ */
{
zend_string *ret, *buffer;

if (length > (INT_MAX / 3)) {
/* 64 ought to be enough for anybody */
if (length > 64) {
zend_value_error("Length is too large to safely generate");
return NULL;
return false;
}

buffer = zend_string_alloc(length * 3 / 4 + 1, 0);
if (FAILURE == php_random_bytes_throw(ZSTR_VAL(buffer), ZSTR_LEN(buffer))) {
if (FAILURE == php_random_bytes_throw(salt, length)) {
zend_value_error("Unable to generate salt");
zend_string_release_ex(buffer, 0);
return NULL;
return false;
}

ret = zend_string_alloc(length, 0);
if (php_password_salt_to64(ZSTR_VAL(buffer), ZSTR_LEN(buffer), length, ZSTR_VAL(ret)) == FAILURE) {
zend_value_error("Generated salt too short");
zend_string_release_ex(buffer, 0);
zend_string_release_ex(ret, 0);
return NULL;
}
zend_string_release_ex(buffer, 0);
ZSTR_VAL(ret)[length] = 0;
return ret;
salt[length] = 0;
return true;
}
/* }}} */

static zend_string* php_password_get_salt(zval *unused_, size_t required_salt_len, HashTable *options) {
if (options && zend_hash_str_exists(options, "salt", sizeof("salt") - 1)) {
php_error_docref(NULL, E_WARNING, "The \"salt\" option has been ignored, since providing a custom salt is no longer supported");
}

return php_password_make_salt(required_salt_len);
}

/* bcrypt implementation */

static bool php_password_bcrypt_get_cost(const zend_string *hash, zend_long *cost) {
Expand Down Expand Up @@ -185,9 +138,9 @@ static bool php_password_bcrypt_verify(const zend_string *password, const zend_s
}

static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_array *options) {
char hash_format[10];
size_t hash_format_len;
zend_string *result, *hash, *salt;
char hash[30];
char salt[17];
zend_string *result, *salt64;
zval *zcost;
zend_long cost = PHP_PASSWORD_BCRYPT_COST;

Expand All @@ -205,25 +158,42 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a
return NULL;
}

hash_format_len = snprintf(hash_format, sizeof(hash_format), "$2y$%02" ZEND_LONG_FMT_SPEC "$", cost);
if (!(salt = php_password_get_salt(NULL, Z_UL(22), options))) {
if (!php_password_make_salt(salt, 16)) {
return NULL;
}
ZSTR_VAL(salt)[ZSTR_LEN(salt)] = 0;

hash = zend_string_concat2(hash_format, hash_format_len, ZSTR_VAL(salt), ZSTR_LEN(salt));
salt64 = php_base64_encode_ex((const unsigned char*) salt, 16, PHP_BASE64_NO_PADDING);
ZEND_ASSERT(salt64 != NULL && ZSTR_LEN(salt64) == 22);

char *ptr = ZSTR_VAL(salt64);
for (size_t i = 0, len = ZSTR_LEN(salt64)-1; i < len; i++) {
if (ptr[i] == '+') {
ptr[i] = '.';
}
}
/* Bcrypt handles the last byte differently.
* It's more complicated than this, but easy with the fixed length. */
switch (ptr[21]) {
case 'A': ptr[21] = '.'; break;
case 'Q': ptr[21] = 'O'; break;
case 'g': ptr[21] = 'e'; break;
case 'w': ptr[21] = 'u'; break;
default:
zend_string_free(salt64);
return NULL;
}

zend_string_release_ex(salt, 0);
snprintf(hash, sizeof(hash), "$2y$%02" ZEND_LONG_FMT_SPEC "$%.22s", cost, ZSTR_VAL(salt64));
ZEND_ASSERT(hash[28] == ptr[21] && hash[29] == 0);
zend_string_free(salt64);

/* This cast is safe, since both values are defined here in code and cannot overflow */
result = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1);
zend_string_release_ex(hash, 0);

result = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), hash, (int)sizeof(hash)-1, 1);
if (!result) {
return NULL;
}

if (ZSTR_LEN(result) < 13) {
if (ZSTR_LEN(result) != 60) {
zend_string_free(result);
return NULL;
}
Expand Down Expand Up @@ -310,7 +280,9 @@ static bool php_password_argon2_needs_rehash(const zend_string *hash, zend_array

static zend_string *php_password_argon2_hash(const zend_string *password, zend_array *options, argon2_type type) {
zval *option_buffer;
zend_string *salt, *out, *encoded;
zend_string *out, *encoded;
char salt[17];
size_t salt_len = sizeof(salt)-1;
size_t time_cost = PHP_PASSWORD_ARGON2_TIME_COST;
size_t memory_cost = PHP_PASSWORD_ARGON2_MEMORY_COST;
size_t threads = PHP_PASSWORD_ARGON2_THREADS;
Expand Down Expand Up @@ -344,7 +316,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a
return NULL;
}

if (!(salt = php_password_get_salt(NULL, Z_UL(16), options))) {
if (!php_password_make_salt(salt, salt_len)) {
return NULL;
}

Expand All @@ -353,7 +325,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a
time_cost,
memory_cost,
threads,
(uint32_t)ZSTR_LEN(salt),
(uint32_t)salt_len,
ZSTR_LEN(out),
type
);
Expand All @@ -365,8 +337,8 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a
threads,
ZSTR_VAL(password),
ZSTR_LEN(password),
ZSTR_VAL(salt),
ZSTR_LEN(salt),
salt,
salt_len,
ZSTR_VAL(out),
ZSTR_LEN(out),
ZSTR_VAL(encoded),
Expand All @@ -376,7 +348,6 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a
);

zend_string_release_ex(out, 0);
zend_string_release_ex(salt, 0);

if (status != ARGON2_OK) {
zend_string_efree(encoded);
Expand Down Expand Up @@ -657,6 +628,10 @@ PHP_FUNCTION(password_hash)
RETURN_THROWS();
}

if (options && zend_hash_str_exists(options, "salt", sizeof("salt") - 1)) {
php_error_docref(NULL, E_WARNING, "The \"salt\" option has been ignored, since providing a custom salt is no longer supported");
}

digest = algo->hash(password, options);
if (!digest) {
if (!EG(exception)) {
Expand Down
4 changes: 4 additions & 0 deletions ext/standard/tests/password/password_hash_argon2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ foreach ($algos as $algo) {
var_dump(password_get_info($hash)['algo']);
}

// Bug where libargon2 implementation unintentionally used less entropy through already base64-encoded salt
var_dump(strspn(base64_decode(explode("$", $hash)[4]), "./ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789") !== 16);

echo "OK!";
?>
--EXPECT--
Expand All @@ -39,4 +42,5 @@ bool(true)
string(8) "argon2id"
bool(true)
string(8) "argon2id"
bool(true)
OK!
Loading