1430942 - Likely write beyond bounds in OmxDataDecoder::FillCodecConfigDataToOmx()

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[mozillabugs]

Attachment: 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.

Comment 1

[mozillabugs]

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 2

[Daniel Veditz [:dveditz]]

Gerald: who can look into this one?

Comment 3

[Gerald Squelart [:gerald] (he/him)]

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?

Comment 4

[Jean-Yves Avenard [:jya]]

I can, but this will be at the bottom of the queue. This code isnt used, and certainly not from Android devices.

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

[mozillabugs]

(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

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[mozillabugs]

> 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

[Ralph Giles (:rillian)]

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.

Comment 10

[u480271]

If we don't use the code, should we remove it from gecko?

Comment 11

[Ralph Giles (:rillian)]

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

[Ralph Giles (:rillian)]

See also bug 1430942.

Comment 13

[u480271]

Attachment: Tighten up handling codec specific config. r?gerald

MozReview-Commit-ID: 8GO6z7U1kx8

Comment 14

[u480271]

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.

Comment 15

[u480271]

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?

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be154f4de4e9f53e8cdd5363d5a923794cb72174

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/df08d5fbb370