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

gdImageFillToBorder stack-overflow when invalid color is used #215

Closed
fmunozs opened this issue May 25, 2016 · 13 comments
Closed

gdImageFillToBorder stack-overflow when invalid color is used #215

fmunozs opened this issue May 25, 2016 · 13 comments
Labels
Milestone

Comments

@fmunozs
Copy link

fmunozs commented May 25, 2016

Invalid color causes stack exhaustion by recursive call to function gdImageFillToBorder when the image used is not truecolor.

Source code:
https://github.com/php/php-src/blob/master/ext/gd/libgd/gd.c#L1811

    if (y > 0) {
        lastBorder = 1;
        for (i = leftLimit; i <= rightLimit; i++) {
            int c = gdImageGetPixel(im, i, y - 1);
            if (lastBorder) {
                if ((c != border) && (c != color)) {
                    gdImageFillToBorder(im, i, y - 1, border, color);
                    lastBorder = 0;
                }
            } else if ((c == border) || (c == color)) {
                lastBorder = 1;
            }
        }
    }

    /* Below */
    if (y < ((im->sy) - 1)) {
        lastBorder = 1;
        for (i = leftLimit; i <= rightLimit; i++) {
            int c = gdImageGetPixel(im, i, y + 1);

            if (lastBorder) {
                if ((c != border) && (c != color)) {
                    gdImageFillToBorder(im, i, y + 1, border, color);
                    lastBorder = 0;
                }
            } else if ((c == border) || (c == color)) {
                lastBorder = 1;
            }
        }
    }
    im->alphaBlendingFlag = restoreAlphaBlending;

gdb -q --args /home/user/php-7.0/sapi/cli/php -n poc.php
Reading symbols from /home/user/php-7.0/sapi/cli/php...done.
(gdb) b gd.c:1811
Breakpoint 1 at 0x8176146: file /home/user/php-7.0/ext/gd/libgd/gd.c, line 1811.
(gdb) b gd.c:1829
Breakpoint 2 at 0x81761f2: file /home/user/php-7.0/ext/gd/libgd/gd.c, line 1829.
(gdb) r
Starting program: /home/user/php-7.0/sapi/cli/php -n poc.php

Breakpoint 2, php_gd_gdImageFillToBorder (im=0xf5a6c000, x=0, y=0, border=1, color=313) at /home/user/php-7.0/ext/gd/libgd/gd.c:1829
1829                                            gdImageFillToBorder(im, i, y + 1, border, color);
(gdb) p c
$1 = 0
(gdb) c
Continuing.

