Found while fuzzing a windows build. mozglue!mozalloc_abort+0x2a mozglue!mozalloc_handle_oom+0x61 mozglue!moz_xmalloc+0x1f xul!stagefright::SampleTable::setTimeToSampleParams+0x8c xul!stagefright::MPEG4Extractor::parseChunk+0x18e7 xul!stagefright::MPEG4Extractor::parseChunk+0x2155 verifier!AVrfpDphRemoveEntry+0x10b ntdll!RtlpAllocateHeapInternal+0x27b

Memory allocations for which the size is based on media data are now fallible, and therefore no OOM should happen there. Ideally we should be able to pre-check that the source has enough data before we even allocate an output buffer, but this is quite the rabbit hole, with stacks of calls through abstraction layers, some going into files, etc. So I don't think it's worth the effort here.

Comment on attachment 8783272 [details] [diff] [review] 1296473.patch Review of attachment 8783272 [details] [diff] [review]: ----------------------------------------------------------------- can we have a mochitest using the sample please? so that the rust one can be checked against? I think we shouldn't be using mozilla::fallible here. ::: media/libstagefright/frameworks/av/media/libstagefright/ESDS.cpp @@ +32,5 @@ > mDecoderSpecificOffset(0), > mDecoderSpecificLength(0), > mObjectTypeIndication(0) { > + if (!mData) { > + mInitCheck = ERROR_BUFFER_TOO_SMALL; I'm guessing it's missing a return here. ::: media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp @@ +251,5 @@ > return ERROR_MALFORMED; > } > > mSampleToChunkEntries = > + new (mozilla::fallible) SampleToChunkEntry[mNumSampleToChunkOffsets]; this is no mozilla code, shouldn't it use standard ways? that is void* operator new[] (std::size_t size, const std::nothrow_t& nothrow_value) noexcept; if std is allowed in this code that is.

Comment on attachment 8783272 [details] [diff] [review] 1296473.patch (In reply to Jean-Yves Avenard [:jya] from comment #3) > Comment on attachment 8783272 [details] [diff] [review] > 1296473.patch > > Review of attachment 8783272 [details] [diff] [review]: > ----------------------------------------------------------------- > > can we have a mochitest using the sample please? so that the rust one can be > checked against? Good idea, will do. Though this will only test one of the fallible calls, and may not exercise the fallibility of it (depending on how much memory our try machines have.) Removing review flag as it's not shippable as-is. > ::: media/libstagefright/frameworks/av/media/libstagefright/ESDS.cpp > @@ +35,2 @@ > > + if (!mData) { > > + mInitCheck = ERROR_BUFFER_TOO_SMALL; > > I'm guessing it's missing a return here. Oops, good catch, thank you. That will teach me to reverse an if at the last moment. > I think we shouldn't be using mozilla::fallible here. and > ::: media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp > @@ +254,2 @@ > > mSampleToChunkEntries = > > + new (mozilla::fallible) SampleToChunkEntry[mNumSampleToChunkOffsets]; > this is no mozilla code, shouldn't it use standard ways? > > that is void* operator new[] (std::size_t size, const std::nothrow_t& > nothrow_value) noexcept; > > if std is allowed in this code that is. We are already using FallibleTArray and its mozilla::fallible methods (from xpcom/glue/nsTArray.h), so we are already committed to Mozilla's memory framework, including 'operator new(size_t, mozilla::fallible_t)'. So for consistency I think we should not use a mix of mozilla::fallible and std::no_throw.

Added missing return. But keeping mozilla::fallible for consistency with its existing use with FallibleTArray (see comment 4 for details). Carrying r+ from comment 3.

Added (shortened, but sufficient) test case to gtest, which will test both libstagefright and the rust parser.

Comment on attachment 8783383 [details] [diff] [review] 1296473-p1-fallible-new.patch sec-approval? for both patches: [Security approval request comment] How easily could an exploit be constructed based on the patch? I would actually argue that this is *not* a real security issue: Either 'new' allocations OOM and Firefox crashes, or they work and the next instruction will fail gracefully because there is not as much media data available as requested. Now, it would be fairly easy to create (non-exploitable) crashing files based on the patch. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit description points at checking memory allocations. Which older supported branches are affected by this flaw? All, since libstagefright was introduced. If not all supported branches, which bug introduced the flaw? - Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should be applicable as-is to all branches. How likely is this patch to cause regressions; how much testing does it need? All it does is make previously-infallible allocations fallible, and tests them, so I don't think regressions should happen (crashing videos would just not play now). Tests include existing media tests and new gtest for this case.

Comment on attachment 8783383 [details] [diff] [review] 1296473-p1-fallible-new.patch Agree this is a DoS so I unhid the bug. sec-approval+ anyway since it was asked.

Thank you.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9562e8442f Check stagefright memory allocations. r=jya https://hg.mozilla.org/integration/mozilla-inbound/rev/1f54d63a02c3 Add test case to gtest. r=jya

https://hg.mozilla.org/mozilla-central/rev/fe9562e8442f https://hg.mozilla.org/mozilla-central/rev/1f54d63a02c3