Skip to content

Fix buffer overflow in hash_pbkdf2() with a large output length #23380

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
lazerg wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
+25 −3
Open

Fix buffer overflow in hash_pbkdf2() with a large output length #23380

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
4 changes: 4 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ PHP NEWS
. Fixed imageaffinematrixget() and imageaffinematrixconcat() reporting the
wrong argument in error messages. (Weilin Du)

- Hash:
. Fixed a buffer overflow in hash_pbkdf2() with a large output length.
(Lazizbek Ergashev)

- Intl:
. Fixed a double-free when IntlGregorianCalendar construction fails after
the ICU constructor adopts the TimeZone. (iliaal)
Expand Down
5 changes: 2 additions & 3 deletions ext/hash/hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include <config.h>
#endif

#include <math.h>
#include "php_hash.h"
#include "ext/standard/info.h"
#include "ext/standard/file.h"
Expand Down Expand Up @@ -1043,10 +1042,10 @@ PHP_FUNCTION(hash_pbkdf2)
}
digest_length = length;
if (!raw_output) {
digest_length = (zend_long) ceil((float) length / 2.0);
digest_length = (length + 1) / 2;
}

loops = (zend_long) ceil((float) digest_length / (float) ops->digest_size);
loops = (digest_length + ops->digest_size - 1) / ops->digest_size;

@Sjord Sjord Aug 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this gives a compiler warning (if I enable all warnings):

ext/hash/hash.c:1048:17: warning: conversion to ‘zend_long’ {aka ‘long int’} from ‘long unsigned int’ may change the sign of the result [-Wsign-conversion]
 1048 |         loops = (digest_length + ops->digest_size - 1) / ops->digest_size;
      |                 ^

It is not an actual problem, so I think it's fine.

@lazerg lazerg Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it cannot bite here. digest_length is always at least 1 at that point: a negative length is rejected earlier, and 0 is replaced by the digest size. So the unsigned arithmetic cannot wrap, and the quotient always fits back into a zend_long. php-src does not build with -Wsign-conversion, so I left the line as it is. I can add an explicit cast if you would rather not see the warning.


result = safe_emalloc(loops, ops->digest_size, 0);

Expand Down
19 changes: 19 additions & 0 deletions ext/hash/tests/hash_pbkdf2_large_length.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
--TEST--
Hash: hash_pbkdf2() function : large output length
--FILE--
<?php

$length = 33554433;
$hash = hash_pbkdf2('md5', 'password', 'salt', 1, $length);

/* The last hexit comes from the first byte of the final PBKDF2 block. */
$block = intdiv(intdiv($length + 1, 2) - 1, 16) + 1;
$expected = bin2hex(hash_hmac('md5', 'salt' . pack('N', $block), 'password', true));

var_dump(strlen($hash));
var_dump($hash[$length - 1] === $expected[0]);

?>
--EXPECT--
int(33554433)
bool(true)
Loading