CVE-2019-17371: memory leak in png_malloc_warn and png_create_info_struct · Issue #307 · glennrp/libpng

@zer0yu Hi, I had some tests on gif2png. But I need your help.

I used valgrind to check the memory leakage by executing valgrind --leak-check=full ./gif2png -r poc, and here is the output message.

==25395==
==25395== HEAP SUMMARY:
==25395==     in use at exit: 91,120 bytes in 136 blocks
==25395==   total heap usage: 602 allocs, 466 frees, 1,177,468 bytes allocated
==25395==
==25395== 8,704 bytes in 34 blocks are definitely lost in loss record 1 of 3
==25395==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25395==    by 0x10CC48: xalloc (memory.c:17)
==25395==    by 0x10BEF3: ReadImage (gifread.c:662)
==25395==    by 0x10C646: ReadGIF (gifread.c:218)
==25395==    by 0x10B0A1: processfile.part.0 (gif2png.c:707)
==25395==    by 0x109A2B: processfile (stdio2.h:97)
==25395==    by 0x109A2B: main (gif2png.c:987)
==25395==
==25395== 18,360 bytes in 51 blocks are definitely lost in loss record 2 of 3
==25395==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25395==    by 0x4E40F32: png_create_info_struct (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.34.0)
==25395==    by 0x109F9C: writefile (gif2png.c:157)
==25395==    by 0x10B29D: processfile.part.0 (gif2png.c:788)
==25395==    by 0x109A2B: processfile (stdio2.h:97)
==25395==    by 0x109A2B: main (gif2png.c:987)
==25395==
==25395== 64,056 bytes in 51 blocks are definitely lost in loss record 3 of 3
==25395==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==25395==    by 0x4E46E81: png_malloc_warn (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.34.0)
==25395==    by 0x4E40E8F: ??? (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.34.0)
==25395==    by 0x4E4A40C: png_create_read_struct_2 (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.34.0)
==25395==    by 0x4E4A460: png_create_read_struct (in /usr/lib/x86_64-linux-gnu/libpng16.so.16.34.0)
==25395==    by 0x109F86: writefile (gif2png.c:151)
==25395==    by 0x10B29D: processfile.part.0 (gif2png.c:788)
==25395==    by 0x109A2B: processfile (stdio2.h:97)
==25395==    by 0x109A2B: main (gif2png.c:987)
==25395==
==25395== LEAK SUMMARY:
==25395==    definitely lost: 91,120 bytes in 136 blocks
==25395==    indirectly lost: 0 bytes in 0 blocks
==25395==      possibly lost: 0 bytes in 0 blocks
==25395==    still reachable: 0 bytes in 0 blocks
==25395==         suppressed: 0 bytes in 0 blocks
==25395==
==25395== For counts of detected and suppressed errors, rerun with: -v
==25395== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

We can see the key function png_malloc_warn and png_create_info_struct. Although the program did not crash, the output message was similar to the error information offered by @zer0yu .

So I reviewed the gif2png source code, whose git-tag was 2.5.9 and commit id was b6c6bb. The function wirtefile of gif2png calls png_create_read_struct and png_create_info_struct. That will cause libpng to allocate memory. But png_destroy_read_struct is not called to free the memory after that.

Here is my patch to fixed it.

diff --git a/gif2png.c b/gif2png.c
index 8a3d1c1..25537a5
--- a/gif2png.c
+++ b/gif2png.c
@@ -311,6 +311,7 @@ static int writefile(struct GIFelement *s, struct GIFelement *e,
         (void)free(info_ptr);
         return 1;
     }
+    png_destroy_read_struct(&png_ptr, &info_ptr, NULL);

     /* Create and initialize the png_struct with the desired error handler
      * functions.  If you want to use the default stderr and longjump method,
diff --git a/gifread.c b/gifread.c
index 6995c3a..f48ab65
--- a/gifread.c
+++ b/gifread.c
@@ -727,9 +727,11 @@ ReadImage(FILE *fd, int x_off, int y_off, int width, int height,
                         goto fini;
                     } else {
                         (void)check_recover(true);
+                        (void)free(image);
                         return(true);
                     }
                 } else {
+                    (void)free(image);
                     return(true);
                 }
             }

And here is the new output message

==27072== HEAP SUMMARY:
==27072==     in use at exit: 0 bytes in 0 blocks
==27072==   total heap usage: 602 allocs, 602 frees, 1,177,468 bytes allocated
==27072==
==27072== All heap blocks were freed -- no leaks are possible
==27072==
==27072== For counts of detected and suppressed errors, rerun with: -v
==27072== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

0 error!

So my conclusion is that: the memory leakage is caused by gif2png but NOT libpng.
But I need some more information from @zer0yu to further confirm it.
My env:

Ubuntu 18.04 x86_64
valgrind-3.13.0
gcc 7.4
libpng-dev 1.6.34