Closed
Bug 1430942
Opened 5 years ago
Closed 5 years ago
Likely write beyond bounds in OmxDataDecoder::FillCodecConfigDataToOmx()
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | fixed |
People
(Reporter: mozillabugs, Assigned: u480271)
References
Details
(Keywords: sec-other)
Attachments
(2 files)
553.14 KB,
application/x-7z-compressed
|
Details | |
1.14 KB,
patch
|
mozbugz
:
review+
|
Details | Diff | Splinter Review |
OmxDataDecoder::FillCodecConfigDataToOmx() (dom\media\platforms\omx\OmdDataDecoder.cpp) appears able to write beyond bounds by memcpy()-ing to a buffer without first checking that the buffer is sufficiently long. The bug is in lines 618-20: 597: void 598: OmxDataDecoder::FillCodecConfigDataToOmx() 599: { 600: // Codec configure data should be the first sample running on Omx TaskQueue. 601: MOZ_ASSERT(mOmxTaskQueue->IsCurrentThreadIn()); 602: MOZ_ASSERT(!mMediaRawDatas.Length()); 603: MOZ_ASSERT(mOmxState == OMX_StateIdle || mOmxState == OMX_StateExecuting); 604: 605: 606: RefPtr<BufferData> inbuf = FindAvailableBuffer(OMX_DirInput); 607: RefPtr<MediaByteBuffer> csc; 608: if (mTrackInfo->IsAudio()) { 609: csc = mTrackInfo->GetAsAudioInfo()->mCodecSpecificConfig; 610: } else if (mTrackInfo->IsVideo()) { 611: csc = mTrackInfo->GetAsVideoInfo()->mExtraData; 612: } 613: 614: MOZ_RELEASE_ASSERT(csc); 615: 616: // Some codecs like h264, its codec specific data is at the first packet, not in container. 617: if (csc->Length()) { 618: memcpy(inbuf->mBuffer->pBuffer, 619: csc->Elements(), 620: csc->Length()); One source of |csc| is via |mCodecSpecificConfig|, which is set by, among other things, VorbisState::Init() (dom\media\ogg\OggCodecState.cpp). The attached Vorbis POC causes the Vorbis decoder to fill |mCodecSpecificConfig| with nearly 0x7fffffff bytes of data, the vast majority of which is read directly from the POC file itself. It is not clear exactly how large the destination buffer |inbuf->mBuffer->pBuffer| is, but presumably it is on the order of a few MB. Its size eventually comes from GonkOmxPlatformLayer::AllocateOmxBuffer(), which gets its size via the Android call OMX_ERRORTYPE err = GetParameter(OMX_IndexParamPortDefinition, &def, sizeof(OMX_PARAM_PORTDEFINITIONTYPE)); (see dom/media/platforms/omx/GonkOmxPlatformLayer.cpp). The buffer's size comes from |nBufferSize| in |OMX_PARAM_PORTDEFINITIONTYPE| (in media/openmax_il/il112/OMX_Component.h ), and isa |OMX_U32|. It is thus < 4 GB, but the code cannot work -- especially not on the mobile devices that typically use Android -- if it is not far smaller than that. Thus, the attached POC should cause FF on an Android device to crash with evidence of writing beyond bounds. (I don't have such a device, so I can't test it.) If this write occurs, mostly arbitrary binary attacker-provided data will be written over the heap. (About 1/255 th of the data, at the beginning, will be header data generated by XiphHeadersToExtradata () (dom\media\XiphExtradata.cpp), and thus not under the attacker's direct control). The attacker should be able to adjust the length of the attack data to overrun the buffer by any desired amount. Use the attached POC by unzipping it, starting FF, attaching a debugger, and setting a BP on VorbisState::Init()'s call to XiphHeadersToExtradata (). Then load the POC (which will take 20-30 minutes). Wait for the BP, and step over the call to XiphHeadersToExtradata (). Examine |mCodecSpecificConfig|. Notice that it contains slightly < 0x7fffffff bytes of data, the beginning of which is repeats of 0xff (header data, as noted above), but the vast majority of which is repeats of the string "Put attack code and data here! ". Now, if you're using an Android device, set a BP on OmxDataDecoder::FillCodecConfigDataToOmx(), proceed, and see whether the overwrite occurs on lines 618-20.
Updated•5 years ago
|
Flags: sec-bounty?
Updated•5 years ago
|
Group: core-security → media-core-security
Reporter | ||
Comment 1•5 years ago
|
||
BTW, you will need to test on a 64-bit FF/Android build, and probably need >= 8GB RAM. I can shrink the POC so that it should overrun a smaller buffer if you tell me what |nBufferSize| your build uses. This should allow you to explore the bug on a 32-bit build with less RAM.
Comment 3•5 years ago
|
||
I don't have an Android device handy, but the analysis looks correct to me. It should be easy to catch (test if csc->Length() > inbuf->mBuffer->nAllocLen), but I'm not sure what should be done after that to handle this error. Jean-Yves, would you be able to help with this one?
Flags: needinfo?(gsquelart) → needinfo?(jyavenard)
Comment 4•5 years ago
|
||
I can, but this will be at the bottom of the queue. This code isnt used, and certainly not from Android devices.
Flags: needinfo?(jyavenard)
Comment 5•5 years ago
|
||
The CSC is a few hundred bytes long, and its size is at most on a 32 bits integer. So testing with content greater than 8GB seems pointless to me as this is not a case that can happen. I'll have a closer look later. But the STR seems incorrect to me
Reporter | ||
Comment 6•5 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #5) > The CSC is a few hundred bytes long, and its size is at most on a 32 bits > integer. So testing with content greater than 8GB seems pointless to me as > this is not a case that can happen. I'll have a closer look later. But the > STR seems incorrect to me The POC puts slightly < 0x7fffffff bytes of data into the CSC via |mCodecSpecificConfig|, as you can see by running it under, eg., Windows. The content is not > 8GB, but you probably need >= 8 GB of RAM on the device to test the POC. > This code isnt used, and certainly not from Android devices. It seems that OmxDataDecoder must be used for something, since it was added at the end of 2015 (https://bugzilla.mozilla.org/show_bug.cgi?id=1224887 ) and has been updated often since then ( https://hg.mozilla.org/mozilla-central/log/tip/dom/media/platforms/omx/OmxDataDecoder.cpp ). Maybe its original implementer , https://bugzilla.mozilla.org/user_profile?user_id=466002 , has some ideas?
Comment 7•5 years ago
|
||
the code is compiled in, but it is not used. There's no OMX interface available on Android. We kept it up to date, because it would allow people to *maybe* use it on platform where OMX is available : such as Raspberry Pi. As the platforms that do support OMX are typically very under-powered CPU wise, having a HW acceleration is desired. Still, it requires to develop the OMX drivers for that platforms and make it usable under gecko, AFAIK no one has done so. Again, this code is *not* used, and is in fact unusable in its current state. There are two parts required for OMX to be usable in Gecko, we've only implemented one part.
Reporter | ||
Comment 8•5 years ago
|
||
> There are two parts required for OMX to be usable in Gecko, we've
> only implemented one part.
Thank you for looking into this issue.
Comment 9•5 years ago
|
||
Dan, could you please follow up on Gerald's suggestion from #3? The code isn't used, so no versions of Firefox are affected, but it's not good to have vulnerable code lying around in-tree. That means we can just fix it in m-c and let it ride the trains.
Assignee: nobody → dglastonbury
status-firefox58:
--- → unaffected
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Priority: -- → P3
Assignee | ||
Comment 10•5 years ago
|
||
If we don't use the code, should we remove it from gecko?
Comment 11•5 years ago
|
||
Usually, yes. Supporting RasberryPi, as Jean-Yves suggested, is valuable. The trick with removing it is remembering we used to have code if someone does show up to contribute drivers.
Comment 12•5 years ago
|
||
See also bug 1430942.
Assignee | ||
Comment 13•5 years ago
|
||
MozReview-Commit-ID: 8GO6z7U1kx8
Attachment #8953336 -
Flags: review?(gsquelart)
Updated•5 years ago
|
Attachment #8953336 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 14•5 years ago
|
||
There are two memcpy in platform/omx. One was protected by a MOZ_RELEASE_ASSERT, so I implemented the same behaviour for the unprotected memcpy.
Assignee | ||
Comment 15•5 years ago
|
||
Daniel, Jean-Yves says "Again, this code is *not* used, and is in fact unusable in its current state. There are two parts required for OMX to be usable in Gecko, we've only implemented one part.", can the security severity of this bug be reassessed?
Flags: needinfo?(dveditz)
Updated•5 years ago
|
Keywords: checkin-needed
![]() |
||
Comment 16•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/be154f4de4e9f53e8cdd5363d5a923794cb72174
Keywords: checkin-needed
![]() |
||
Comment 17•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df08d5fbb370
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•5 years ago
|
Group: media-core-security → core-security-release
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•