Closed Bug 1348936 Opened 3 years ago Closed 3 years ago

Possible integer overflow in allocation size in BasicPlanarYCbCrImage::CopyData?


(Core :: Graphics: Layers, defect)

Not set



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


(Reporter: mats, Assigned: sotaro)


(Keywords: csectype-intoverflow, sec-want, Whiteboard: [post-critsmash-triage][adv-main54-])


(1 file)

Are the values multiplied here controlled by content?
If so, it might lead to integer overflow and potential security issues.
Group: core-security → gfx-core-security
Milan, can you please help find someone who can investigate?
Flags: needinfo?(milan)
I think that these kinds of images only come from video which means that we would have run into trouble or handled bad size much earlier, but it may be easier to just use CheckedInt.  Either way, Sotaro should know best.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(milan)
We already check the size just before allocation, it should not cause integer overflow.
But we could use CheckedInt to be more safer.
Attachment #8852748 - Flags: review?(nical.bugzilla)
Attachment #8852748 - Flags: review?(nical.bugzilla) → review+
Keywords: sec-want
Comment on attachment 8852748 [details] [diff] [review]
patch - Fix BasicPlanarYCbCrImage::CopyData()

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

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

If not all supported branches, which bug introduced the flaw?
Exist on all supported branches. The problem is not serious since Bug 701259.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, but it is easier to create with CheckedInt32.

How likely is this patch to cause regressions; how much testing does it need?
It is not likely to cause the regression. Current tests seems enough.
Attachment #8852748 - Flags: sec-approval?
Comment on attachment 8852748 [details] [diff] [review]
patch - Fix BasicPlanarYCbCrImage::CopyData()

This is rated "sec-want" and doesn't need sec-approval+ to go into trunk. Only sec-high and sec-critical rated bugs (and all security bugs need a rating) require sec-approval.
Attachment #8852748 - Flags: sec-approval?
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Given the severity of this bug, this doesn't seem worth a backport beyond maybe to Aurora54. Would you agree with that, Sotaro?
Flags: needinfo?(sotaro.ikeda.g)
Yes, I agree.
Flags: needinfo?(sotaro.ikeda.g)
Group: gfx-core-security → core-security-release
Can you please add an Aurora nomination?
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8852748 [details] [diff] [review]
patch - Fix BasicPlanarYCbCrImage::CopyData()

Approval Request Comment
[Feature/Bug causing the regression]:None. Exist on all supported branches. The problem is not serious since Bug 701259.
[User impact if declined]: Impact is very low, since the bug actually might not hit the problem. If it hit, content process could crash.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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 riak.
[Why is the change risky/not risky?]: It just adds CheckedInt32.
[String changes made/needed]: None.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8852748 - Flags: approval-mozilla-aurora?
Comment on attachment 8852748 [details] [diff] [review]
patch - Fix BasicPlanarYCbCrImage::CopyData()

Fix a security issue. Aurora54+.
Attachment #8852748 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.