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

memory leak in png_malloc_warn and png_create_info_struct #307

Closed
zer0yu opened this issue Jul 11, 2019 · 20 comments
Closed

memory leak in png_malloc_warn and png_create_info_struct #307

zer0yu opened this issue Jul 11, 2019 · 20 comments

Comments

@zer0yu
Copy link

zer0yu commented Jul 11, 2019

Hi,libpng team. there are memory leaks in the function png_malloc_warn and png_create_info_struct, respectively.

I compiler gif2png to the 32-bit LSB version with ASAN. The software runs in the x86-64 Ubuntu 16.04 services.

the bug is trigered by ./gif2png -r poc.
libpng_poc.zip

the asan debug info is as follows:

=================================================================
==35676==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 64056 byte(s) in 51 object(s) allocated from:
#0 0x7ffff6f02602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
#1 0x7ffff6c43c0d in png_malloc_warn (/lib/x86_64-linux-gnu/libpng16.so.16+0xac0d)

Direct leak of 17544 byte(s) in 51 object(s) allocated from:
#0 0x7ffff6f02602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
#1 0x7ffff6c3e032 in png_create_info_struct (/lib/x86_64-linux-gnu/libpng16.so.16+0x5032)
#2 0x4039d8 in processfile (/home/zeroyu/target_gif2png/gif2png64+0x4039d8)
#3 0x40406d in main (/home/zeroyu/target_gif2png/gif2png64+0x40406d)
#4 0x7ffff688f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

Direct leak of 6656 byte(s) in 26 object(s) allocated from:
#0 0x7ffff6f02602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
#1 0x405df0 in xalloc (/home/zeroyu/target_gif2png/gif2png64+0x405df0)
#2 0x405953 in ReadImage (/home/zeroyu/target_gif2png/gif2png64+0x405953)
#3 0x404a6d in ReadGIF (/home/zeroyu/target_gif2png/gif2png64+0x404a6d)
#4 0x403647 in processfile (/home/zeroyu/target_gif2png/gif2png64+0x403647)
#5 0x40406d in main (/home/zeroyu/target_gif2png/gif2png64+0x40406d)
#6 0x7ffff688f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

Direct leak of 2048 byte(s) in 8 object(s) allocated from:
#0 0x7ffff6f02602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
#1 0x405df0 in xalloc (/home/zeroyu/target_gif2png/gif2png64+0x405df0)
#2 0x405953 in ReadImage (/home/zeroyu/target_gif2png/gif2png64+0x405953)
#3 0x4049cc in ReadGIF (/home/zeroyu/target_gif2png/gif2png64+0x4049cc)
#4 0x403647 in processfile (/home/zeroyu/target_gif2png/gif2png64+0x403647)
#5 0x40406d in main (/home/zeroyu/target_gif2png/gif2png64+0x40406d)
#6 0x7ffff688f82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: 90304 byte(s) leaked in 136 allocation(s).
[Inferior 1 (process 35676) exited with code 027]

@abergmann
Copy link

CVE-2019-17371 was assigned to this issue.

@zer0yu
Copy link
Author

zer0yu commented Oct 10, 2019

thx

@willson-chen
Copy link
Contributor

@zer0yu Hi, could you please offer more information about how to reproduce this bug.

I have cloned gif2png and checkout to tag 2.5.9, the latest version is 3.0.0 and is ported to Golang.

After compiling gif2png and extracting gif file(renamed to poc.gif) from your attachment, I executed ./gif2png -r poc and got a error message:

libpng error: Invalid palette length

and some files:

poc.p01
poc.p02
...
poc.p50
poc.png

poc.p01...poc.p50 are empty files.

But no message about memory leakage were generated.

@rossburton
Copy link
Contributor

Important detail missing: what version of libpng?

@willson-chen
Copy link
Contributor

willson-chen commented Oct 22, 2019

@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

@zer0yu
Copy link
Author

zer0yu commented Oct 22, 2019

Sorry, it’s my mistake.

My env:

