Bug 623831 (CVE-2010-1519)

Created libglpng tracking bugs for this issue

Affects: fedora-all [bug 623832]

The advisory:
http://secunia.com/secunia_research/2010-87/

Is a bit scarce on details, it mentions posts to the vendor-sec list, did those include some more details perhaps ?

Not much.  They did indicate that the integer overflow in pngLoadRoawF() was around line 304 and the integer overflow in pngLoadF() was around line 443.  They also indicated that there were other potential integer overflows in line 461 and 542, and also noted that the functions do not properly check if the return value is NULL, which could potentially be exploited to cause a crash.

That is the extent of any "additional" details provided.  Not overly helpful, I'm afraid.

Note that the attached patch appears to add some (small) memory leaks at 280, 282, 400 and 402.  Specifically, if png (and optionally info) succeeds but either info or endinfo fails, the memory allocated in the succeeding case(s) is leaked.

(In reply to comment #16)
> Note that the attached patch appears to add some (small) memory leaks at 280,
> 282, 400 and 402.  Specifically, if png (and optionally info) succeeds but
> either info or endinfo fails, the memory allocated in the succeeding case(s) is
> leaked.

Thanks, but I knew already. The problem is that the png destroy function wants
all 3 pointers to do a destroy. So I see no clear way to free up the png and/or info when the endinfo allocation failed.

Note it may be that the destroy function will accept NULL pointers for info / endinfo. The documentation is unclear on this, and given the very small likely hood of this ever happening, I didn't bother to actual check the code. So I've deliberately chosen to leak some memory in this case rather then the destroy function not grokking a NULL ptr causing a crash (although once malloc's start failing the app is doomed anyway).

Regards,

Hans

> Note it may be that the destroy function will accept NULL pointers for info /
> endinfo. The documentation is unclear on this

In fact, libpng.txt includes an explicit example of precisely this:

    png_structp png_ptr = png_create_read_struct
       (PNG_LIBPNG_VER_STRING, (png_voidp)user_error_ptr,
        user_error_fn, user_warning_fn);
    if (!png_ptr)
        return (ERROR);
 
    png_infop info_ptr = png_create_info_struct(png_ptr);
    if (!info_ptr) 
    {
        png_destroy_read_struct(&png_ptr,
           (png_infopp)NULL, (png_infopp)NULL);
        return (ERROR); 
    }

    png_infop end_info = png_create_info_struct(png_ptr);
    if (!end_info) 
    { 
        png_destroy_read_struct(&png_ptr, &info_ptr,
          (png_infopp)NULL);
        return (ERROR);
    }

This example is present even in the 1.2.5 manual (2002), which is the oldest non-proprietary-format version still linked on libpng.org:  http://www.libpng.org/pub/png/libpng-1.2.5-manual.html .  I'm certain even 1.0 supported it; it's a fairly fundamental bit of logic.  I used it without ifdefs in the contrib/gregbook demo programs, and those date from 1999 (1.0.3 days).