Closed Bug 1352556 Opened 4 years ago Closed 4 years ago
Possible integer overflow in usage of MFGet
Attribute Size results
10.71 KB, patch
|Details | Diff | Splinter Review|
10.68 KB, patch
|Details | Diff | Splinter Review|
10.66 KB, patch
|Details | Diff | Splinter Review|
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.
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.
Chris Pearce is the best person to talk to about this.
Flags: needinfo?(mreavy) → needinfo?(cpearce)
Chris - ping? Or do you want to forward this?
Yeah, we'd better check these, and the other cases where we call this function.
Assignee: nobody → cpearce
Attachment #8863310 - Flags: review?(gsquelart)
Attachment #8863310 - Flags: review?(gsquelart) → review+
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.
Attachment #8863310 - Flags: sec-approval?
Fairly certain we won't ever get there. Will provide analysis later today.
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.
Sec-approval+ and I'll approve for branches since we're early. We should make sure trunk is green and happy before checkin though.
Attachment #8863310 - Flags: sec-approval? → sec-approval+
Attachment #8863576 - Flags: approval-mozilla-esr52+
Attachment #8863577 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0614395829f No more planned releases coming for ESR45, so calling that wontfix.
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
Attachment #8863577 - Attachment is obsolete: true
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.
(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
Is there anything actionable here in terms of manual testing?
(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.
Flags: qe-verify? → qe-verify-
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
You need to log in before you can comment on or make changes to this bug.