Breakpoint 1, php_gd_gdImageFillToBorder (im=0xf5a6c000, x=0, y=1, border=1, color=313) at /home/user/php-7.0/ext/gd/libgd/gd.c:1811
1811                                    if ((c != border) && (c != color)) {
(gdb) p c
$2 = 57         (different of color 313)
(gdb) c
Continuing.

Breakpoint 2, php_gd_gdImageFillToBorder (im=0xf5a6c000, x=0, y=0, border=1, color=313) at /home/user/php-7.0/ext/gd/libgd/gd.c:1829
1829                                            gdImageFillToBorder(im, i, y + 1, border, color);
(gdb) p c
$3 = 57        (different of color 313)
(gdb) c
Continuing.

Breakpoint 1, php_gd_gdImageFillToBorder (im=0xf5a6c000, x=0, y=1, border=1, color=313) at /home/user/php-7.0/ext/gd/libgd/gd.c:1811
1811                                    if ((c != border) && (c != color)) {
(gdb) p c
$4 = 57        (different of color 313)

Test script:

<?php
$img = imagecreate(10, 10);
imagefilltoborder($img, 0, 0, 1, 0x139);

Expected result:

No crash

Actual result:

user@Xenial32-2:~/crashes/fillborder$ /ramdisk/php-71/sapi/cli/php -n poc.php
ASAN:SIGSEGV
=================================================================
==5027==ERROR: AddressSanitizer: stack-overflow on address 0xff276ff0 (pc 0x085ee3f8 bp 0x1e4cdb90 sp 0xff276ff0 T0)
    #0 0x85ee3f7 in php_gd_gdImageSetPixel /home/user/php-7.1asan/ext/gd/libgd/gd.c:755
    #1 0xf266bfff  (<unknown module>)

SUMMARY: AddressSanitizer: stack-overflow /home/user/php-7.1asan/ext/gd/libgd/gd.c:755 php_gd_gdImageSetPixel
==5027==ABORTING
user@Xenial32-2:~/crashes/fillborder$ /ramdisk/php-71/sapi/cli/php -n poc.php
ASAN:SIGSEGV
=================================================================
==5771==ERROR: AddressSanitizer: stack-overflow on address 0xff758ff4 (pc 0x085ebfcf bp 0xfffffffc sp 0xff758ff0 T0)
    #0 0x85ebfce in gdImageTileApply /home/user/php-7.1asan/ext/gd/libgd/gd.c:869
    #1 0x85ebfce in php_gd_gdImageSetPixel /home/user/php-7.1asan/ext/gd/libgd/gd.c:749
    #2 0x85f7eae in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1788
    #3 0x85f8f32 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1829
    #4 0x85f8e04 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1812
    #5 0x85f8f32 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1829
    #6 0x85f8e04 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1812
    #7 0x85f8f32 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1829
    #8 0x85f8e04 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1812
    #9 0x85f8f32 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1829
    #10 0x85f8e04 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1812
    #11 0x85f8f32 in php_gd_gdImageFillToBorder /home/user/php-7.1asan/ext/gd/libgd/gd.c:1829
@vapier vapier changed the title gdImageFillToBorder stack-overflow gdImageFillToBorder stack-overflow when invalid color is used May 25, 2016
@vapier
Copy link
Member

vapier commented May 25, 2016

seems related to the fix for issue #213. we silently clip the coordinates to the limits of the image now.

@fmunozs
Copy link
Author

fmunozs commented May 25, 2016

I'm not sure If its the same issue, since I identified it on the libgd version included with php, that fixed #213 first. Also note that I'm no using big numbers on parameters. According to my testing there is no stackoverflow If I use imagecreatetruecolor.

@vapier
Copy link
Member

vapier commented May 25, 2016

i'm not saying it's the same issue, just somewhat related

@fmunozs
Copy link
Author

fmunozs commented May 25, 2016

Ok, sorry for the noise then :) language barrier. How does this patch look to you?

--- a/ext/gd/libgd/gd.c
+++ b/ext/gd/libgd/gd.c
@@ -1770,6 +1770,8 @@ void gdImageFillToBorder (gdImagePtr im
        restoreAlphaBlending = im->alphaBlendingFlag;
        im->alphaBlendingFlag = 0;

+       if (!im->trueColor) color &= 0xff;
+
        if (x >= im->sx) {
                x = im->sx - 1;
        } else if (x < 0) {

It's for php's libgd though.

@vapier
Copy link
Member

vapier commented May 26, 2016

that will make it work in the non-truecolor mode, but i think truecolor still has issues. when you factor in the different effect modes (alpha-blending, overlays, multiply), doing a set+get on a pixel might not return the exact same value you put in (color in this case).

one thing i really hate about gd's API in general is its inconsistent (and uncommon) ability to signal errors. ideally, gdImageFillToBorder would return an error whenever sanity was violated:

  • x or y coordinates out of bounds
  • border is invalid
  • color is invalid for image

@pierrejoye
Copy link
Contributor

pierrejoye commented Jun 2, 2016

@vapier totally agree, sadly remaining of the old APIs where we try to keep BC.

also correct fix could be:

if (!im->trueColor) {
    if ((color >= gdMaxColors) && (color > im->colorsTotal)) {
        return;
    }
}

@pierrejoye
Copy link
Contributor

pierrejoye commented Jun 2, 2016

@vapier about tc colors, this function overrides the modes, it only works in full replace mode, no blending is ever done.

See

libgd/src/gd.c

Line 1939 in 502e4cd

im->alphaBlendingFlag = 0;

@vapier
Copy link
Member

vapier commented Jun 3, 2016

ah yeah, i missed gdImageFillToBorder clobbering alphaBlendingFlag. that addresses my concern.

i'm not sure about that fix -- if the user requested a color that is outside of colorsTotal, shouldn't we reject that too ? the gdImageFill func enforces things this way too:

    if (!im->trueColor && nc > (im->colorsTotal - 1)) {
        return;
    }

i think we want to use that in this func.

@pierrejoye
Copy link
Contributor

pierrejoye commented Jun 3, 2016

Yes that works too. Is color unsigned or signed? If the latter we must check for >0 too.

@vapier
Copy link
Member

vapier commented Jun 3, 2016

colorsTotal is an int as is nc and color, so gdImageFill will be comparing nc against -1 in the initial (common?) case.

it's not clear to me how a value of 0 should be handled with colorsTotal. should we be initializing that to 256 instead in gdImageCreate ?

@pierrejoye
Copy link
Contributor

colorsTotal is the currently amount of colors allocated (see the color
allocate function). 256 is the maximum amount of colors. Both being used
for palette images.
On Jun 3, 2016 10:41 PM, "Mike Frysinger" notifications@github.com wrote:

colorsTotal is an int as is nc and color, so gdImageFill will be
comparing nc against -1 in the initial (common?) case.

it's not clear to me how a value of 0 should be handled with colorsTotal.
should we be initializing that to 256 instead in gdImageCreate ?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#215 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AARPKHngKjw7haLn10v4se99bO8UANUEks5qIEs1gaJpZM4ImbEz
.

pierrejoye added a commit that referenced this issue Jun 4, 2016
* GD-2.2:
  fix #215 gdImageFillToBorder stack-overflow when invalid color is used
  tests: add bug_github_18 to gitignore #18
  tests: fix typo in test name #18
@vapier
Copy link
Member

vapier commented Jun 4, 2016

i think you forgot to git add the test so the build now fails ...

also, i think the question is still open: should gdImageCreate be defaulting colorsTotal to 256 instead of 0 ? if we do that, we can make more assumptions elsewhere and not treat 0 specially.

pierrejoye added a commit that referenced this issue Jun 4, 2016
pierrejoye added a commit that referenced this issue Jun 4, 2016
* GD-2.2:
  fix #215 missing test
@pierrejoye
Copy link
Contributor

We need the amount of colors allocated when it comes to save the image. We can indeed scan the image to see if a color index is used or not but it can be very slow.

Also 0 is a special case and have to remain one as it is used at the background color (the 1st allocate will be the default bgd color for palette image, why gdCalloc is used to allocate the rows).

pierrejoye added a commit that referenced this issue Jun 8, 2016
* GD-2.2:
  fix #215, invalid color index, missing case for invalid border
pierrejoye added a commit that referenced this issue Jun 8, 2016
* GD-2.2:
  fix #215, invalid color index, missing case for invalid border
@pierrejoye pierrejoye added this to the GD 2.2.2 milestone Jun 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants