Closed Bug 1367058 Opened 3 years ago Closed 3 years ago

Integer overflow in dom/canvas/CanvasRenderingContext2D.cpp with getImageData

Categories

(Core :: Canvas: 2D, defect, P3, critical)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: decoder, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-intoverflow, sec-moderate, testcase, Whiteboard: [post-critsmash-triage][adv-main55-] gfx-noted)

Attachments

(2 files)

The attached testcase triggers an integer overflow on 32-bit platforms here:

http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/dom/canvas/CanvasRenderingContext2D.cpp#5921

The message I get with UBSan's -fsanitize=integer is:

dom/canvas/CanvasRenderingContext2D.cpp:5916:42: runtime error: unsigned integer overflow: 1351508277 * 262196 cannot be represented in type 'unsigned int'

The revision I was fuzzing was a little older, hence the difference in line numbers. The location in the first URL is the one overflowing.

This overflow caught my attention because it is flowing into pointer arithmetics. At first glance, it looked to me like we might potentially leak uninitialized memory here (which would make this sec-high). I briefly showed this to :aosmond and he mentioned that indeed this looks sketchy and should be filed.

Marking s-s until we have a security analysis.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: gfx-noted
Now that I have spent more time looking at this, it is less concerning than I originally thought. The math actually checks out -- unless there is no overlap between the requested coordinate, and the canvas. Complicating matters was that the y parameter overflows an int32 and due to the modulo logic for JS double -> int32 conversion, it turns negative as well.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa74d31cb6c76304b506436f59ef388c5d24564

In the end, it results in a noop with that pointer, so it is safe, but we can restructure the code to be more intuitive and avoid tripping the hazard. Should not have any real functional change.
Attachment #8871277 - Flags: review?(mchang)
Group: core-security → gfx-core-security
Blocks: grizzly
Comment on attachment 8871277 [details] [diff] [review]
Clarify no overlap path in CanvasRenderingContext2D::GetImageDataArray, v1

Confirmed that with the patch, the overflow is indeed gone. So this should not trip any further alerts. Thanks!
Attachment #8871277 - Flags: feedback+
Attachment #8871277 - Flags: review?(mchang) → review+
https://hg.mozilla.org/mozilla-central/rev/6b6a15d2bf8a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
we're late in beta54, and from comment 1 it sounds like this is actually safe, so marking wontfix for 54.
Sounds like a wontfix for ESR52 too.
Group: gfx-core-security → core-security-release
Whiteboard: gfx-noted → [adv-main55-] gfx-noted
Flags: qe-verify-
Whiteboard: [adv-main55-] gfx-noted → [post-critsmash-triage][adv-main55-] gfx-noted
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.