gdraheim / zziplib

The ZZIPlib provides read access on ZIP-archives and unpacked data. It features an additional simplified API following the standard Posix API for file access
Other
62 stars 50 forks source link

There are memory leaks in zziplib <=v0.13.69 which is trigged in __zzip_parse_root_directory(in zzip/zip.c:427) #58

Closed kky0h closed 5 years ago

kky0h commented 6 years ago

There are memory leaks in zziplib <=v0.13.69 which is trigged in __zzip_parse_root_directory(in zzip/zip.c:427) I wrote a demo based on the documentation.

#include <stdio.h>
#include <stdlib.h>
#include <zzip/zzip.h>
static const char usage[] = 
{
    "zzip\n"
};

int main(int argc, char const *argv[])
{
    if (argc  <= 1)
    {
        printf(usage);
        exit(0);
    }

    ZZIP_DIR* dir = zzip_dir_open(argv[1],0);

    if (dir)
    {
        ZZIP_DIRENT dirent;
    if (zzip_dir_read(dir,&dirent))
        {
            printf("%s %i/%i\n", dirent.d_name, dirent.d_csize, dirent.st_size);
        }
    zzip_dir_close(dir);
    }
    return 0;
}

when i use https://github.com/Kingkingyoung/fuzz_test/blob/poc/zzip-memory-leak memory leak happened

=================================================================
==125971==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 76 byte(s) in 1 object(s) allocated from:
    #0 0x7f92614fc4d0 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xda4d0)
    #1 0x4016f3 in __zzip_parse_root_directory ../../zzip/zip.c:427
    #2 0x7f9260e61f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)

It seems hdr0(zzip/zip.c:427) doesn't free correctly in some cases.

kky0h commented 6 years ago

This was assigned CVE-2018-16548

jmoellers commented 6 years ago

I seem to be unable to reproduce the issue with the latest sources. Maybe the bug is already fixed? There IS a free(dir->hdr0) in zzip_dir_free()! I tried with valgrind and it says All heap blocks were freed -- no leaks are possible

kky0h commented 6 years ago

I tried with valgrind and it says(latest sources and old version)

~/testapp/zziplib-0.13.69/test$ valgrind --leak-check=full  ./fuzz_zzip zzip-memory-leak
==186527== Memcheck, a memory error detector
==186527== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==186527== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==186527== Command: ./fuzz_zzip zzip-memory-leak
==186527== 
==186527== 
==186527== HEAP SUMMARY:
==186527==     in use at exit: 76 bytes in 1 blocks
==186527==   total heap usage: 2 allocs, 1 frees, 196 bytes allocated
==186527== 
==186527== 76 bytes in 1 blocks are definitely lost in loss record 1 of 1
==186527==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==186527==    by 0x4012B1: __zzip_parse_root_directory (zip.c:427)
==186527==    by 0x4019D5: __zzip_dir_parse (zip.c:757)
==186527==    by 0x4018BE: zzip_dir_fdopen_ext_io (zip.c:715)
==186527==    by 0x401BBA: zzip_dir_open_ext_io (zip.c:831)
==186527==    by 0x401B3D: zzip_dir_open (zip.c:808)
==186527==    by 0x400B4D: main (fuzz_zzip.c:21)
==186527== 
==186527== LEAK SUMMARY:
==186527==    definitely lost: 76 bytes in 1 blocks
==186527==    indirectly lost: 0 bytes in 0 blocks
==186527==      possibly lost: 0 bytes in 0 blocks
==186527==    still reachable: 0 bytes in 0 blocks
==186527==         suppressed: 0 bytes in 0 blocks
==186527== 
==186527== For counts of detected and suppressed errors, rerun with: -v
==186527== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 2 from 1)

You can try again with the code I post above,and input file https://github.com/Kingkingyoung/fuzz_test/blob/poc/zzip-memory-leak After debugging with gdb, it seems in this case ,the p_reclen=0(zip.c:576), so the dir->hdr0 =0, zzip_dir_free() don't free hdr0, I 'm not sure whether it is correct,you can try again.

kky0h commented 6 years ago

Valgrind said 'a memory error detector' as you post. My sources are also git clone from the master tree.And I also test released version 0.13.68 and 0.13.69. All of them have this problem. I just use '-g' and '-static' when I compile the test code.

jmoellers commented 6 years ago

Yes, it was early morning ;-) I did test with ZIP files downloaded from the 'net, yours is a misformed one, __zzip_parse_root_directory() then takes an error return and the hdr0 is not free()'d.

carnil commented 5 years ago

@jmoellers are those the complete commits, to fix the reported issue? https://github.com/gdraheim/zziplib/commit/0e1dadb05c1473b9df2d7b8f298dab801778ef99, https://github.com/gdraheim/zziplib/commit/d2e5d5c53212e54a97ad64b793a4389193fec687 and https://github.com/gdraheim/zziplib/commit/9411bde3e4a70a81ff3ffd256b71927b2d90dcbb

jmoellers commented 5 years ago

On 04.10.2018 23:14, carnil wrote:

@jmoellers https://github.com/jmoellers are those the complete commits, to fix the reported issue? 0e1dadb https://github.com/gdraheim/zziplib/commit/0e1dadb05c1473b9df2d7b8f298dab801778ef99, d2e5d5c https://github.com/gdraheim/zziplib/commit/d2e5d5c53212e54a97ad64b793a4389193fec687 and 9411bde https://github.com/gdraheim/zziplib/commit/9411bde3e4a70a81ff3ffd256b71927b2d90dcbb

Yes.

Josef

carnil commented 5 years ago

@jmoellers

On 04.10.2018 23:14, carnil wrote: @jmoellers https://github.com/jmoellers are those the complete commits, to fix the reported issue? 0e1dadb <0e1dadb>, d2e5d5c <d2e5d5c> and 9411bde <9411bde> Yes. Josef

Ack, thanks for confirming. Possibly so this issue can be closed.