Nothing Special   »   [go: up one dir, main page]

Skip to content
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_export() can cause overflow #16411

Closed
YuanchengJiang opened this issue Oct 13, 2024 · 4 comments
Closed

gmp_export() can cause overflow #16411

YuanchengJiang opened this issue Oct 13, 2024 · 4 comments

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
gmp_export(-9223372036854775808, 9223372036854775807, -9223372036854775808);

Resulted in this output:

/php-src/ext/gmp/gmp.c:1015:31: runtime error: signed integer overflow: 9223372036854775807 * 8 cannot be represented in type 'long'

PHP Version

nightly

Operating System

ubuntu 22.04

@cmb69
Copy link
Member
cmb69 commented Oct 13, 2024
 ext/gmp/gmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ext/gmp/gmp.c b/ext/gmp/gmp.c
index 04bfafe869..bbea70225e 100644
--- a/ext/gmp/gmp.c
+++ b/ext/gmp/gmp.c
@@ -920,9 +920,9 @@ ZEND_FUNCTION(gmp_init)
 
 static bool gmp_import_export_validate(zend_long size, zend_long options, int *order, int *endian)
 {
-	if (size < 1) {
+	if (size < 1 || size > ZEND_LONG_MAX / 4) {
 		/* size argument is in second position */
-		zend_argument_value_error(2, "must be greater than or equal to 1");
+		zend_argument_value_error(2, "must be greater than or equal to 1 and less than or equal to " ZEND_LONG_FMT, ZEND_LONG_MAX / 4);
 		return false;
 	}
 
@@ -1012,7 +1012,7 @@ ZEND_FUNCTION(gmp_export)
 	if (mpz_sgn(gmpnumber) == 0) {
 		RETVAL_EMPTY_STRING();
 	} else {
-		size_t bits_per_word = size * 8;
+		size_t bits_per_word = (size_t) size * 8;
 		size_t count = (mpz_sizeinbase(gmpnumber, 2) + bits_per_word - 1) / bits_per_word;
 
 		zend_string *out_string = zend_string_safe_alloc(count, size, 0, 0);

would prevent this overflow, but still an unexpected unsigned overflow would occur on the next line, so size needs to be smaller than ZEND_LONG_MAX/4.

@nielsdos
Copy link
Member

@cmb69 Feel free to make a PR, although I don't understand yet how you arrived at ZEND_LONG_MAX/4.

@cmb69
Copy link
Member
cmb69 commented Oct 13, 2024

Would need to divide by 8, but since size is signed, we can double to fit into unsigned; thus divide by 4.

However, as I said, that value may still be to large for

size_t count = (mpz_sizeinbase(gmpnumber, 2) + bits_per_word - 1) / bits_per_word;

Would probably need another check here.

@nielsdos
Copy link
Member

Right that makes sense indeed

@cmb69 cmb69 self-assigned this Oct 13, 2024
cmb69 added a commit to cmb69/php-src that referenced this issue Oct 13, 2024
We need not only to avoid the signed overflow while calculating
`bits_per_word` (reported issue), but also the unsigned overflow when
calculating `count`.  While the former has a fixed threshold, the
latter does not, since it also depends on the size in base 2.  Thus we
use a somewhat unconventional error message.
@cmb69 cmb69 changed the title Signed integer overflow in ext/gmp/gmp.c gmp_export() can cause overflow Oct 13, 2024
@cmb69 cmb69 linked a pull request Oct 13, 2024 that will close this issue
cmb69 added a commit that referenced this issue Oct 15, 2024
* PHP-8.2:
  Fix GH-16411: gmp_export() can cause overflow
@cmb69 cmb69 closed this as completed in ab595c0 Oct 15, 2024
cmb69 added a commit that referenced this issue Oct 15, 2024
* PHP-8.3:
  Fix GH-16411: gmp_export() can cause overflow
cmb69 added a commit that referenced this issue Oct 15, 2024
* PHP-8.4:
  Fix GH-16411: gmp_export() can cause overflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants