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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Mats Palmgren (inactive)]

Are the values multiplied here controlled by content?
http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/gfx/layers/basic/BasicImages.cpp#114
If so, it might lead to integer overflow and potential security issues.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Milan, can you please help find someone who can investigate?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

We already check the size just before allocation, it should not cause integer overflow.
http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/gfx/layers/basic/BasicImages.cpp#106

Comment 4

[Sotaro Ikeda [:sotaro]]

But we could use CheckedInt to be more safer.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix BasicPlanarYCbCrImage::CopyData()

Comment 6

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc6e07db4570ea665c16d15e1f048470e7e2e98

Comment 7

[Sotaro Ikeda [:sotaro]]

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

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

Comment 8

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

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.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 9

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Given the severity of this bug, this doesn't seem worth a backport beyond maybe to Aurora54. Would you agree with that, Sotaro?

Comment 13

[Sotaro Ikeda [:sotaro]]

Yes, I agree.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Can you please add an Aurora nomination?

Comment 15

[Sotaro Ikeda [:sotaro]]

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.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8852748 [details] [diff] [review]
patch - Fix BasicPlanarYCbCrImage::CopyData()

Fix a security issue. Aurora54+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/011ddbeca97e