heap buffer over-read and overflow is found in extractData (movDemuxer.cpp)

[iwashiira]

Our fuzzer found heap buffer over-read and heap buffer overflow in MovDemuxer. in the current master(75c9cb3). PoC is here.

#include "bufferedReaderManager.h"
#include "vod_common.h"
#include "abstractDemuxer.h"
#include "movDemuxer.h"
#include <cstdint>
#include <fs/systemlog.h>

using namespace std;

BufferedReaderManager readManager(2, DEFAULT_FILE_BLOCK_SIZE, DEFAULT_FILE_BLOCK_SIZE + MAX_AV_PACKET_SIZE,
                                  DEFAULT_FILE_BLOCK_SIZE / 2);

int main(int argc, char* argv[]) {

    string fileName = argv[1];
    AbstractDemuxer* demuxer = new MovDemuxer(readManager);

    uint32_t fileBlockSize = demuxer->getFileBlockSize();
    demuxer->openFile(fileName);
    int64_t discardedSize = 0;
    DemuxedData demuxedData;
    map<int32_t, TrackInfo> acceptedPidMap;
    demuxer->getTrackList(acceptedPidMap);

    PIDSet acceptedPidSet;
    for (const auto& itr : acceptedPidMap) acceptedPidSet.insert(itr.first);

    for (unsigned i = 0; i < DETECT_STREAM_BUFFER_SIZE / fileBlockSize; i++)
            demuxer->simpleDemuxBlock(demuxedData, acceptedPidSet, discardedSize);

    return 0;
}

Folloing is an output of valgrind. vuln14.mov is in poc14.zip

==5877== Memcheck, a memory error detector
==5877== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==5877== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==5877== Command: ./build/tsMuxer/tsmuxer ./out/crash/vuln14.mov
==5877==
==5877== Warning: invalid file descriptor -1 in syscall close()
==5877== Invalid read of size 2
==5877==    at 0x48529E0: memmove (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==5877==    by 0x338908: MovParsedAudioTrackData::extractData(AVPacket*, unsigned char*, int) (movDemuxer.cpp:196)
==5877==    by 0x3334C2: MovDemuxer::simpleDemuxBlock(std::map<int, MemoryBlock, std::less<int>, std::allocator<std::pair<int const, MemoryBlock> > >&, std::set<int, std::less<int>, std::allocator<int> > const&, long&) (movDemuxer.cpp:777)
==5877==    by 0x2E6974: main (main.cpp:30)
==5877==  Address 0x5145a40 is 4 bytes after a block of size 31,356 alloc'd
==5877==    at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==5877==    by 0x28F5FA: __gnu_cxx::new_allocator<unsigned char>::allocate(unsigned long, void const*) (new_allocator.h:127)
==5877==    by 0x28F4D0: std::allocator_traits<std::allocator<unsigned char> >::allocate(std::allocator<unsigned char>&, unsigned long) (alloc_traits.h:464)
==5877==    by 0x28F387: std::_Vector_base<unsigned char, std::allocator<unsigned char> >::_M_allocate(unsigned long) (stl_vector.h:346)
==5877==    by 0x28EFBB: std::vector<unsigned char, std::allocator<unsigned char> >::_M_default_append(unsigned long) (vector.tcc:635)
==5877==    by 0x28EDC0: std::vector<unsigned char, std::allocator<unsigned char> >::resize(unsigned long) (stl_vector.h:940)
==5877==    by 0x333212: MovDemuxer::simpleDemuxBlock(std::map<int, MemoryBlock, std::less<int>, std::allocator<std::pair<int const, MemoryBlock> > >&, std::set<int, std::less<int>, std::allocator<int> > const&, long&) (movDemuxer.cpp:755)
==5877==    by 0x2E6974: main (main.cpp:30)
==5877==
==5877== Invalid read of size 1
==5877==    at 0x4852A10: memmove (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==5877==    by 0x338908: MovParsedAudioTrackData::extractData(AVPacket*, unsigned char*, int) (movDemuxer.cpp:196)
==5877==    by 0x3334C2: MovDemuxer::simpleDemuxBlock(std::map<int, MemoryBlock, std::less<int>, std::allocator<std::pair<int const, MemoryBlock> > >&, std::set<int, std::less<int>, std::allocator<int> > const&, long&) (movDemuxer.cpp:777)
==5877==    by 0x2E6974: main (main.cpp:30)
==5877==  Address 0x5145a44 is 8 bytes after a block of size 31,356 alloc'd
==5877==    at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==5877==    by 0x28F5FA: __gnu_cxx::new_allocator<unsigned char>::allocate(unsigned long, void const*) (new_allocator.h:127)
==5877==    by 0x28F4D0: std::allocator_traits<std::allocator<unsigned char> >::allocate(std::allocator<unsigned char>&, unsigned long) (alloc_traits.h:464)
==5877==    by 0x28F387: std::_Vector_base<unsigned char, std::allocator<unsigned char> >::_M_allocate(unsigned long) (stl_vector.h:346)
==5877==    by 0x28EFBB: std::vector<unsigned char, std::allocator<unsigned char> >::_M_default_append(unsigned long) (vector.tcc:635)
==5877==    by 0x28EDC0: std::vector<unsigned char, std::allocator<unsigned char> >::resize(unsigned long) (stl_vector.h:940)
==5877==    by 0x333212: MovDemuxer::simpleDemuxBlock(std::map<int, MemoryBlock, std::less<int>, std::allocator<std::pair<int const, MemoryBlock> > >&, std::set<int, std::less<int>, std::allocator<int> > const&, long&) (movDemuxer.cpp:755)
==5877==    by 0x2E6974: main (main.cpp:30)
==5877==
==5877==
==5877== HEAP SUMMARY:
==5877==     in use at exit: 33,715 bytes in 15 blocks
==5877==   total heap usage: 281 allocs, 266 frees, 4,467,261 bytes allocated
==5877==
==5877== LEAK SUMMARY:
==5877==    definitely lost: 1,136 bytes in 1 blocks
==5877==    indirectly lost: 32,579 bytes in 14 blocks
==5877==      possibly lost: 0 bytes in 0 blocks
==5877==    still reachable: 0 bytes in 0 blocks
==5877==         suppressed: 0 bytes in 0 blocks
==5877== Rerun with --leak-check=full to see details of leaked memory
==5877==
==5877== For lists of detected and suppressed errors, rerun with: -s
==5877== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 0 from 0)

It is caused by this place. https://github.com/justdan96/tsMuxer/blob/75c9cb3514815d07378007d36cc90c3f209e7b36/tsMuxer/movDemuxer.cpp#L187-L198

Since the value of buff + frameSize is not compared with srcEnd before memcpy, heap buffer over-read and heap buffer overflow of buff + frameSize - srcEnd bytes occur.

Ricerca Security, Inc.