If function png_read_image encounters error it will call longjmp and will
jump to place where was called setjmp. In such case pBuffer and pRows will
never be deallocated.

Cut of code from mentioned functions:
```
    // Read the file
    if( setjmp(png_jmpbuf(pPng)) )
    {
        png_destroy_read_struct(&pPng, &pInfo, (png_infopp)NULL);
        PODOFO_RAISE_ERROR( ePdfError_InvalidHandle );
    }

    long lLen = static_cast<long>(png_get_rowbytes(pPng, pInfo) * height);
    char* pBuffer = static_cast<char*>(podofo_calloc(lLen, sizeof(char)));
...
    png_bytepp pRows = static_cast<png_bytepp>(podofo_calloc(height,
sizeof(png_bytep)));
...
    png_read_image(pPng, pRows);
...
    podofo_free(pBuffer);
    podofo_free(pRows);
```

Test example:
```
// include podofo, vector, etc
int main()
{
    PdfMemDocument doc;
    PdfImage image(&doc);
    std::vector<unsigned char> corrupt_png = {
        0x89, 'P', 'N', 'G', 0x0D, 0x0A, 0x1A, 0x0A,
            0x00, 0x00, 0x00, 0x0D,
                'I', 'H', 'D', 'R',
                0x00, 0x00, 0x04, 0x00,
                0x00, 0x00, 0x05, 0xA9,
                0x08, 0x02, 0x00, 0x00, 0x00,
                0x68, 0x1B, 0xF7, 0x46,
            0x00, 0x00, 0x00, 0x00,
                'I', 'D', 'A', 'T',
                0x35, 0xAF, 0x06, 0x1E,
            0x00, 0x00, 0x00, 0x00,
                'I', 'E', 'N', 'D',
                0xAE, 0x42, 0x60, 0x82
    };
    try
    {
        image.LoadFromPngData(corrupt_png.data(), corrupt_png.size());
    }
    catch(PdfError &e)
    {
        e.PrintErrorMsg();
    }
    return 0;
}
```

Test for leaks using valgrind (libpng outputs errors to stderr as it is not
muted in podofo like libjpeg and libtiff):
```
libpng error: Not enough image data

PoDoFo encountered an error. Error: 2 ePdfError_InvalidHandle
Error Description: A NULL handle was passed, but initialized data was
expected.
Callstack:
#0 Error Source: podofo/doc/PdfImage.cpp:1142

==97240==
==97240== HEAP SUMMARY:
==97240==     in use at exit: 4,462,920 bytes in 2 blocks
==97240==   total heap usage: 128 allocs, 126 frees, 4,567,527 bytes
allocated
==97240==
==97240== 4,462,920 (11,592 direct, 4,451,328 indirect) bytes in 1 blocks
are definitely lost in loss record 2 of 2
==97240==    at 0x4C31B25: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==97240==    by 0x1759CE: PoDoFo::podofo_calloc(unsigned long, unsigned
long)
==97240==    by 0x1E0EE4: PoDoFo::PdfImage::LoadFromPngData(unsigned char
const*, long)
==97240==    by 0x125610: main
==97240==
==97240== LEAK SUMMARY:
==97240==    definitely lost: 11,592 bytes in 1 blocks
==97240==    indirectly lost: 4,451,328 bytes in 1 blocks
==97240==      possibly lost: 0 bytes in 0 blocks
==97240==    still reachable: 0 bytes in 0 blocks
==97240==         suppressed: 0 bytes in 0 blocks
```

There is png image which advertises that it has dimensions 1024x1449 and
RGB colorspace so after decompression it will take 1024 * 1449 * 3 =
4,451,328 bytes of memory as pBuffer. It has height 1449 so there are
allocated 1449 row pointers which takes 1449 * 8 = 11,592 bytes as pRows.
Buffer pBuffer is reported as indirect loss because there are pointers
in pRows which points to it.

This leak is especially dangerous in server applications that uses podofo
as library. Let say such application takes as inputs from network pdf and
image then inserts image into pdf and sends resulting pdf back. Attacker
can repeatedly send crafted png images like in test example above to
exhaust memory and cause crash.


Also line "if( setjmp(png_jmpbuf(pPng)) )" is probably undefined behaviour
(there should be comparison with integer constant):

https://en.cppreference.com/w/cpp/utility/program/setjmp


Also function pngReadData should call png_error in case it cannot read
requested length of data but instead it will read only available data or do
nothing (if _size == _pos):

```
        if (length > _size - _pos)
        {
            memcpy(data, &_data[_pos], _size - _pos);
            _pos = _size;
        }
        else
        {
            memcpy(data, &_data[_pos], length);
            _pos += length;
        }
```


Also are you sure that throwing C++ exceptions from function which is
called from C code in another library (libjpeg) is not undefined behaviour
or something? Maybe in PdfImage::LoadFromJpegData should be used
setjmp/longjmp mechanism too:

```
extern "C" {
void JPegErrorExit(j_common_ptr cinfo)
{
#if 1
    char buffer[JMSG_LENGTH_MAX];

    /* Create the message */
    (*cinfo->err->format_message) (cinfo, buffer);
#endif
    jpeg_destroy(cinfo);
    PODOFO_RAISE_ERROR_INFO( ePdfError_UnsupportedImageFormat, buffer);
}
```
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users

Reply via email to