Closed Bug 1349595 Opened 3 years ago Closed 3 years ago
Possible integer overflow in allocation size in GMPVideoi420Frame
Impl::Create Empty Frame?
Are the values multiplied here controlled by content? http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/dom/media/gmp/GMPVideoi420FrameImpl.cpp#142,147 If so, I think this can lead to integer overflow and might be exploitable. It calls GMPPlaneImpl::CreateEmptyPlane: http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/dom/media/gmp/GMPVideoPlaneImpl.cpp#114,120 which calls MaybeResize which does the allocation: http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/dom/media/gmp/GMPVideoPlaneImpl.cpp#77,87
Chris, do you know who might be able to look at this? Thanks.
Group: core-security → media-core-security
Will have a look soon.
Assignee: nobody → gsquelart
"Check GMP i420 frame size computations" Added check to CheckDimensions(), to ensure that 'h * s_y + (h+1)/2 * (s_u + s_v)' does not overflow.
Attachment #8853195 - Flags: review?(cpearce)
There is a potential for overflows, which I guess could capture some uninitialized memory past the too-small buffer. Dan, what do you think, please?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8853195 - Flags: review?(cpearce) → review+
The value passed into this function comes from OpenH264 and from the Widevine and ClearKey CDMs, so yes, we'd better sanitize it.
Comment on attachment 8853195 [details] [diff] [review] 1349595.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? If the GMP is compromised, it could send unsafe sizes that overflow, probably resulting in writes past the image buffer. Not sure how easy that is. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It points at the area (video output sizes), but would require more work beforehand to compromise the GMP. Which older supported branches are affected by this flaw? If not all supported branches, which bug introduced the flaw? All branches! Code landed in 32 from bug 957928. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch should just apply everywhere (or need a trivial rebase) as that portion of the file hasn't changed since 32. (I'll test my assumptions soon.) How likely is this patch to cause regressions; how much testing does it need? Unlikely, as it's only catching impossibly-big image sizes.
Attachment #8853195 - Flags: sec-approval?
This sounds like a sec-high. Sec-approval+ for checkin on May 3, which is two weeks into the next cycle. We're releasing 53 in the next few days. We'll want branch patches nominated as well so we can ship this on both ESR branches and 54 and 55.
Attachment #8853195 - Flags: sec-approval? → sec-approval+
Thank you Al. NI:myself to prepare uplifts.
Comment on attachment 8853195 [details] [diff] [review] 1349595.patch Approval Request Comment [Feature/Bug causing the regression]: bug 957928 [User impact if declined]: Crashes and potential exploits from compromised GMP. [Is this code covered by automated tests?]: No* [Has the fix been verified in Nightly?]: No* [Needs manual test from QE? If yes, steps to reproduce]: No?* [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Only added a check for size-calculation overflow [String changes made/needed]: None * I admit I'm not sure how to test this. I added a MOZ_CRASH above this test and launched mochitests, but nothing bad happened. I've looked for i420 test videos only but couldn't find any. We could wait for :cpearce to be back (in ~5 days), he would probably have suggestions...
Attachment #8853195 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac383ff77420336970a0b21ee46683b60fa1e08 There are no further planned ESR45 releases, so setting that branch to wontfix.
Comment on attachment 8853195 [details] [diff] [review] 1349595.patch See comment 9. Grafts cleanly to Beta/ESR52.
Comment on attachment 8853195 [details] [diff] [review] 1349595.patch Sec-high, Beta54+, ESR52.2+
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.