Likely write beyond bounds in OmxDataDecoder::FillCodecConfigDataToOmx()

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: mozillabugs, Assigned: kamidphish)

Tracking

({sec-other})

57 Branch
mozilla60
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 fixed)

Details

Attachments

(2 attachments)

Posted file bug_1022_poc_2.7z
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.
Flags: sec-bounty?
Group: core-security → media-core-security
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.
Gerald: who can look into this one?
Flags: needinfo?(gsquelart)
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)
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)
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
(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?
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.
> 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.
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
Priority: -- → P3
If we don't use the code, should we remove it from gecko?
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.
MozReview-Commit-ID: 8GO6z7U1kx8
Attachment #8953336 - Flags: review?(gsquelart)
Attachment #8953336 - Flags: review?(gsquelart) → review+
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.
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)
Flags: sec-bounty?
Flags: sec-bounty-
Flags: needinfo?(dveditz)
Keywords: sec-other
https://hg.mozilla.org/mozilla-central/rev/df08d5fbb370
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Group: media-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.