1352556 - Possible integer overflow in usage of MFGetAttributeSize results

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect)

Description

[Mats Palmgren (inactive)]

We have a number of calls to MFGetAttributeSize
http://searchfox.org/mozilla-central/search?q=MFGetAttributeSize&path=

MSDN says the out params are UINT32:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms703101(v=vs.85).aspx

However, we store them in int32_t here for example, without checking if it fits:
http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/media/gmp-clearkey/0.1/WMFH264Decoder.cpp#95-96

These values are then propagated as is:
http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/media/gmp-clearkey/0.1/VideoDecoder.cpp#175-179
and are multiplied to compute an allocation size:
http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/media/gmp-clearkey/0.1/VideoDecoder.cpp#234-237,245,249,255

The attempt to check overflow on line 245/249, bit it doesn't work since
"ySize + 2 * uSize" is using uint32_t, not uint64_t.  It also seems bad to call
SetStride/SetSize before the values have been checked to be OK.

I would recommend auditing all callers of MFGetAttributeSize to see how these
values are propagated and used in our code.

Comment 1

[Mats Palmgren (inactive)]

s/bit/but/

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like we will ship 53 with this issue.  I can track it for a bit in case we get a patch that we might want in a dot release. Otherwise we can wontfix and wait for 54. 

Maire, mats is recommending an audit here. Can you help find someone to take this on? I'm asking you since you are the owner for the component triage.

Comment 3

[Maire Reavy [:mreavy]]

Chris Pearce is the best person to talk to about this.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Chris - ping?  Or do you want to forward this?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah, we'd better check these, and the other cases where we call this function.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Validate output of MFGetAttributeSize

MozReview-Commit-ID: J1NirhKfTK6

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch for ESR52

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch for Beta54

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8863310 [details] [diff] [review]
Validate output of MFGetAttributeSize

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?

I don't think it would be easy to construct an exploit.

The concern this patch addresses is that we call a Windows function MFGetAttributeSize in the GMP process and the content process and use the return values to allocate memory buffers without sanity checking their values.

MFGetAttributeSize should return a frame size that's a function of the media file being decoded, which could be maliciously constructed.

In the GMP process case a malicious file could result in these values causing an integer overflow which would result in nsTArray::SetLength() failing, and since that's infallible, we'd end up with a crash.

In the content process cases, we pass these values to the Windows system decoders, so we're dependent on the system decoders handling malicious values. When we later copy frames that come out of the system decoders we sanity check the frame sizes, so any allocations *we* (as opposed to the system decoders) do based on these values is sanity checked.

So my only concern here would be whether MS's decoders are also checking for overflow.


* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I think the code makes it pretty easy to figure out that the sizes of frames is being sanity checked in this patch. I don't see how we can avoid that.


* Which older supported branches are affected by this flaw?

All supported branches.


* If not all supported branches, which bug introduced the flaw?

N/A


* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Rebased patches uploaded.


* How likely is this patch to cause regressions; how much testing does it need?

I think this patch is very low risk, as it's just sanitizing the width and height of video frames as stored in the encoded stream.

Comment 10

[Jean-Yves Avenard [:jya]]

Fairly certain we won't ever get there. Will provide analysis later today.

Comment 11

[Jean-Yves Avenard [:jya]]

Not looking at the GMP side of things yet, but seeing that the patch also modifies the normal H264 decoder.

The WMF decoder is used for decoding H264 and on some machines, to decode VP9.

If H264, the dimensions of the video is contained in the SPS NAL, and is decoded there:
https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/H264.cpp#390

Those dimensions are first sanitised via https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/H264.cpp#254

The WMF decoder then has another set of checks happening:
https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFDecoderModule.cpp#239

if the resolution is over 4096x2304 then the content will be rejected and we won't attempt decoding.

And finally, assuming after this 3rd round of barriers, someone still managed to get through.
When the video image is created in VideoData::CreateAndCopyData:
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaData.cpp#308
https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaData.cpp?q=%2Bfunction%3A%22mozilla%3A%3AValidateBufferAndPicture%28const+VideoData%3A%3AYCbCrBuffer+%26%2C+const+IntRect+%26%29%22&redirect_type=single#99

we have checked for overflows that the buffer could properly be created.

And to top it all, the WMF decoder itself will generate a decode error if the SPS is invalid (just like all other decoders).

This new patch adds a lot of new checks around dom/media/platforms/wmf, none of those are necessary.

For VP9, the dimensions come from the WebMDemuxer, which also performs checks to ensure that the resolution used is sane:
https://dxr.mozilla.org/mozilla-central/source/dom/media/webm/WebMDemuxer.cpp#347

Now, let's look at GMP code. There is indeed an invalid cast occurring there from uint32 to int32.

However, the int32 goes back into a uint32_t in SampleToVideoFrame
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp?q=VideoDecoder.cpp&redirect_type=direct#239

This code is only used on Windows Clearkey, the uint32_t->int32_t->uint32_t behaviour is well known.

And we have a check to ensure we have no overflow here:
https://dxr.mozilla.org/mozilla-central/source/media/gmp-clearkey/0.1/VideoDecoder.cpp?q=VideoDecoder.cpp&redirect_type=direct#253

So in all, I don't think we have a security risk here...
The patch can go (though I would argue that the changes to dom/media/platforms/wmf are totally unnecessary) but they don't hurt either.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ and I'll approve for branches since we're early. We should make sure trunk is green and happy before checkin though.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

No more planned releases coming for ESR45, so calling that wontfix.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49be8c224dea
https://hg.mozilla.org/releases/mozilla-esr52/rev/2095754e5751

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The ESR52 patch hit Windows Werror bustage in DXVA2Manager.cpp. From IRC discussion, it sounds like the patch may have other issues as well, so I've backed it out for now.
https://hg.mozilla.org/releases/mozilla-esr52/rev/b0d9b06d8a67

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Fixed patch for ESR52

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have uploaded a fixed version of the ESR52 patch. Can someone push it please; the previous broken version was a+d by Al Billings already.

Comment 19

[Carsten Book [:Tomcat]]

(In reply to Chris Pearce (:cpearce) from comment #18)
> I have uploaded a fixed version of the ESR52 patch. Can someone push it
> please; the previous broken version was a+d by Al Billings already.

no problem, landed!

https://hg.mozilla.org/releases/mozilla-esr52/rev/6d80ca63ff8b7a86c38c3f41a0cc25b8e5ef65f2

Comment 20

[Andrei Vaida [:avaida]]

Is there anything actionable here in terms of manual testing?

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Andrei Vaida [:avaida], Desktop Release QA – please ni? me from comment #20)
> Is there anything actionable here in terms of manual testing?

No, I don't think so.