1349604 - Possible integer overflow in allocation size in WidevineVideoFrame::InitToBlack?

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/widevine-adapter/WidevineVideoFrame.cpp#132-134
If so, I think this can lead to integer overflow and might be exploitable.

Comment 1

[Andrew McCreight [:mccr8]]

Maybe Gerald can take a look?

Comment 2

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

Will do so now.

Comment 3

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

Attachment: 1349604.patch

"Check CDM black-frame size computations."

CDMs will return a decode error is size computations overflow.

Comment 4

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

There is a potential for overflows on 32-bit builds, but then the memory is memset() with 0s or 128s only, so I doubt it's exploitable -- but I'm not an expert!
Dan, what do you think, please?

Comment 5

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

Comment on attachment 8853167 [details] [diff] [review]
1349604.patch

Review of attachment 8853167 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/gmp/ChromiumCDMChild.cpp
@@ +539,5 @@
>        // to a stream that doesn't require OP. In order to keep the playback
>        // pipeline rolling, just output a black frame. See bug 1343140.
> +      if (!frame.InitToBlack(mCodedSize.width, mCodedSize.height,
> +                             input.timestamp)) {
> +        Unused << SendDecodeFailed(rv);

I think it makes more sense to report kDecodeError here rather than kNoKey, as semantically kNoKey means something other than complete failure (which is what we've hit here), and we may very well end up retrying the decode with rv=kNoKey, and then failing again.

Comment 6

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

Attachment: 1349604.patch

Rebase, and using kDecodeError as per comment 5.

Carrying r+ from comment 5.

Comment 7

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

Comment on attachment 8853167 [details] [diff] [review]
1349604.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I doubt it's possible for this one, as (I think) in the worst case we could write zeroes and 128s outside of the under-allocated buffer.

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

Which older supported branches are affected by this flaw?
53 (end of beta -- too late?) and 54.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports ready yet, but I think it should be easy to produce, as that same new code was created in 55 and backported to 54 and 53 (see bug 1343140).

How likely is this patch to cause regressions; how much testing does it need?
Unlikely I think, as it only adds overflow checks for impossibly-big video sizes.

Comment 8

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

We need to figure out a security rating for this before sec-approval can be given.

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Gerald Squelart [:gerald] from comment #7)
> How easily could an exploit be constructed based on the patch?
> I doubt it's possible for this one, as (I think) in the worst case we could
> write zeroes and 128s outside of the under-allocated buffer.

You're probably correct that it's not easy, but don't underestimate a determined hacker. Here's a write up of how a one-byte overflow was turned into a working exploit:
https://googleprojectzero.blogspot.com/2016/12/chrome-os-exploit-one-byte-overflow-and.html

We have to assume that the allocation patterns for a given malicious content would be known and could be shaped to drop into a cleared memory hole, though it could be so much is going on it would be chaotic and unpredictable.

Comment 10

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

Thank you Daniel. Scary stuff!

Comment 11

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

Tracking for 53. I'm not sure if this is something we'd want for a dot release or whether it can wait for 54.

Comment 12

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

Sec-approval+ for checkin on *May 3* or later, two weeks into the new cycle. We'll want this back ported so please nominate for 54 as well.

Comment 13

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

Attachment: 1349604-beta.patch

Rebase to beta-54, no code change, just one fewer file that didn't exist then.
Carrying r+ from comment 5.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1343140, which itself was backported to 54, see bug 1343140 comment 13
[User impact if declined]: Crashes and potential exploit when playing ill-formed encrypted videos
[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 way to test bad videos; but if we wanted to test that good videos still behave correctly, see bug 1343140 comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's only converting unchecked arithmetic operations to checked ones
[String changes made/needed]: None

Comment 14

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

Comment on attachment 8853167 [details] [diff] [review]
1349604.patch

Obsoleted by attachment 8857750 [details] [diff] [review].

Comment 15

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

Comment on attachment 8857750 [details] [diff] [review]
1349604.patch

Sorry Al, the sec-approval was requested&granted on an older patch.
Could you please rubber stamp this one? (Only difference is the SendDecodeFailed argument, as per comment 5)

Comment 16

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

Comment on attachment 8857750 [details] [diff] [review]
1349604.patch

sec-approval+. Go and sin no more.

May 3 date still applies.

Comment 17

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

This missed 53. Let's aim the fix at 54.

Comment 18

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

Thank you Al & Liz.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

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

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b7a8c7f7e088