Add sanity check on af_get_page function to avoid an undefined behavior

[PracaGrande]

Hi,

While fuzzing AFFLib tools with AFL, I came a cross a segmentation fault. When using affinfo 3.7.16, to open a specially crafted AFF file, a programming error occurs in the function af_get_page(), in afflib_pages.cpp, that can cause a segmentation fault or an undefined behavior due to passing a fishy (possibly negative) value as argument of 'size' to the function malloc.

Below is the output of valgrind when running affinfo with the forged AFF file.

$ valgrind affinfo segfault2.aff
==914== Memcheck, a memory error detector
==914== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==914== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==914== Command: affinfo segfault2.aff
==914==
segfault2.aff is a AFF file
==914== Argument 'size' of function malloc has a fishy (possibly negative) value: -301989889
==914==    at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==914==    by 0x40582EE: af_get_page (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x40584EE: af_read_sizes (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x405683A: af_open_with (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4056ACE: af_open (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x804DA73: info_file(char const*) (affinfo.cpp:513)
==914==    by 0x804A508: main (affinfo.cpp:759)
==914==
==914== Invalid write of size 1
==914==    at 0x4030F43: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==914==    by 0x405E3D9: COutMemoryStream::Write(void const*, unsigned int, unsigned int*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4090B91: COutBuffer::FlushPart() (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4090C58: COutBuffer::FlushWithCheck() (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4085E97: NCompress::NLZMA::CDecoder::CodeSpec(unsigned int) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4086CA1: NCompress::NLZMA::CDecoder::CodeReal(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4084C59: NCompress::NLZMA::CDecoder::Code(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x405E0B5: lzma_uncompress (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4058251: af_get_page (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x40584EE: af_read_sizes (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x405683A: af_open_with (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4056ACE: af_open (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==914==
==914==
==914== Process terminating with default action of signal 11 (SIGSEGV)
==914==  Access not within mapped region at address 0x0
==914==    at 0x4030F43: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==914==    by 0x405E3D9: COutMemoryStream::Write(void const*, unsigned int, unsigned int*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4090B91: COutBuffer::FlushPart() (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4090C58: COutBuffer::FlushWithCheck() (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4085E97: NCompress::NLZMA::CDecoder::CodeSpec(unsigned int) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4086CA1: NCompress::NLZMA::CDecoder::CodeReal(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4084C59: NCompress::NLZMA::CDecoder::Code(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x405E0B5: lzma_uncompress (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4058251: af_get_page (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x40584EE: af_read_sizes (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x405683A: af_open_with (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==    by 0x4056ACE: af_open (in /usr/lib/i386-linux-gnu/libafflib.so.0.0.0)
==914==  If you believe this happened as a result of a stack
==914==  overflow in your program's main thread (unlikely but
==914==  possible), you can try to increase the size of the
==914==  main thread stack using the --main-stacksize= flag.
==914==  The main thread stack size used in this run was 8388608.
==914==

==914== HEAP SUMMARY:
==914==     in use at exit: 1,354,105 bytes in 1,223 blocks
==914==   total heap usage: 1,857 allocs, 634 frees, 1,370,424 bytes allocated
==914==
==914== LEAK SUMMARY:
==914==    definitely lost: 0 bytes in 0 blocks
==914==    indirectly lost: 0 bytes in 0 blocks
==914==      possibly lost: 0 bytes in 0 blocks
==914==    still reachable: 1,354,105 bytes in 1,223 blocks
==914==         suppressed: 0 bytes in 0 blocks
==914== Rerun with --leak-check=full to see details of leaked memory
==914==
==914== For counts of detected and suppressed errors, rerun with: -v
==914== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
Segmentation fault

After speaking with Phillip Hellewell about it, he suggested we should add the following sanity check:

if (af->image_pagesize <= 0 || af->image_pagesize > 16*1024*1024)
    return -1;

before the following snippet of code:

/* Now uncompress directly into the buffer provided by the caller, unless the caller didn't
 * provide a buffer. If that happens, allocate our own...
 */
int res = -1;   // 0 is success
bool free_data = false;
if(data==0){
    data = (unsigned char *)malloc(af->image_pagesize);
    free_data = true;
    *bytes = af->image_pagesize; // I can hold this much
}

Thanks, Luis

[sshock]

Something is wrong with the pull request, because instead of showing up as a modification to the existing lib/afflib_pages.cpp, it is showing up as a new file tools/afflib_pages.cpp.

[sshock]

Try one more time with a new pull request if you like, or let me know and I'll just take care of it.

[PracaGrande]

Hi Phillip , might be faster if you could do it please. Not sure what happened.

[sshock]

Not a problem. I will take care of it.

[sshock]

Fixed in 435a2ca.