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 1 commit into
base: PHP-8.4
Choose a base branch
from
+61 −54
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
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
111 changes: 57 additions & 54 deletions ext/standard/password.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
/* }}} */

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

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