Skip to content

ext/standard: Validate the bcrypt cost before reading it #24203

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
iliaal wants to merge 1 commit into from
+71 −11
Closed

ext/standard: Validate the bcrypt cost before reading it #24203

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
2 changes: 2 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ PHP NEWS
- Standard:
. Fixed chown() and lchown() failing to resolve user names in ZTS builds
when getpwnam_r() needs a larger buffer. (Ilia Alshanetsky)
. Fixed password_get_info() and password_needs_rehash() accepting malformed
bcrypt costs. (Ilia Alshanetsky)

- Zip:
. Fixed use-after-free when re-entering ZipArchive during destruction or
Expand Down
32 changes: 21 additions & 11 deletions ext/standard/password.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,37 +113,47 @@ static zend_string* php_password_get_salt(zval *unused_, size_t required_salt_le

/* bcrypt implementation */

static bool php_password_bcrypt_valid(const zend_string *hash) {
static bool php_password_bcrypt_get_cost(const zend_string *hash, zend_long *cost) {
const char *h = ZSTR_VAL(hash);
return (ZSTR_LEN(hash) == 60) &&
(h[0] == '$') && (h[1] == '2') && (h[2] == 'y');

if ((ZSTR_LEN(hash) != 60) ||
(h[0] != '$') || (h[1] != '2') || (h[2] != 'y') || (h[3] != '$') ||
!ZEND_IS_DIGIT(h[4]) || !ZEND_IS_DIGIT(h[5]) || (h[6] != '$')) {
return false;
}

*cost = (h[4] - '0') * 10 + (h[5] - '0');

return *cost >= 4 && *cost <= 31;
}

static bool php_password_bcrypt_valid(const zend_string *hash) {
zend_long cost;

return php_password_bcrypt_get_cost(hash, &cost);
}

static int php_password_bcrypt_get_info(zval *return_value, const zend_string *hash) {
zend_long cost = PHP_PASSWORD_BCRYPT_COST;
zend_long cost;

if (!php_password_bcrypt_valid(hash)) {
if (!php_password_bcrypt_get_cost(hash, &cost)) {
/* Should never get called this way. */
return FAILURE;
}

sscanf(ZSTR_VAL(hash), "$2y$" ZEND_LONG_FMT "$", &cost);
add_assoc_long(return_value, "cost", cost);

return SUCCESS;
}

static bool php_password_bcrypt_needs_rehash(const zend_string *hash, zend_array *options) {
zval *znew_cost;
zend_long old_cost = PHP_PASSWORD_BCRYPT_COST;
zend_long old_cost;
zend_long new_cost = PHP_PASSWORD_BCRYPT_COST;

if (!php_password_bcrypt_valid(hash)) {
if (!php_password_bcrypt_get_cost(hash, &old_cost)) {
/* Should never get called this way. */
return 1;
}

sscanf(ZSTR_VAL(hash), "$2y$" ZEND_LONG_FMT "$", &old_cost);
if (options && (znew_cost = zend_hash_str_find(options, "cost", sizeof("cost")-1)) != NULL) {
new_cost = zval_get_long(znew_cost);
}
Expand Down
27 changes: 27 additions & 0 deletions ext/standard/tests/password/password_get_info.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,26 @@ var_dump(password_get_info('$2y$11$MTIzNDU2Nzg5MDEyMzQ1Nej0NmcAWSLR.oP7XOR9HD/vj
// Test Non-Bcrypt
var_dump(password_get_info('$1$rasmusle$rISCgZzpwk3UhDidwXvin0'));

// Valid cost boundaries
$suffix = 'MTIzNDU2Nzg5MDEyMzQ1Nej0NmcAWSLR.oP7XOR9HD/vjUuOj100y';
foreach ([4, 31] as $cost) {
$info = password_get_info('$2y$' . sprintf('%02d', $cost) . '$' . $suffix);
printf("valid %d: %s, cost %d\n", $cost, $info['algoName'], $info['options']['cost']);
}

// Invalid cost grammar and range
$invalidHashes = [
'non-digit' => '$2y$a0$' . $suffix,
'malformed separator' => '$2y$10x' . $suffix,
'below range' => '$2y$03$' . $suffix,
'above range' => '$2y$32$' . $suffix,
'missing separator' => '$2y$' . str_repeat('9', 56),
];
foreach ($invalidHashes as $description => $hash) {
$info = password_get_info($hash);
printf("%s: %s, options %d\n", $description, $info['algoName'], count($info['options']));
}

echo "OK!";
?>
--EXPECT--
Expand Down Expand Up @@ -55,4 +75,11 @@ array(3) {
array(0) {
}
}
valid 4: bcrypt, cost 4
valid 31: bcrypt, cost 31
non-digit: unknown, options 0
malformed separator: unknown, options 0
below range: unknown, options 0
above range: unknown, options 0
missing separator: unknown, options 0
OK!
21 changes: 21 additions & 0 deletions ext/standard/tests/password/password_needs_rehash.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,21 @@ var_dump(password_needs_rehash('$2y$'.$cost.'$MTIzNDU2Nzg5MDEyMzQ1Nej0NmcAWSLR.o
// Should Issue Needs Rehash, Since Foo is cast to 0...
var_dump(password_needs_rehash('$2y$10$MTIzNDU2Nzg5MDEyMzQ1Nej0NmcAWSLR.oP7XOR9HD/vjUuOj100y', PASSWORD_BCRYPT, array('cost' => 'foo')));

// Valid cost boundaries
$suffix = 'MTIzNDU2Nzg5MDEyMzQ1Nej0NmcAWSLR.oP7XOR9HD/vjUuOj100y';
$costCases = [
'valid lower boundary' => ['$2y$04$' . $suffix, 4],
'valid upper boundary' => ['$2y$31$' . $suffix, 31],
'non-digit' => ['$2y$a0$' . $suffix, 12],
'malformed separator' => ['$2y$10x' . $suffix, 10],
'below range' => ['$2y$03$' . $suffix, 3],
'above range' => ['$2y$32$' . $suffix, 32],
];
foreach ($costCases as $description => $case) {
echo $description, ': ';
var_dump(password_needs_rehash($case[0], PASSWORD_BCRYPT, ['cost' => $case[1]]));
}

// CRYPT_MD5
var_dump(password_needs_rehash(crypt('Example', '$1$'), PASSWORD_DEFAULT));

Expand All @@ -54,6 +69,12 @@ bool(true)
bool(true)
bool(false)
bool(true)
valid lower boundary: bool(false)
valid upper boundary: bool(false)
non-digit: bool(true)
malformed separator: bool(true)
below range: bool(true)
above range: bool(true)
bool(true)
bool(true)
OK!
Loading