From 2dc37a9a45eb80ed481eeaac8dee0033fea388e5 Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Mon, 12 Oct 2026 21:16:02 +0300 Subject: [PATCH 1/5] Make password salt generation algo-agnostic and isolate bcrypt-specific logic - Fixes a bug where the libargon2 implementation uses less entropy than intended (still safe, but below baseline) - Aligned bcrypt salt encoding more closely to bcrypt's variant (previously, it relied on php_crypt() doing the error correction) - Stricter bcrypt sanity checks, and slightly optimized code --- ext/standard/password.c | 111 +++++++++--------- .../tests/password/password_hash_argon2.phpt | 4 + 2 files changed, 61 insertions(+), 54 deletions(-) diff --git a/ext/standard/password.c b/ext/standard/password.c index 9d6bd6f2003d..f70f49690686 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -46,60 +46,25 @@ 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) /* {{{ */ -{ - 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; + zend_string *salt; - 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; } - buffer = zend_string_alloc(length * 3 / 4 + 1, 0); - if (FAILURE == php_random_bytes_throw(ZSTR_VAL(buffer), ZSTR_LEN(buffer))) { + salt = zend_string_alloc(length, 0); + if (FAILURE == php_random_bytes_throw(ZSTR_VAL(salt), ZSTR_LEN(salt))) { zend_value_error("Unable to generate salt"); - zend_string_release_ex(buffer, 0); + zend_string_free(salt); return NULL; } - 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; + ZSTR_VAL(salt)[length] = 0; + return salt; } /* }}} */ @@ -113,6 +78,44 @@ static zend_string* php_password_get_salt(zval *unused_, size_t required_salt_le /* bcrypt implementation */ +static bool php_password_bcrypt_salt_to64(const char *in, const size_t in_len, const size_t out_len, char *out) /* {{{ */ +{ + if (in_len != 16 || out_len != 22) { + zend_value_error("Unexpected bcrypt salt size"); + return false; + } + + zend_string *buffer = php_base64_encode_ex((const unsigned char*) in, in_len, PHP_BASE64_NO_PADDING); + if (!buffer) { + return false; + } + + if (ZSTR_LEN(buffer) != out_len || ZSTR_VAL(buffer)[out_len] == '=') { + zend_string_free(buffer); + return false; + } + + char *str = ZSTR_VAL(buffer); + for (size_t i = 0; i < out_len; i++) { + if (str[i] == '+') out[i] = '.'; + else out[i] = str[i]; + } + zend_string_free(buffer); + + // Bcrypt handles the last byte differently. + // It's more complicated than this, but easy with the fixed length. + switch (out[21]) { + case 'A': out[21] = '.'; break; + case 'Q': out[21] = 'O'; break; + case 'g': out[21] = 'e'; break; + case 'w': out[21] = 'u'; break; + default: break; + } + + return true; +} +/* }}} */ + static bool php_password_bcrypt_get_cost(const zend_string *hash, zend_long *cost) { const char *h = ZSTR_VAL(hash); @@ -185,8 +188,7 @@ 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; + char salt64[22]; zend_string *result, *hash, *salt; zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; @@ -205,25 +207,26 @@ 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 (!(salt = php_password_get_salt(NULL, Z_UL(16), options))) { return NULL; } - ZSTR_VAL(salt)[ZSTR_LEN(salt)] = 0; - hash = zend_string_concat2(hash_format, hash_format_len, ZSTR_VAL(salt), ZSTR_LEN(salt)); + if (!php_password_bcrypt_salt_to64(ZSTR_VAL(salt), ZSTR_LEN(salt), sizeof(salt64), salt64)) { + zend_string_free(salt); + return NULL; + } - zend_string_release_ex(salt, 0); + zend_string_free(salt); + hash = strpprintf(0, "$2y$%02" ZEND_LONG_FMT_SPEC "$%s", cost, salt64); - /* This cast is safe, since both values are defined here in code and cannot overflow */ + // 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); - + zend_string_free(hash); if (!result) { return NULL; } - if (ZSTR_LEN(result) < 13) { + if (ZSTR_LEN(result) != 60) { zend_string_free(result); return NULL; } diff --git a/ext/standard/tests/password/password_hash_argon2.phpt b/ext/standard/tests/password/password_hash_argon2.phpt index b179ee202318..43c7a9e236bc 100644 --- a/ext/standard/tests/password/password_hash_argon2.phpt +++ b/ext/standard/tests/password/password_hash_argon2.phpt @@ -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-- @@ -39,4 +42,5 @@ bool(true) string(8) "argon2id" bool(true) string(8) "argon2id" +bool(true) OK! From 5a68cd9962e5b90ee469387e5074578721a45c9f Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Tue, 13 Oct 2026 15:35:44 +0300 Subject: [PATCH 2/5] Fix overflow bug, drop an unnecessary zend_string + CS fixes --- ext/standard/password.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/ext/standard/password.c b/ext/standard/password.c index f70f49690686..fc8a97392172 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -50,7 +50,7 @@ static zend_string* php_password_make_salt(size_t length) /* {{{ */ { zend_string *salt; - // 64 ought to be enough for anybody + /* 64 ought to be enough for anybody */ if (length > 64) { zend_value_error("Length is too large to safely generate"); return NULL; @@ -97,13 +97,13 @@ static bool php_password_bcrypt_salt_to64(const char *in, const size_t in_len, c char *str = ZSTR_VAL(buffer); for (size_t i = 0; i < out_len; i++) { - if (str[i] == '+') out[i] = '.'; - else out[i] = str[i]; + out[i] = (str[i] == '+') ? '.' : str[i]; } zend_string_free(buffer); - // Bcrypt handles the last byte differently. - // It's more complicated than this, but easy with the fixed length. + /* Bcrypt handles the last byte differently. + * It's more complicated than this, but easy with the fixed length. + */ switch (out[21]) { case 'A': out[21] = '.'; break; case 'Q': out[21] = 'O'; break; @@ -111,7 +111,7 @@ static bool php_password_bcrypt_salt_to64(const char *in, const size_t in_len, c case 'w': out[21] = 'u'; break; default: break; } - + out[out_len] = 0; return true; } /* }}} */ @@ -188,8 +188,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 salt64[22]; - zend_string *result, *hash, *salt; + char salt64[23]; + char hash[30]; + zend_string *result, *salt; zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; @@ -211,17 +212,18 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a return NULL; } - if (!php_password_bcrypt_salt_to64(ZSTR_VAL(salt), ZSTR_LEN(salt), sizeof(salt64), salt64)) { + if (!php_password_bcrypt_salt_to64(ZSTR_VAL(salt), ZSTR_LEN(salt), sizeof(salt64)-1, salt64)) { zend_string_free(salt); return NULL; } zend_string_free(salt); - hash = strpprintf(0, "$2y$%02" ZEND_LONG_FMT_SPEC "$%s", cost, salt64); + if (snprintf(hash, sizeof(hash), "$2y$%02" ZEND_LONG_FMT_SPEC "$%.22s", cost, salt64) != 29 || hash[29] != 0) { + return NULL; + } - // 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_free(hash); + /* 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), hash, (int)sizeof(hash)-1, 1); if (!result) { return NULL; } From b04d59fab2f27eda1c076b134079280e8c3008cf Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 14 Oct 2026 12:29:43 +0300 Subject: [PATCH 3/5] Drop php_password_bcrypt_salt_to64() entirely --- ext/standard/password.c | 71 ++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 47 deletions(-) diff --git a/ext/standard/password.c b/ext/standard/password.c index fc8a97392172..a99ef7574530 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -78,44 +78,6 @@ static zend_string* php_password_get_salt(zval *unused_, size_t required_salt_le /* bcrypt implementation */ -static bool php_password_bcrypt_salt_to64(const char *in, const size_t in_len, const size_t out_len, char *out) /* {{{ */ -{ - if (in_len != 16 || out_len != 22) { - zend_value_error("Unexpected bcrypt salt size"); - return false; - } - - zend_string *buffer = php_base64_encode_ex((const unsigned char*) in, in_len, PHP_BASE64_NO_PADDING); - if (!buffer) { - return false; - } - - if (ZSTR_LEN(buffer) != out_len || ZSTR_VAL(buffer)[out_len] == '=') { - zend_string_free(buffer); - return false; - } - - char *str = ZSTR_VAL(buffer); - for (size_t i = 0; i < out_len; i++) { - out[i] = (str[i] == '+') ? '.' : str[i]; - } - zend_string_free(buffer); - - /* Bcrypt handles the last byte differently. - * It's more complicated than this, but easy with the fixed length. - */ - switch (out[21]) { - case 'A': out[21] = '.'; break; - case 'Q': out[21] = 'O'; break; - case 'g': out[21] = 'e'; break; - case 'w': out[21] = 'u'; break; - default: break; - } - out[out_len] = 0; - return true; -} -/* }}} */ - static bool php_password_bcrypt_get_cost(const zend_string *hash, zend_long *cost) { const char *h = ZSTR_VAL(hash); @@ -188,9 +150,8 @@ 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 salt64[23]; char hash[30]; - zend_string *result, *salt; + zend_string *result, *salt, *salt64; zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; @@ -212,15 +173,31 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a return NULL; } - if (!php_password_bcrypt_salt_to64(ZSTR_VAL(salt), ZSTR_LEN(salt), sizeof(salt64)-1, salt64)) { - zend_string_free(salt); - return NULL; - } - + salt64 = php_base64_encode_ex((const unsigned char*) ZSTR_VAL(salt), ZSTR_LEN(salt), PHP_BASE64_NO_PADDING); zend_string_free(salt); - if (snprintf(hash, sizeof(hash), "$2y$%02" ZEND_LONG_FMT_SPEC "$%.22s", cost, salt64) != 29 || hash[29] != 0) { - return NULL; + 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; + } + + 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), hash, (int)sizeof(hash)-1, 1); From 9bcde6f87c9e13e2c2fe9bffda0709dfa7e86754 Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 14 Oct 2026 15:44:08 +0300 Subject: [PATCH 4/5] Drop php_password_get_salt() helper Originally designed with only bcrypt in mind and contributed to the argon2 salt bug. Today it only exists to emit the ignored salt warning and is only complicating things. Moved the warning trigger in PHP_FUNCTION(password_hash) to avoid a breaking change, since argon2 inherited it even though it never accepted a user-provided salt. This is a bug on its own, but it can be argued both ways so being safe with it. I reckon the warning should eventually be dropped entirely in a major release. --- ext/standard/password.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/ext/standard/password.c b/ext/standard/password.c index a99ef7574530..4091696a274a 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -68,14 +68,6 @@ static zend_string* php_password_make_salt(size_t length) /* {{{ */ } /* }}} */ -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) { @@ -169,7 +161,7 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a return NULL; } - if (!(salt = php_password_get_salt(NULL, Z_UL(16), options))) { + if (!(salt = php_password_make_salt(16))) { return NULL; } @@ -326,7 +318,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 (!(salt = php_password_make_salt(16))) { return NULL; } @@ -639,6 +631,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)) { From c06fec8569ea511db0e6374296a7ef27236e829a Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 14 Oct 2026 16:17:17 +0300 Subject: [PATCH 5/5] Further simplify salt handling --- ext/standard/password.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/ext/standard/password.c b/ext/standard/password.c index 4091696a274a..09b358b6bdde 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -46,25 +46,21 @@ void php_password_algo_unregister(const char *ident) { zend_hash_str_del(&php_password_algos, ident, strlen(ident)); } -static zend_string* php_password_make_salt(size_t length) /* {{{ */ +static bool php_password_make_salt(char *salt, size_t length) /* {{{ */ { - zend_string *salt; - /* 64 ought to be enough for anybody */ if (length > 64) { zend_value_error("Length is too large to safely generate"); - return NULL; + return false; } - salt = zend_string_alloc(length, 0); - if (FAILURE == php_random_bytes_throw(ZSTR_VAL(salt), ZSTR_LEN(salt))) { + if (FAILURE == php_random_bytes_throw(salt, length)) { zend_value_error("Unable to generate salt"); - zend_string_free(salt); - return NULL; + return false; } - ZSTR_VAL(salt)[length] = 0; - return salt; + salt[length] = 0; + return true; } /* }}} */ @@ -143,7 +139,8 @@ 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[30]; - zend_string *result, *salt, *salt64; + char salt[17]; + zend_string *result, *salt64; zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; @@ -161,12 +158,11 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a return NULL; } - if (!(salt = php_password_make_salt(16))) { + if (!php_password_make_salt(salt, 16)) { return NULL; } - salt64 = php_base64_encode_ex((const unsigned char*) ZSTR_VAL(salt), ZSTR_LEN(salt), PHP_BASE64_NO_PADDING); - zend_string_free(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); @@ -284,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; @@ -318,7 +316,7 @@ static zend_string *php_password_argon2_hash(const zend_string *password, zend_a return NULL; } - if (!(salt = php_password_make_salt(16))) { + if (!php_password_make_salt(salt, salt_len)) { return NULL; } @@ -327,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 ); @@ -339,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), @@ -350,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);