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

RESOLVED FIXED in Firefox 54



Audio/Video: GMP
8 months ago
26 days ago


(Reporter: mats, Assigned: gerald)


({csectype-intoverflow, regression, sec-high})

csectype-intoverflow, regression, sec-high
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)


(Whiteboard: [post-critsmash-triage][adv-main54+])


(2 attachments, 1 obsolete attachment)



8 months ago
Are the values multiplied here controlled by content?
If so, I think this can lead to integer overflow and might be exploitable.
Group: core-security → media-core-security
Flags: needinfo?(cpearce)
Maybe Gerald can take a look?
Flags: needinfo?(gsquelart)

Comment 2

8 months ago
Will do so now.
Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
Flags: needinfo?(cpearce)

Comment 3

8 months ago
Created attachment 8853167 [details] [diff] [review]

"Check CDM black-frame size computations."

CDMs will return a decode error is size computations overflow.
Attachment #8853167 - Flags: review?(cpearce)

Comment 4

8 months ago
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?
Ever confirmed: true
Flags: sec-review?(dveditz)
Comment on attachment 8853167 [details] [diff] [review]

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.
Attachment #8853167 - Flags: review?(cpearce) → review+

Comment 6

7 months ago
Created attachment 8857750 [details] [diff] [review]

Rebase, and using kDecodeError as per comment 5.

Carrying r+ from comment 5.
Attachment #8857750 - Flags: review+

Comment 7

7 months ago
Comment on attachment 8853167 [details] [diff] [review]

[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?

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.
Attachment #8853167 - Flags: sec-approval?
We need to figure out a security rating for this before sec-approval can be given.
Flags: needinfo?(dveditz)
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
(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:

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.
Blocks: 1343140
Flags: sec-review?(dveditz)
Flags: needinfo?(dveditz)
Keywords: csectype-intoverflow, regression, sec-high

Comment 10

7 months ago
Thank you Daniel. Scary stuff!
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.
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox55: --- → +
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.
Whiteboard: [checkin on 5/3]
Attachment #8853167 - Flags: sec-approval? → sec-approval+

Comment 13

7 months ago
Created attachment 8860240 [details] [diff] [review]

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
Attachment #8860240 - Flags: review+
Attachment #8860240 - Flags: approval-mozilla-beta?

Comment 14

7 months ago
Comment on attachment 8853167 [details] [diff] [review]

Obsoleted by attachment 8857750 [details] [diff] [review].
Attachment #8853167 - Attachment is obsolete: true

Comment 15

7 months ago
Comment on attachment 8857750 [details] [diff] [review]

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)
Attachment #8857750 - Flags: sec-approval?
Comment on attachment 8857750 [details] [diff] [review]

sec-approval+. Go and sin no more.

May 3 date still applies.
Attachment #8857750 - Flags: sec-approval? → sec-approval+
Attachment #8860240 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This missed 53. Let's aim the fix at 54.

Comment 18

7 months ago
Thank you Al & Liz.
status-firefox53: affected → wontfix
tracking-firefox53: + → ---
Whiteboard: [checkin on 5/3]
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → fixed
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.