Closed Bug 1348894 Opened 3 years ago Closed 3 years ago

fix integer overflow in RecyclingPlanarYCbCrImage::CopyData


(Core :: Graphics, defect)

Not set



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


(Reporter: dbaron, Assigned: dbaron)



(Keywords: csectype-intoverflow, sec-high, Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+])


(2 files, 2 obsolete files)

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.)
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch Use CheckedInt more (obsolete) — Splinter Review
(Note that I haven't attempted to compile the Android changes, but will try to remember to push to try before pushing to inbound.)
Attachment #8849345 - Flags: review?(jgilbert)
Attachment #8849163 - Attachment is obsolete: true
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
See Also: → CVE-2017-5428
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.
Attachment #8849345 - Flags: review?(jgilbert) → review+
[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 (at least the first half of it; didn't blame the second half).  The AndroidMediaReader code is newer and comes from bug 866080 in .

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.
Attachment #8849345 - Attachment is obsolete: true
Attachment #8851999 - Flags: sec-approval?
Attachment #8851999 - Flags: review+
sec-approval+ for trunk. We'll want Aurora, Beta, and ESR52 patches made and nominated as well.
Attachment #8851999 - Flags: sec-approval? → sec-approval+
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 (at least the first half of it; didn't blame the second half).  The AndroidMediaReader code is newer and comes from bug 866080 in .
[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 for more info.
Attachment #8851999 - Flags: approval-mozilla-esr52?
Attachment #8851999 - Flags: approval-mozilla-beta?
Attachment #8851999 - Flags: approval-mozilla-aurora?
It applies cleanly to ESR52 as well.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8851999 [details] [diff] [review]
Use CheckedInt more.  r=jgilbert

sec-high fix for aurora54/beta53/esr52
Attachment #8851999 - Flags: approval-mozilla-esr52?
Attachment #8851999 - Flags: approval-mozilla-esr52+
Attachment #8851999 - Flags: approval-mozilla-beta?
Attachment #8851999 - Flags: approval-mozilla-beta+
Attachment #8851999 - Flags: approval-mozilla-aurora?
Attachment #8851999 - Flags: approval-mozilla-aurora+
Do we need this on ESR45 as well still?
Flags: needinfo?(dbaron)
If it's still supported, probably.  Comment 6 seemed to imply that it wasn't...?
Flags: needinfo?(dbaron)
Comment on attachment 8851999 [details] [diff] [review]
Use CheckedInt more.  r=jgilbert

ESR45 doesn't go EOL until 52.2 ships.
Attachment #8851999 - Flags: approval-mozilla-esr45?
Attached patch ESR45 patchSplinter Review
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.
Attachment #8851999 - Flags: approval-mozilla-esr45?
Attachment #8853827 - Flags: approval-mozilla-esr45?
Release Managers, can we get ESR45 approval here?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Comment on attachment 8853827 [details] [diff] [review]
ESR45 patch

Makes sense to put this on esr45 as well.
Flags: needinfo?(lhenry)
Attachment #8853827 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr48.9+]
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr48.9+] → [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Group: gfx-core-security → core-security-release
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).
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.