gmp_export() can cause overflow #16411

Activity

1. YuanchengJiang

   added

   Bug

   Status: Needs Triage

   on Oct 13, 2024
2. ndossche

   added

   Extension: gmp

   Status: Verified

   and removed

   Status: Needs Triage

   on Oct 13, 2024
3. cmb69 commented on Oct 13, 2024

   

   cmb69

   on Oct 13, 2024

   Member

   More actions

    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.
4. ndossche commented on Oct 13, 2024

   

   ndossche

   on Oct 13, 2024

   Member

   More actions

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

   

   cmb69

   on Oct 13, 2024

   Member

   More actions

   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

   php-src/ext/gmp/gmp.c

   Line 1016 in 79c71c9

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

   Would probably need another check here.
6. ndossche commented on Oct 13, 2024

   

   ndossche

   on Oct 13, 2024

   Member

   More actions

   Right that makes sense indeed
7. cmb69

   self-assigned this

   on Oct 13, 2024
8. cmb69

   added a commit that references this issue on Oct 13, 2024

   Fix phpGH-16411: gmp_export() can cause overflow

   ...

   This issue will close once commit 4000ee7 is merged into the 'master' branch.

   4000ee7
9. cmb69

   changed the title [-]Signed integer overflow in ext/gmp/gmp.c[/-] [+]gmp_export() can cause overflow[/+] on Oct 13, 2024
10. cmb69

    linked a pull request that will close this issueFix GH-16411: gmp_export() can cause overflow #16418on Oct 13, 2024
11. cmb69

    added 2 commits that reference this issue on Oct 15, 2024

    Fix GH-16411: gmp_export() can cause overflow

    ...

    This issue will close once commit ab595c0 is merged into the 'master' branch.

    ab595c0

    Merge branch 'PHP-8.2' into PHP-8.3

    ...

    This issue will close once commit ea6f78e is merged into the 'master' branch.

    ea6f78e
12. cmb69

    closed this as completedin ab595c0on Oct 15, 2024
13. cmb69

    added 2 commits that reference this issue on Oct 15, 2024

    Merge branch 'PHP-8.3' into PHP-8.4

    ...

    This issue will close once commit b1fbdd8 is merged into the 'master' branch.

    b1fbdd8

    Merge branch 'PHP-8.4'

    ...

    This issue will close once commit dbdcc95 is merged into the 'master' branch.

    dbdcc95