#426 One possible null pointer dereference vulnerability in GraphicsMagick-1.3.25/coders/png.c

[shqking]

Hello all.

I found one possible null pointer dereference vulnerability in GraphicsMagick-1.3.25/coders/png.c, at function ReadMNGImage().

The vulnerable code snippet is shown as below.

 5038                        for (y=0; y < (long) image->rows; y++)
 5039                          {
 5040:                           q=GetImagePixels(image,0,y,image->columns,1);
 5041                            for (x=(long) image->columns; x > 0; x--)
 5042                              {
 ....
 5090                                             MemoryAllocationFailed,image)
 5091                      }
 5092:                   n=GetImagePixels(image,0,0,image->columns,1);
 5093                    (void) memcpy(next,n,row_length);
 5094                    for (y=0; y < (long) image->rows; y++)
 ....
 5109                        if (y < (long) image->rows-1)
 5110                          {
 5111:                           n=GetImagePixels(image,0,y+1,image->columns,1);
 5112                            (void) memcpy(next,n,row_length);
 5113                          }
 ....
 5213                    for (y=0; y < (long) image->rows; y++)
 5214                      {
 5215:                       q=GetImagePixels(image,0,y,image->columns,1);
 5216                        p=q+(image->columns-row_length);
 5217                        n=p+1;
 ....
 5306                        for (y=0; y < (long) image->rows; y++)
 5307                          {
 5308:                           q=GetImagePixels(image,0,y,image->columns,1);
 5309                            for (x=(long) image->columns; x > 0; x--)
 5310                              {

It is important to note that function GetImagePixels might return NULL if any exception happens.
Therefore, the subsequent usage of this NULL return value might cuase null pointer dereference.

As I have reviewed the usage of function GetImagePixels in other source files, its return value is usually checked with NULL, which is safe enough.
Also, one possible workaround I suggest for function ReadMNGImage is also to conduct safety checks for the return value of function GetImagePixels.

Thanks.

[Bob Friesenhahn]

Thanks for reporting this. I wonder why Coverity has not reported such obvious issues?

[shqking]

Thanks for your reply.

I want to note that this issue was found with Coverity, by a checker named NULL_RETURNS.
This checker aims to find many instances where a pointer or reference is checked against NULL and then later dereferenced.

Attached please find one possible workaround.
Thanks.

[Bob Friesenhahn]

Thanks for reporting this, and for the patch.