Skip to content

Fuzzing fixes#95

Open
bobsayshilol wants to merge 8 commits into
TechSmith:mainfrom
bobsayshilol:ts-fuzzing-fixes
Open

Fuzzing fixes#95
bobsayshilol wants to merge 8 commits into
TechSmith:mainfrom
bobsayshilol:ts-fuzzing-fixes

Conversation

@bobsayshilol

Copy link
Copy Markdown

This PR is basically a copy of enzo1982/mp4v2#63 brought over because this fork seems more active. I checked each commit and only the ones that actually fixed issues in this fork were kept. There's also 2 new fixes that were found when running against the old corpus (the last 2 commits).

See individual commits for more details. Sanitizer output in summaries below:

edaad04 - Avoid leaking memory if |new MP4BytesProperty()| throws
Direct leak of 136 byte(s) in 1 object(s) allocated from:
    #1 0x00000060e9d9 in mp4v2::impl::MP4Atom::factory(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:1045:12
    #2 0x00000060b26b in mp4v2::impl::MP4Atom::CreateAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:78:21
    #3 0x000000611f5a in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:175:22
    #4 0x0000006147c0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
    #5 0x00000061395b in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:247:11
    #6 0x00000061f1db in mp4v2::impl::MP4File::ReadFromFile() /mp4v2/src/mp4file.cpp:431:18
    #7 0x00000061e3e3 in mp4v2::impl::MP4File::Read(char const*, MP4FileProvider_s const*) /mp4v2/src/mp4file.cpp:98:5
    #8 0x0000005ee04b in MP4ReadProvider /mp4v2/src/mp4.cpp:133:16
c5d62c9 - Fix an OOM and stack overflow
==62817==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc6c460ff8 (pc 0x00000059c975 bp 0x000000000001 sp 0x7ffc6c461000 T0)
    #4 0x000000609239 in mp4v2::impl::MP4Realloc(void*, unsigned int) /mp4v2/src/mp4util.h:90:18
    #5 0x0000006764eb in mp4v2::impl::MP4Integer8Array::Resize(unsigned int) /mp4v2/src/mp4array.h:126:1
    #6 0x0000006764eb in mp4v2::impl::MP4Integer8Property::SetCount(unsigned int) /mp4v2/src/mp4property.h:202:1
    #7 0x000000617833 in mp4v2::impl::MP4Integer8Property::MP4Integer8Property(mp4v2::impl::MP4Atom&, char const*) /mp4v2/src/mp4property.h:202:1
    #8 0x000000617833 in mp4v2::impl::MP4Atom::AddVersionAndFlags() /mp4v2/src/mp4atom.cpp:607:21
    #9 0x00000071eebe in mp4v2::impl::MP4UrlAtom::MP4UrlAtom(mp4v2::impl::MP4File&, char const*) /mp4v2/src/atom_url.cpp:32:5
    #10 0x00000060d440 in mp4v2::impl::MP4Atom::factory(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:1021:28
    #11 0x00000060b26b in mp4v2::impl::MP4Atom::CreateAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:78:21
    #12 0x000000611f5f in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:175:22
    #13 0x0000006147e0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
    #14 0x00000061397b in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:247:11
    #15 0x0000006f59b2 in mp4v2::impl::MP4DrefAtom::Read() /mp4v2/src/atom_dref.cpp:47:14
    #16 0x000000612573 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:202:10
    #17 0x0000006147e0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
    #18 0x00000061397b in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:247:11
    #19 0x000000612573 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:202:10
    #20 0x0000006147e0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
    ...
    #243 0x00000061397b in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:247:11
    #244 0x000000612573 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:202:10
    #245 0x0000006147e0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
    #246 0x00000061397b in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:247:11
    #247 0x000000612573 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:202:10
    #248 0x0000006147e0 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:451:31
1accd5d - Fix crash on nullptr
/mp4v2/src/mp4.cpp:2570:22: runtime error: load of null pointer of type 'uint32_t' (aka 'unsigned int')
    #0 0x0000005fe4f4 in MP4FreeH264SeqPictHeaders /mp4v2/src/mp4.cpp:2570:22
9aacbe7 - Fix read/write out of bounds due to off-by-one error
==66391==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7d17d25e3900 at pc 0x000000573ab7 bp 0x7ffef50fdf20 sp 0x7ffef50fd6b0
WRITE of size 39 at 0x7d17d25e3900 thread T0
    #1 0x000000661855 in MP4Info /mp4v2/src/mp4info.cpp:590:21

