Skip to content

Fix GH-16771: imagecreatefromstring overflow on invalid format. #16776

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix GH-16771: imagecreatefromstring overflow on invalid format. #16776

wants to merge 2 commits into from

Conversation

devnexen
Copy link
Member

No description provided.

@devnexen devnexen marked this pull request as ready for review November 13, 2024 12:13
@cmb69
Copy link
Member

cmb69 commented Nov 13, 2024

I think the patch fixes the overflow (and as such is good as bug fix), but also see

php-src/ext/standard/image.c

Lines 912 to 957 in 7b029a3

static int php_get_wbmp(php_stream *stream, struct gfxinfo **result, int check)
{
int i, width = 0, height = 0;
if (php_stream_rewind(stream)) {
return 0;
}
/* get type */
if (php_stream_getc(stream) != 0) {
return 0;
}
/* skip header */
do {
i = php_stream_getc(stream);
if (i < 0) {
return 0;
}
} while (i & 0x80);
/* get width */
do {
i = php_stream_getc(stream);
if (i < 0) {
return 0;
}
width = (width << 7) | (i & 0x7f);
/* maximum valid width for wbmp (although 127 may be a more accurate one) */
if (width > 2048) {
return 0;
}
} while (i & 0x80);
/* get height */
do {
i = php_stream_getc(stream);
if (i < 0) {
return 0;
}
height = (height << 7) | (i & 0x7f);
/* maximum valid height for wbmp (although 127 may be a more accurate one) */
if (height > 2048) {
return 0;
}
} while (i & 0x80);

That code looks better. According to the WBMP specification (section 6), the first field is a multibyte integer, but the second field is a fixed size integer (8 bit). The code in gd.c assumes that both are MBI. Plus, only 0 is supported as type (first field), so basically we only need to check for a 0 byte, followed by a non-zero byte (although, that is probably wrong). See also

php-src/ext/gd/libgd/wbmp.c

Lines 160 to 186 in 7b029a3

wbmp->type = getin (in);
if (wbmp->type != 0)
{
gdFree (wbmp);
return (-1);
}
if (skipheader (getin, in))
{
gdFree (wbmp);
return (-1);
}
wbmp->width = getmbi (getin, in);
if (wbmp->width == -1)
{
gdFree (wbmp);
return (-1);
}
wbmp->height = getmbi (getin, in);
if (wbmp->height == -1)
{
gdFree (wbmp);
return (-1);
}

So I suggest (maybe for master only) to completely drop _php_ctx_getmbi() and instead just to check whether the first byte is zero and then to return PHP_GDIMG_TYPE_WBM; (the code in readwbmp() should detect invalid WBMP early). Untested patch:

 ext/gd/gd.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/ext/gd/gd.c b/ext/gd/gd.c
index a2c3e7d0de..938672ccc0 100644
--- a/ext/gd/gd.c
+++ b/ext/gd/gd.c
@@ -1358,24 +1358,6 @@ PHP_FUNCTION(imagetypes)
 }
 /* }}} */
 
-/* {{{ _php_ctx_getmbi */
-
-static int _php_ctx_getmbi(gdIOCtx *ctx)
-{
-	int i, mbi = 0;
-
-	do {
-		i = (ctx->getC)(ctx);
-		if (i < 0) {
-			return -1;
-		}
-		mbi = (mbi << 7) | (i & 0x7f);
-	} while (i & 0x80);
-
-	return mbi;
-}
-/* }}} */
-
 /* {{{ _php_image_type
  * Based on ext/standard/image.c
  */
@@ -1413,15 +1395,8 @@ static int _php_image_type(zend_string *data)
 		}
 	}
 
-	gdIOCtx *io_ctx;
-	io_ctx = gdNewDynamicCtxEx(8, ZSTR_VAL(data), 0);
-	if (io_ctx) {
-		if (_php_ctx_getmbi(io_ctx) == 0 && _php_ctx_getmbi(io_ctx) >= 0) {
-			io_ctx->gd_free(io_ctx);
-			return PHP_GDIMG_TYPE_WBM;
-		} else {
-			io_ctx->gd_free(io_ctx);
-		}
+	if (ZSTR_VAL(data)[0] == 0) {
+		return PHP_GDIMG_TYPE_WBM;
 	}
 
 	return -1;

@devnexen
Copy link
Member Author

So I propose we merge it and then I suggest you open a PR for master, wdyt ?

@cmb69
Copy link
Member

cmb69 commented Nov 13, 2024

Sounds good!

@devnexen devnexen closed this in 4124b04 Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Extension: gd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devnexen @cmb69