Closed Bug 1349595 Opened 3 years ago Closed 3 years ago

Possible integer overflow in allocation size in GMPVideoi420FrameImpl::CreateEmptyFrame?


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

Not set



Tracking Status
firefox-esr45 54+ wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed


(Reporter: mats, Assigned: gerald)


(Keywords: csectype-intoverflow, sec-high, Whiteboard: [post-critsmash-triage][adv-main54+][adv-esr52.2+])


(1 file)

Chris, do you know who might be able to look at this? Thanks.
Group: core-security → media-core-security
Flags: needinfo?(cpearce)
Will have a look soon.
Assignee: nobody → gsquelart
Flags: needinfo?(cpearce)
Attached patch 1349595.patchSplinter Review
"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?
Ever confirmed: true
Flags: sec-review?(dveditz)
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]

[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.
Whiteboard: [checkin on 5/3]
Attachment #8853195 - Flags: sec-approval? → sec-approval+
Thank you Al.
NI:myself to prepare uplifts.
Flags: needinfo?(gsquelart)
Comment on attachment 8853195 [details] [diff] [review]

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...
Flags: needinfo?(gsquelart)
Attachment #8853195 - Flags: approval-mozilla-beta?

There are no further planned ESR45 releases, so setting that branch to wontfix.
Whiteboard: [checkin on 5/3]
Comment on attachment 8853195 [details] [diff] [review]

See comment 9. Grafts cleanly to Beta/ESR52.
Attachment #8853195 - Flags: approval-mozilla-esr52?
Comment on attachment 8853195 [details] [diff] [review]

Sec-high, Beta54+, ESR52.2+
Attachment #8853195 - Flags: approval-mozilla-esr52?
Attachment #8853195 - Flags: approval-mozilla-esr52+
Attachment #8853195 - Flags: approval-mozilla-beta?
Attachment #8853195 - Flags: approval-mozilla-beta+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+][adv-esr52.2+]
Group: core-security-release
Flags: sec-review?(dveditz)
You need to log in before you can comment on or make changes to this bug.