1348894 - fix integer overflow in RecyclingPlanarYCbCrImage::CopyData

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In bug 1348168 we fixed a reported integer overflow exploit by disabling the feature that the exploit was in, since the feature contained other bugs.

However, the actual integer overflow that the exploit *used* is in code that I *believe* is not specific to that feature (RecyclingPlanarYCbCrImage::CopyData), and I believe it would be a good idea to land the integer overflow fix that I originally attached in bug 1348168 comment 17 (with a previous non-working version in bug 1348168 comment 14).

(I hope this isn't a duplicate of a bug that I can't see.)

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

This is the set of changes I have from a slightly broader audit of callers, but I still need to compile and test the new changes I added.

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Use CheckedInt more

(Note that I haven't attempted to compile the Android changes, but will try to remember to push to try before pushing to inbound.)

Comment 3

[Daniel Veditz [:dveditz]]

Should we try to ship this in a 52.0.x point release? Pros: have we leaked enough information in the other bug to lead people to this underlying bug and can it be triggered in other ways? Cons: might highlight this type of bug and cause people to search for this pattern of overflow (if they weren't already, as Chaitin Sec obviously was). Also con: requires an ESR45 update as well.

At the very least we should fix in Firefox 53/52.1/45.9

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8849345 [details] [diff] [review]
Use CheckedInt more

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

::: gfx/layers/ImageContainer.cpp
@@ +500,5 @@
>    mData = aData;
>  
>    // update buffer size
> +  // Use uint32_t throughout to match AllocateBuffer's param and mBufferSize
> +  const CheckedInt<uint32_t> checkedSize =

This can be const auto. CheckedInt<T> won't coerce between types.

@@ +507,5 @@
> +
> +  if (!checkedSize.isValid())
> +    return false;
> +
> +  const uint32_t size = checkedSize.value();

I would prefer `const auto&`, since we're just aliasing this with a new variable name.
`const auto` is fine otherwise. No need to write the obvious type again.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Use CheckedInt more. r=jgilbert

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I'm not entirely sure.  I think it's at least substantially harder now that we've disabled the overload of createImageBitmap in bug 1348168.

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

No, although it's relatively obvious from the code what sort of security bug it's trying to protect against.

Which older supported branches are affected by this flaw?

I think all.  The ImageContainer code seems to date from https://hg.mozilla.org/mozilla-central/rev/05e0e6423227 (at least the first half of it; didn't blame the second half).  The AndroidMediaReader code is newer and comes from bug 866080 in https://hg.mozilla.org/mozilla-central/rev/65adb5a76773 .

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?

I don't have the backports now, but I think they should be trivial to create.

How likely is this patch to cause regressions; how much testing does it need?

I think it's relatively unlikely to cause regressions.

Comment 6

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

sec-approval+ for trunk. We'll want Aurora, Beta, and ESR52 patches made and nominated as well.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d60ff982f8a69bd41a629345f6b0c43595bb9c1f

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b572053885859b78071bd6fd32e8e4dd2767dc70
Bug 1348894 - Use CheckedInt more.  r=jgilbert

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8851999 [details] [diff] [review]
Use CheckedInt more.  r=jgilbert

Patch applies cleanly to Aurora and Beta.  Still pulling an ESR52 tree to test there.

Approval Request Comment
[Feature/Bug causing the regression]: The ImageContainer code seems to date from https://hg.mozilla.org/mozilla-central/rev/05e0e6423227 (at least the first half of it; didn't blame the second half).  The AndroidMediaReader code is newer and comes from bug 866080 in https://hg.mozilla.org/mozilla-central/rev/65adb5a76773 .
[User impact if declined]:  Risk of integer overflow security vulnerabilities, although probably not as easily exploitable as bug 1348168.
[Is this code covered by automated tests?]: Probably to some degree, but not necessarily thoroughly.
[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?]: Low risk.
[Why is the change risky/not risky?]: It's a relatively straightforward change to fail (in a way that we would have failed before on allocation failure) in only the integer overflow cases that would have led to crashes.
[String changes made/needed]: No.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: see above
Fix Landed on Version: depends on approvals above
Risk to taking this patch (and alternatives if risky): see above
String or UUID changes made by this patch:  none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

It applies cleanly to ESR52 as well.

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8851999 [details] [diff] [review]
Use CheckedInt more.  r=jgilbert

sec-high fix for aurora54/beta53/esr52

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa249f1c67934d6d3bdb4048bf43d313412507bc

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/8c06f4260c2e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/00c051cd38c7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR45 as well still?

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

If it's still supported, probably.  Comment 6 seemed to imply that it wasn't...?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8851999 [details] [diff] [review]
Use CheckedInt more.  r=jgilbert

ESR45 doesn't go EOL until 52.2 ships.

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: ESR45 patch

I'm going to assume that we don't actually support the Gonk code that looks like it would also need patching.

I didn't see anything in the gstreamer code that looked like it needed to be patched.

Comment 20

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

Release Managers, can we get ESR45 approval here?

Comment 21

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8853827 [details] [diff] [review]
ESR45 patch

Makes sense to put this on esr45 as well.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/eed8086d0af7f2a76152af1b7893afc149d26e47

Comment 23

[Andrei Vaida [:avaida]]

Setting qe-verify- based on David's assessment on manual testing needs and the fact that this fix has automated coverage, at least in some measure (see Comment 9).