1818674 - fix a multiply in gfx/2d/DataSurfaceHelpers.cpp that can overflow signed int32

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Timothy Nikkel (:tnikkel)]

I did a quick audit of all multiplies in gfx/2d/DataSurfaceHelpers.cpp after reviewing bug 1817442. Only one seemed to be a possible problem. This one

https://searchfox.org/mozilla-central/rev/edccfac5746704da49c1551aed8b79f57003bd68/gfx/2d/DataSurfaceHelpers.cpp#91

happens as signed int32 but the result could exceed the max for that size.

Assuming that the passed in surface has been validly allocated then this calculation, if performed correctly, should point to some memory within the surface: the SurfaceContainsPoint check ensures that. So we just need to make sure to use the correct type for this calculation, and we don't need to check that the calculation is valid and doesn't overflow etc.

https://hg.mozilla.org/mozilla-central/rev/8d737d52c4cbe38b7e577b8050f1f0caffcd8214 (bug 924102) introduced this code but it's been moved around and used for different things over the years.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1818674. r?lsalzman

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 924102

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9319609 [details]
Bug 1818674. r?lsalzman

Security Approval Request

• How easily could an exploit be constructed based on the patch?: the patch can't hide where/what the problem is, attacker could use that to read memory anywhere in the process I guess
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 924102
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: trivial
• How likely is this patch to cause regressions; how much testing does it need?: unlikely
• Is Android affected?: Yes

Comment 4

[Andrew McCreight [:mccr8]]

From the description, it sounds like a sec-high issue.

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9319609 [details]
Bug 1818674. r?lsalzman

Approved to request uplift and land

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9319609 [details]
Bug 1818674. r?lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: sec issue
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): fixes potential overflow
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sechigh
• User impact if declined: sec issue
• Fix Landed on Version: 112
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): fixes potential overflow

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/920b34ac292b

Comment 8

[Donal Meehan [:dmeehan]]

Comment on attachment 9319609 [details]
Bug 1818674. r?lsalzman

Approved for 111.0b7

Comment 9

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/853af2a66f07

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9319609 [details]
Bug 1818674. r?lsalzman

Approved for 102.9esr.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/afef5fc4c0e1