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

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Andrew McCreight [:mccr8]]

Chris, do you know who might be able to look at this? Thanks.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Will have a look soon.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1349595.patch

"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.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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?

Comment 5

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

The value passed into this function comes from OpenH264 and from the Widevine and ClearKey CDMs, so yes, we'd better sanitize it.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 7

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

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.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Al.
NI:myself to prepare uplifts.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ac383ff77420336970a0b21ee46683b60fa1e08

There are no further planned ESR45 releases, so setting that branch to wontfix.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8853195 [details] [diff] [review]
1349595.patch

See comment 9. Grafts cleanly to Beta/ESR52.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8853195 [details] [diff] [review]
1349595.patch

Sec-high, Beta54+, ESR52.2+

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/9ac383ff7742

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e664fb8bf2fd
https://hg.mozilla.org/releases/mozilla-esr52/rev/9071c7d4cc9c