0x7d17d25e3900 is located 0 bytes after 4096-byte region [0x7d17d25e2900,0x7d17d25e3900)
allocated by thread T0 here:
    #1 0x0000005ece83 in mp4v2::impl::MP4Malloc(unsigned long) /mp4v2/src/mp4util.h:61:15
    #2 0x0000006617ac in mp4v2::impl::MP4Calloc(unsigned long) /mp4v2/src/mp4util.h:70:19
    #3 0x0000006617ac in MP4Info /mp4v2/src/mp4info.cpp:580:31
e93427b - Bounds check tag sizes before attempting to read them
==69725==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7bc7d19e15d1 at pc 0x0000006c91a7 bp 0x7fffd0ce5390 sp 0x7fffd0ce5388
READ of size 1 at 0x7bc7d19e15d1 thread T0
    #0 0x0000006c91a6 in mp4v2::impl::itmf::Tags::fetchInteger(CodeItemMap const&, std::string const&, unsigned short&, unsigned short const*&) /mp4v2/src/itmf/Tags.cpp:524:21
    #1 0x0000006c52ce in mp4v2::impl::itmf::Tags::c_fetch(MP4Tags_s*&, void*) /mp4v2/src/itmf/Tags.cpp:100:5
    #2 0x0000005d5d38 in MP4TagsFetch /mp4v2/src/cmeta.cpp:133:14

0x7bc7d19e15d1 is located 0 bytes after 1-byte region [0x7bc7d19e15d0,0x7bc7d19e15d1)
allocated by thread T0 here:
    #1 0x0000005ecb93 in mp4v2::impl::MP4Malloc(unsigned long) /mp4v2/src/mp4util.h:61:15
    #2 0x00000062b811 in mp4v2::impl::MP4BytesProperty::GetValue(unsigned char**, unsigned int*, unsigned int) /mp4v2/src/mp4property.h:413:30
    #3 0x0000006bbdae in mp4v2::impl::itmf::(anonymous namespace)::__itemAtomToModel(mp4v2::impl::MP4ItemAtom&, MP4ItmfItem_s&) /mp4v2/src/itmf/generic.cpp:206:28
    #4 0x0000006baf7a in mp4v2::impl::itmf::genericGetItems(mp4v2::impl::MP4File&) /mp4v2/src/itmf/generic.cpp:308:9
    #5 0x0000006c4a58 in mp4v2::impl::itmf::Tags::c_fetch(MP4Tags_s*&, void*) /mp4v2/src/itmf/Tags.cpp:72:33
    #6 0x0000005d5d38 in MP4TagsFetch /mp4v2/src/cmeta.cpp:133:14

^ this fixes #90

7f5d4ed - Plug a leak if an exception is thrown
Direct leak of 2064 byte(s) in 2 object(s) allocated from:
    #1 0x0000005ecb93 in mp4v2::impl::MP4Malloc(unsigned long) /mp4v2/src/mp4util.h:61:15
    #2 0x00000063fa50 in mp4v2::impl::MP4File::GetChapters(MP4Chapter_s**, unsigned int*, MP4ChapterType) /mp4v2/src/mp4file.cpp:2650:50
    #3 0x0000005f7199 in MP4GetChapters /mp4v2/src/mp4.cpp:1597:43
ccc1d7e - Assign array size after reallocation
==109623==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7bc400fe02d8 at pc 0x00000066766a bp 0x7ffddb3ca580 sp 0x7ffddb3ca578
READ of size 8 at 0x7bc400fe02d8 thread T0
    #0 0x000000667669 in mp4v2::impl::MP4StringProperty::~MP4StringProperty() /mp4v2/src/mp4property.cpp:338:17
    #1 0x000000667669 in mp4v2::impl::MP4StringProperty::~MP4StringProperty() /mp4v2/src/mp4property.cpp:335:1
    #2 0x00000060e773 in mp4v2::impl::MP4Atom::~MP4Atom() /mp4v2/src/mp4atom.cpp:66:9
    #3 0x0000006ed553 in mp4v2::impl::MP4FtypAtom::~MP4FtypAtom() /mp4v2/src/atoms.h:344:7
    #4 0x0000006172d2 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:209:3
    #5 0x000000618a20 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:454:31
    #6 0x000000617bb6 in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:250:11
    #7 0x0000006e9992 in mp4v2::impl::MP4DrefAtom::Read() /mp4v2/src/atom_dref.cpp:47:14
    #8 0x000000616e66 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:205:10
    #9 0x000000618a20 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:454:31
    #10 0x000000617bb6 in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:250:11
    #11 0x000000622a42 in mp4v2::impl::MP4File::ReadFromFile() /mp4v2/src/mp4file.cpp:431:18
    #12 0x000000621c13 in mp4v2::impl::MP4File::Read(char const*, MP4FileProvider_s const*) /mp4v2/src/mp4file.cpp:98:5
    #13 0x0000005f365b in MP4ReadProvider /mp4v2/src/mp4.cpp:133:16