Ubuntu 16.04 x86_64
gcc version 5.4.0 20160609
ASAN
libpng 1.6.37
gif2png 2.5.13

details

gif2png: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/l, for GNU/Linux 2.6.32,

gif2png-2.5.13.tar.gz

@anselmolsm
Copy link

I got hore the same results as this @willson-chen's comment, it does look like a gif2png issue, not libpng's.

Clear Linux 31360
valgrind-3.15.0
gcc version 9.2.1 20191019
libpng 1.6.37

@willson-chen
Copy link
Contributor

@ctruta Hi, could you please to offer some help?

@rossburton
Copy link
Contributor

Note that the reporter filed the same leak with gif2png but ESR closed it because he has "withdrawn" the version of gif2png written in C: https://gitlab.com/esr/gif2png/issues/8

@willson-chen
Copy link
Contributor

Note that the reporter filed the same leak with gif2png but ESR closed it because he has "withdrawn" the version of gif2png written in C: https://gitlab.com/esr/gif2png/issues/8

The gif2png v3.0 has poted to Golang. So I think ESR won't continue to maintain v2.x version.

@iskunk
Copy link

iskunk commented Mar 7, 2020

Does any such vulnerability currently exist in libpng proper? If not, could this issue be closed?

(Also, would anyone know if there is a way to withdraw the CVE, as well?)

My org is a downstream user of this library, and the existence of this "vulnerability" with no forthcoming fix is leading to confusion with respect to security reviews and auditing.

@willson-chen
Copy link
Contributor

willson-chen commented Mar 7, 2020

Does any such vulnerability currently exist in libpng proper? If not, could this issue be closed?

(Also, would anyone know if there is a way to withdraw the CVE, as well?)

My org is a downstream user of this library, and the existence of this "vulnerability" with no forthcoming fix is leading to confusion with respect to security reviews and auditing.

mitre.org manage the CVEs. You can jump to their website, choose Update a CVE Entry, fill the form to request mitre.org to withdraw this CVE.

I did that when I found the cause of this CVE. But mitre.org just appended a NOTE to the CVE description and no more response after that.

@iskunk
Copy link

iskunk commented Mar 7, 2020

Do they just need a number of requests, or will they want to hear from a project maintainer?

It seems like someone would need to say, authoritatively, that the advisory is in error.

@willson-chen
Copy link
Contributor

I have no idea. You can just ask them about it.

@theta682
Copy link
Contributor

theta682 commented Mar 8, 2020

I wrote e-mail to nvd@nist.gov and they marked CVE-2019-17371 as related only to gif2png project. So, now this CVE is not assigned to libpng. This issue can be closed now.

@iskunk
Copy link

iskunk commented Mar 9, 2020

@theta682, I appreciate your action, but the NVD page still names libpng 1.6.37 as the vulnerable software. Did they mention a time frame for updating the public record?

@theta682
Copy link
Contributor

theta682 commented Mar 9, 2020

If you check the history of CVE-2019-17371 you can see "Modified Analysis - 2/13/2020 11:38:51 AM". This change replaced cpe:2.3:a:libpng:libpng:1.6.37:*:*:*:*:*:*:* with cpe:2.3:a:gif2png_project:gif2png:*:*:*:*:*:*:*:*. Current status of this CVE reports problem in gif2png, not in libpng. However, on MITRE page it has the description of this CVE without specifying the product. Probablu this can be updated using the form on MITRE.

@iskunk
Copy link

iskunk commented Mar 10, 2020

Oh, I see; you had sent the e-mail in the past.

I've filled out the form on the MITRE site to request an update to the CVE's description. Hopefully that will be enough to get the matter resolved.

@iskunk
Copy link

iskunk commented Mar 11, 2020

All right, that was easy.

The CVE description has been updated, and now makes no mention of libpng. This issue can now officially be put to rest.

@ctruta
Copy link
Member

ctruta commented Mar 30, 2020

Thanks for looking into this all the way to the end. I'm closing the issue.

@ctruta ctruta closed this as completed Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants