Fix GH-16411: gmp_export() can cause overflow #16418

cmb69 · 2024-10-13T17:11:53Z

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.

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.

Girgias

Seems reasonable, but I don't exactly understand what this function does.

Girgias · 2024-10-13T17:29:12Z

ext/gmp/tests/gh16411.phpt

+gmp
+--FILE--
+<?php
+gmp_export(-9223372036854775808, 9223372036854775807, -9223372036854775808);


Seems like we don't validate the $option argument properly?

Ah, we're only checking for the relevant flags:

php-src/ext/gmp/gmp.c

Lines 929 to 958 in 79c71c9

switch (options & (GMP_LSW_FIRST | GMP_MSW_FIRST)) {

case GMP_LSW_FIRST:

*order = -1;

break;

case GMP_MSW_FIRST:

case 0: /* default */

*order = 1;

break;

default:

/* options argument is in third position */

zend_argument_value_error(3, "cannot use multiple word order options");

return false;

}

switch (options & (GMP_LITTLE_ENDIAN | GMP_BIG_ENDIAN | GMP_NATIVE_ENDIAN)) {

case GMP_LITTLE_ENDIAN:

*endian = -1;

break;

case GMP_BIG_ENDIAN:

*endian = 1;

break;

case GMP_NATIVE_ENDIAN:

case 0: /* default */

*endian = 0;

break;

default:

/* options argument is in third position */

zend_argument_value_error(3, "cannot use multiple endian options");

return false;

}

So basically $options % 32. I think this okay (wouldn't want to touch it for a stable release), but could be improved for master (reject >= 32).

Yeah, improving this for master is what I had in mind, I was just surprised that large int wasn't caught by the option checking.

Girgias · 2024-10-13T17:34:37Z

Also, is this not an issue for gmp_import?

cmb69 · 2024-10-13T17:42:44Z

I don't exactly understand what this function does.

Do you mean gmp_export() or the new overflow check. If the latter, I guess I should add a comment (already thought about that; but not quite sure how detailed).

Also, is this not an issue for gmp_import?

No, since there are no calculations, and from what I can tell, we're not calling mpz_import() in a way we truncation may occur.

Anyway, as CI tells me, I need to cater to 32bit.

Girgias · 2024-10-13T17:55:31Z

I don't exactly understand what this function does.

Do you mean gmp_export() or the new overflow check. If the latter, I guess I should add a comment (already thought about that; but not quite sure how detailed).

I meant gmp_export()

Also, is this not an issue for gmp_import?

No, since there are no calculations, and from what I can tell, we're not calling mpz_import() in a way we truncation may occur.

OK

Anyway, as CI tells me, I need to cater to 32bit.

Indeed.

cmb69 · 2024-10-13T18:23:28Z

The documentation is pretty clear about gmp_export(): "Export a GMP number to a binary string". ;)

It's actually an elaborate pack(); besides that it obviously works for large numbers, you cannot only choose endianess, but also the $word_size. Oh, works somewhat different from what I had expected:

php > echo bin2hex(pack("N", 0x12345678));
12345678
php > echo bin2hex(pack("V", 0x12345678));
78563412
php > echo bin2hex(gmp_export(0x12345678, 1, GMP_BIG_ENDIAN));
12345678
php > echo bin2hex(gmp_export(0x12345678, 1, GMP_LITTLE_ENDIAN));
12345678

Need to check what's going on there.

cmb69 · 2024-10-13T18:41:01Z

The documentation is pretty clear about gmp_export(): "Export a GMP number to a binary string". ;)

It's actually an elaborate pack(); besides that it obviously works for large numbers, you cannot only choose endianess, but also the $word_size. Oh, works somewhat different from what I had expected:
php > echo bin2hex(pack("N", 0x12345678));
12345678
php > echo bin2hex(pack("V", 0x12345678));
78563412
php > echo bin2hex(gmp_export(0x12345678, 1, GMP_BIG_ENDIAN));
12345678
php > echo bin2hex(gmp_export(0x12345678, 1, GMP_LITTLE_ENDIAN));
12345678
Need to check what's going on there.

PS: all good. For $word_size=1 endianess is actually determined by GMP_(LSW|MSW)_FIRST:

php > echo bin2hex(gmp_export(0x12345678, 1, GMP_MSW_FIRST));
12345678
php > echo bin2hex(gmp_export(0x12345678, 1, GMP_LSW_FIRST));
78563412

Only for $word_size>1, GMP_(BIG|LITTE)_ENDIAN is relevant:

php > echo bin2hex(gmp_export(0x12345678, 2, GMP_LSW_FIRST|GMP_LITTLE_ENDIAN));
78563412
php > echo bin2hex(gmp_export(0x12345678, 2, GMP_LSW_FIRST|GMP_BIG_ENDIAN));
56781234

Girgias

LGTM

Girgias · 2024-10-13T19:16:48Z

ext/gmp/tests/gh16411.phpt

+--FILE--
+<?php
+gmp_export("-9223372036854775808", PHP_INT_MAX, PHP_INT_MIN);
+?>
+--EXPECTF--
+Fatal error: Uncaught ValueError: gmp_export(): Argument #2 ($word_size) is too large for argument #1 ($num) in %s:%d
+%A


You could try/catch the ValueError and have the output just be an EXPECT section, but that is minor nit.

cmb69 requested a review from Girgias as a code owner October 13, 2024 17:11

github-actions bot added the Extension: gmp label Oct 13, 2024

cmb69 linked an issue Oct 13, 2024 that may be closed by this pull request

gmp_export() can cause overflow #16411

Closed

Girgias approved these changes Oct 13, 2024

View reviewed changes

Support test on 32bit too

5f4e65c

Girgias approved these changes Oct 13, 2024

View reviewed changes

Girgias mentioned this pull request Oct 13, 2024

Improve gmp_export() documentation php/doc-en#3865

Open

cmb69 closed this in ab595c0 Oct 15, 2024

cmb69 deleted the cmb/gh16411 branch October 15, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix GH-16411: gmp_export() can cause overflow #16418

Fix GH-16411: gmp_export() can cause overflow #16418

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

Girgias left a comment

Uh oh!

Girgias Oct 13, 2024

Uh oh!

cmb69 Oct 13, 2024

Uh oh!

Girgias Oct 13, 2024

Uh oh!

Girgias commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

Girgias commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

Girgias left a comment

Uh oh!

Girgias Oct 13, 2024

Uh oh!

Uh oh!

	switch (options & (GMP_LSW_FIRST \| GMP_MSW_FIRST)) {
	case GMP_LSW_FIRST:
	*order = -1;
	break;
	case GMP_MSW_FIRST:
	case 0: /* default */
	*order = 1;
	break;
	default:
	/* options argument is in third position */
	zend_argument_value_error(3, "cannot use multiple word order options");
	return false;
	}

	switch (options & (GMP_LITTLE_ENDIAN \| GMP_BIG_ENDIAN \| GMP_NATIVE_ENDIAN)) {
	case GMP_LITTLE_ENDIAN:
	*endian = -1;
	break;
	case GMP_BIG_ENDIAN:
	*endian = 1;
	break;
	case GMP_NATIVE_ENDIAN:
	case 0: /* default */
	*endian = 0;
	break;
	default:
	/* options argument is in third position */
	zend_argument_value_error(3, "cannot use multiple endian options");
	return false;
	}

Fix GH-16411: gmp_export() can cause overflow #16418

Fix GH-16411: gmp_export() can cause overflow #16418

Uh oh!

Conversation

cmb69 commented Oct 13, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

cmb69 Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

Girgias commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

Girgias commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

cmb69 commented Oct 13, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!