memory leak in png_create_info_struct

[zerokeeper]

Hi,libpng team. there is a memory leak in the file png.c:368 of function png_create_info_struct.
the bug is trigered by ./pngcp poc /dev/null

libpng_poc.zip

the asan debug info is as follows:

=================================================================
==10300==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 360 byte(s) in 1 object(s) allocated from:
#0 0x7fe088bf9602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
#1 0x422f95 in png_create_info_struct /root/fuzz/libpng-1.6.36/png.c:368

SUMMARY: AddressSanitizer: 360 byte(s) leaked in 1 allocation(s).

https://github.com/glennrp/libpng/blob/eddf9023206dc40974c26f589ee2ad63a4227a1e/png.c#L352-L376

[carnil]

CVE-2019-6129 was assigned for this issue.

[pgajdos]

$ pngcp libpng_poc /dev/null
libpng_poc: error(libpng): read: Not a PNG file

=================================================================
==30270==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 360 byte(s) in 1 object(s) allocated from:
    #0 0x7f07e0ecced0 in malloc (/usr/lib64/libasan.so.5+0xebed0)
    #1 0x7f07e1d3c23f in png_malloc_base /usr/src/debug/libpng16-1.6.36-0.x86_64/pngmem.c:95
    #2 0x7f07e1d24d55 in png_create_info_struct /usr/src/debug/libpng16-1.6.36-0.x86_64/png.c:368
    #3 0x55806e30a07b in read_png contrib/tools/pngcp.c:1775
    #4 0x55806e30d0b1 in cp_one_file contrib/tools/pngcp.c:2180
    #5 0x55806e30dba4 in cppng contrib/tools/pngcp.c:2288
    #6 0x55806e30e081 in main contrib/tools/pngcp.c:2351
    #7 0x7f07e0a43fea in __libc_start_main (/lib64/libc.so.6+0x22fea)

SUMMARY: AddressSanitizer: 360 byte(s) leaked in 1 allocation(s).
$

Yes, pngcp does not call png_destroy_info_struct() in error case. I think this is not a security issue at all.

[ctruta]

There are various issues with pngcp. Just FYI, in libpng-1.6.37 I will still focus on fixing core libpng issues. I plan to address the issues with 3rd-party contributed code (like pngcp) after 1.6.37.

[hlef]

right, there is a memory leak but the security impact is extremely low if not absent:

• the memory is leaked during abort(), so in the vast majority of cases there's no leak at all, memory is going to be released by the operating system anyways

• leaked memory is allocated by pngcp which passes a pointer to the library, so even if the process does not abort this is not an issue in the library but rather in the code using it. I don't think it is libpng's job to free this buffer

[ware]

@carnil, I know this has been closed but is there an update for CVE-2019-6129? It doesn't seem to reflect this discussion.

[carnil]

@ware, I do not know. I was basically only the messenger of the CVE while reviewing the CVE feed from MITRE and posting the reference here. If an update to the description or entry in general is needed then this could be submitted by requesting it via https://cveform.mitre.org/ .

[ware]

Thnks @carnil. I submitted a request to MITRE.

[xiao623]

Hi, is this issue solved?

[xteema]

Hello, is this issue solved?

[renkeEdogawa]

Hi,has the vulnerability CVE-2019-6129 been fixed?

[jbowler]

#293 is a duplicate of this. It contains this proposed fix:

openembedded/openembedded-core@38c6b26

However there is no bug to fix here; the memory is freed immediately because the program exits immediately.

The fix certainly looks completely wrong; the relevant structure is destroyed in display_clean_write and that is called from display_clean. Destroying it in display_clean_read will cause read_png to destroy it on exit which will mean it doesn't exist in cp_one_file at the point where write_png is called.

On testing the fix it simply doesn't work; pngcp stops copying anything; look at the first line in the implementation of png_write_png...

Anyway, a somewhat better test:

./pngcp libpng_poc ../code/pngbar.png dir/

shows that while the png_info is not destroyed with a single bad argument (which does not matter because the program exits immediately) with a second good argument the png_info is passed to write_png and used there but a png_info is dropped (i.e. not deleted) on the bogus PNG.

The real fix is trivial but the bug doesn't exactly strike me as worrying; pngcp is an undocumented internal test program. It drops one struct per broken file, so an exploit would have to find a way of running pngcp with a humongous number of command line arguments and it would eventually OOM, maybe. It's ridiculous for this to be in a CVE; it's just a way of getting a program to run out of memory with arbitrary input, for example:

./pngcp . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .

etc (try it; I really do mean "."). It might just OOM on an IoT device with very limited memory, but then consider this:

awk 'BEGIN{i=0; while(1) {; a[i]=++i; }; }'

It gets Killed after about 1 minute on my Linux machine sometime after growing to 64GByte, on my Windows machine it takes a longer; it looks like it is getting paged out.

So the conclusion is that the proposed fix basically destroys the functionality of pngcp and, anyway, it's not worth worrying about.

[jbowler]

EDIT: revised patch.

There is a much more pervasive version of the same behavior:

pngcp --nowrite --search *.png

or to make it really bad:

find / -name '*.png' -print0 | xargs -0 pngcp --nowrite --search

This is because the search code sets dp->tsp to non-zero and it doesn't get reset, so the png_info_struct does not get freed. This happens with well formed PNG files. It's still not a CVE but it is certainly a leak. My previous patch (removed) did not handle this and was somewhat messy. This new version moves the free out of write_png into cppng and removes the check on dp->tsp

The patch retains the behaviour whereby an error before dp->write_pp is successfully created results in a "hanging" png_info which is then cleaned inside the next read_png. I don't want to change this because it would remove some of the value of pngcp.c as an example and require a more complex fix.

contrib-tools-pngcp-c-269.patch.txt

[ctruta]

I forgot to close this issue. (Closing now.) The fix is in libpng-1.6.39.