0x7bc400fe02d8 is located 0 bytes after 8-byte region [0x7bc400fe02d0,0x7bc400fe02d8)
allocated by thread T0 here:
    #1 0x00000060dc7f in mp4v2::impl::MP4Realloc(void*, unsigned int) /mp4v2/src/mp4util.h:90:18
    #2 0x000000667933 in mp4v2::impl::MP4StringArray::Resize(unsigned int) /mp4v2/src/mp4array.h:138:1
    #3 0x00000066774e in mp4v2::impl::MP4StringProperty::SetCount(unsigned int) /mp4v2/src/mp4property.cpp:350:14
    #4 0x000000667294 in mp4v2::impl::MP4StringProperty::MP4StringProperty(mp4v2::impl::MP4Atom&, char const*, bool, bool, bool) /mp4v2/src/mp4property.cpp:330:5
    #5 0x0000006ecd97 in mp4v2::impl::MP4FtypAtom::MP4FtypAtom(mp4v2::impl::MP4File&) /mp4v2/src/atom_ftyp.cpp:32:31
    #6 0x0000006117c7 in mp4v2::impl::MP4Atom::factory(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:895:28
    #7 0x00000060fbf1 in mp4v2::impl::MP4Atom::CreateAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*, char const*) /mp4v2/src/mp4atom.cpp:78:21
    #8 0x0000006167d9 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:178:22
    #9 0x000000618a20 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:454:31
    #10 0x000000617bb6 in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:250:11
    #11 0x0000006e9992 in mp4v2::impl::MP4DrefAtom::Read() /mp4v2/src/atom_dref.cpp:47:14
    #12 0x000000616e66 in mp4v2::impl::MP4Atom::ReadAtom(mp4v2::impl::MP4File&, mp4v2::impl::MP4Atom*) /mp4v2/src/mp4atom.cpp:205:10
    #13 0x000000618a20 in mp4v2::impl::MP4Atom::ReadChildAtoms() /mp4v2/src/mp4atom.cpp:454:31
    #14 0x000000617bb6 in mp4v2::impl::MP4Atom::Read() /mp4v2/src/mp4atom.cpp:250:11
    #15 0x000000622a42 in mp4v2::impl::MP4File::ReadFromFile() /mp4v2/src/mp4file.cpp:431:18
    #16 0x000000621c13 in mp4v2::impl::MP4File::Read(char const*, MP4FileProvider_s const*) /mp4v2/src/mp4file.cpp:98:5
    #17 0x0000005f365b in MP4ReadProvider /mp4v2/src/mp4.cpp:133:16
ab04a28 - Cast to the correct property type
/mp4v2/src/descriptors.cpp:508:10: runtime error: downcast of address 0x7bcbdea19e00 which does not point to an object of type 'MP4Integer8Property'
0x7bcbdea19e00: note: object is of type 'mp4v2::impl::MP4BitfieldProperty'
 00 00 00 00  50 b6 88 00 00 00 00 00  50 19 a0 de 3b 7c 00 00  c0 b6 8b 00 00 00 00 00  00 00 be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'mp4v2::impl::MP4BitfieldProperty'
    #0 0x0000007d29f9 in mp4v2::impl::MP4ContentIdDescriptor::Read(mp4v2::impl::MP4File&) /mp4v2/src/descriptors.cpp:508:10

See comment for why this change is necessary.
MP4File::GetTrackH264SeqPictHeaders() can return early if it fails to
parse or allocate, which means that MP4GetTrackH264SeqPictHeaders() can
return success and look like the pointers need freeing even though some
may still be nullptrs.

To fix this, guard the derefs.
We need an extra byte for the terminating NUL, otherwise the strncat()
can write past the end or the caller can read past the end of the
string.
ASAN flags up that these read out of bounds on bad data.
MP4ConvertTime() can throw on bad data which ends up leaking |chapters|
since it's not owned by anything. Add some RAII to plug that leak.
If the reallocation or overflow check fails then an exception will be
thrown, leaving the container thinking that it has the new size when
it's still using the old data. This lead to cases where we'd try and
free memory that we didn't allocate.
This is created as a 2 bit MP4BitfieldProperty, so read it as that.
@bobsayshilol

Copy link
Copy Markdown
Author

Didn't think to search the closed issues but it looks like #74 (comment) was closed incorrectly. The trace shown there matches that of Assign array size after reallocation (ccc1d7e) above, which I now see is very similar to enzo1982/mp4v2@35c20e02. Looks like that CVE fix was missed from #76.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

src/Tags.cpp:480 Heap Buffer Overflow in mp4v2::impl::itmf::Tags::fetchTrack

1 participant