-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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.
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
gmp_pow(64, 11) throws overflow exception #16870
Comments
This has been introduced via #16384. The threshold seems to be way too restrictive. |
Lines 1363 to 1370 in 3aa4973
The condition on line 1367 is basically checking whether |
PS: there is PPS: PPPS: instead of |
So I guess something like the following would do (where ext/gmp/gmp.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/ext/gmp/gmp.c b/ext/gmp/gmp.c
index 177d0c7f7c..abbffe2485 100644
--- a/ext/gmp/gmp.c
+++ b/ext/gmp/gmp.c
@@ -1360,11 +1360,11 @@ ZEND_FUNCTION(gmp_pow)
RETURN_THROWS();
}
- double powmax = log((double)ZEND_LONG_MAX);
+ double powmax = 2000;
if (Z_TYPE_P(base_arg) == IS_LONG && Z_LVAL_P(base_arg) >= 0) {
INIT_GMP_RETVAL(gmpnum_result);
- if ((log(Z_LVAL_P(base_arg)) * exp) > powmax) {
+ if ((log(Z_LVAL_P(base_arg)) / log(256) * exp) > powmax) {
zend_value_error("base and exponent overflow");
RETURN_THROWS();
}
@@ -1374,8 +1374,7 @@ ZEND_FUNCTION(gmp_pow)
zend_ulong gmpnum;
FETCH_GMP_ZVAL(gmpnum_base, base_arg, temp_base, 1);
INIT_GMP_RETVAL(gmpnum_result);
- gmpnum = mpz_get_ui(gmpnum_base);
- if ((log(gmpnum) * exp) > powmax) {
+ if ((mpz_sizeinbase(gmpnum_base, 16) / 2.0 * exp) > powmax) {
FREE_GMP_TEMP(temp_base);
zend_value_error("base and exponent overflow");
RETURN_THROWS(); That passes the ext/gmp test suite. |
Thanks @cmb69, please open a PR :) |
Should this same approach be used for #16880 ? |
The current guard to prevent FPEs is way too restrictive; `64 ** 11` is a perfectly reasonable operation. Instead, we now estimate the number of bytes of the resulting GMP (assuming that numbers are stored base 256 encoded), and fail if that exceeds a given threshold. The chosen threshold is somewhat arbitrary. We also ensure that we do not prematurely convert a given non int base to an int to avoid overflow which could circumvent our early check.
@Girgias, possibly. There are two unrelated issues: (a) find the proper threshold which doesn't trigger allocation overflow (regardless of whether checked by libgmp or not), and (b) do not prematurely convert to (unsigned) integers (which in itself might cause overflow). (b) may or not may already affect some functions, (a) is likely something we should unify throughout (though not necessarily for stable releases). And (a) may or not may take into account actual OOM conditions (we want to avoid libgmp/mpir aborts, but it's obviously not possible to predict OOM conditions). I think we should reconsider #16609. But especially the approach I'm suggesting here and the PR should be checked by people with a stronger math background than I have, and ideally by people who know more about libgmp/mir than I do. |
Isn't the whole point of gmp to work with large numbers? Why were these warnings added? Using gmp to manipulate IPv6 (128bit) addresses. Now app is borked with PHP Fatal Errors. Works fine in 8.3.13. # php -v
PHP 8.3.14 (cli) (built: Nov 21 2024 05:32:50) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.3.14, Copyright (c) Zend Technologies
with Xdebug v3.3.2, Copyright (c) 2002-2024, by Derick Rethans
# php -a
Interactive shell
php > $a = gmp_pow(2,128);
PHP Warning: Uncaught ValueError: base and exponent overflow in php shell code:1
Stack trace:
#0 php shell code(1): gmp_pow()
#1 {main}
thrown in php shell code on line 1
php > $a = gmp_pow(2,64);
PHP Warning: Uncaught ValueError: base and exponent overflow in php shell code:1
Stack trace:
#0 php shell code(1): gmp_pow()
#1 {main}
thrown in php shell code on line 1
php > $a = gmp_pow(2,63);
php > |
Here gmp gets used to do pubkey crypto, and fails with the reported error in Line 125: I'm not sure about the key size it uses, but it might be either 256, 384 or 521? Anyway that issue brought me here and I can report that the testcase I threw together works again when applying the patch from #16884 : print "\n521bit random number:\n";
#$big = gmp_random_bits(521);
$big = gmp_init("4251217229032923292930005432624928043281508404012751772104304003082554171429833539451625576338397719972103098793555878613020934461497631132347722033178373655", 10);
print gmp_strval($big);
print "\nsquared:\n";
print gmp_strval(gmp_pow($big, 2));
print "\ncubed:\n";
print gmp_strval(gmp_pow($big, 3)); No idea if its correct, but given the same number, .25 and .26-pathed give the same result.
EDIT: noticed I had a 512 bit number instead of a 521 bit number and redid the tests. Maybe the testcase in PHP should also test for number sizes typically found in cryptography? |
Very large number can cause overflow handling in libgmp, which then leads to abort of the whole process (very bad for ZTS environments, but also not nice for NTS). Thus, restricting the range of supported inputs is reasonable. However, the fix was too restrictive. Sorry!
That is a good idea. Maybe you want to contribute a PR? |
Well, I'm no crypto-guy, and I don't really know much about the inner workings of those algos... So, I could contribute the testcase above, maybe not just 521 bit, but also for 384 and 256, maybe 128 bit numbers, but not really anything more than that. |
Me neither.
Yes, that sounds reasonable. I think it's best to target |
@GaryAllan the issue is that the underlying GMP library actually doesn't allow to use a "GMP object" for certain parameters, but wants an Now why the underlying GMP doesn't allow this I don't understand. |
Hello, I've written a test case for ext/gmp/tests and would like to raise a PR. What's the bug ID for this issue? The IDs don't appear to match GitHub issue IDs. e.g bug74670.phpt 74670 isn't a GitHub Issue #. Where did bug ID 74670 come from and what's the equivalent for this bug? Thanks |
bug* prefix seems for Bugs reported at the old bugtracker: https://bugs.php.net/ |
The current guard to prevent FPEs is way too restrictive; `64 ** 11` is a perfectly reasonable operation. Instead, we now estimate the number of bytes of the resulting GMP (assuming that numbers are stored base 256 encoded), and fail if that exceeds a given threshold. The chosen threshold is somewhat arbitrary. We also ensure that we do not prematurely convert a given non int base to an int to avoid overflow which could circumvent our early check.
Not sure if we need more test cases; there are already #16896 and #16898, and I've added a couple of tests to the PR (5cb70a7).
Well, if you're on PHP 8.2 or 8.3, and are possibly affected by the vulnerability which have no been fixed, it might be better to not hold off on upgrading, but rather work around. If ext/gmp is built as static extension, you could use the .so of the old version. Also distro managers may already have sorted out, or are in the process of doing so. |
The current guard to prevent FPEs is way too restrictive; `64 ** 11` is a perfectly reasonable operation. Instead, we now estimate the number of bytes of the resulting GMP (assuming that numbers are stored base 256 encoded), and fail if that exceeds a given threshold. The chosen threshold is somewhat arbitrary. We also ensure that we do not prematurely convert a given non int base to an int to avoid overflow which could circumvent our early check.
Actually, a "simpler" solution might be to just use the fact that |
Good catch @Girgias - I'm testing that change on my library, and it seems to be ok. For some reason, I thought that was a much newer language feature. It does raise the question of why the operator overloads aren't validated as the same was as the functions. I would have (wrongly) assumed that the former is mere syntactic sugar for the latter; this leads me to think that if the original change was in response to a security concern, there could be application vulnerabilities that remain after patching depending on how the code was written. |
The GMP codebase is a "bit" jank atm, I got a big clean-up PR in #16685 that I need to land, but that is only for master. The memory overflow was found via fuzzing, and it only detected something on |
@Firehed :
Well, your example should be covered by the tests in #16896 . If you have any other feel free to share as a comment on my PR or maybe open a PR to the branch of my PR and I'll add them to my PR, if reasonable. |
…k for out-of-bounds values); php/php-src#16870
I tested this also in the 8.4.1 release and I get the same fatal error here too. It happens in these three releases from November 21:
|
Yes, this is normal, because it was a bug fixes that has been applied to all supported versions. This is going to be fixed in the next release after the revert of the broken fix. You can use the |
* avoid using gmp_pow() due to php/php-src#16870 (comment)
* avoid using gmp_pow() due to php/php-src#16870 (comment)
The overflow checks have been reverted, so this ticket can be closed. |
There is a bug in these two PHP releases that causes gmp_pow to fail. See php/php-src#16870
There is a bug in PHP 8.3.14 and 8.2.26 that causes gmp_pow to fail. See php/php-src#16870 Using operator overloading (**) instead of gmp_pow may fix the issue. See php/php-src#16870 (comment)
Description
The following code:
Resulted in this output:
But I expected this output instead:
https://3v4l.org/eY5kA
Not sure why the overflow triggered here, the resulting output is not particularly large (for GMP). Performing the same calculation by using gmp_mul() repeatedly does not cause an overflow exception.
PHP Version
PHP 8.3.14
Operating System
Arch Linux 6.11.6
The text was updated successfully, but these errors were encountered: