Closed Bug 1367058 Opened 3 years ago Closed 3 years ago
Integer overflow in dom/canvas/Canvas
Rendering Context2D .cpp with get Image Data
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
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)
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+
we're late in beta54, and from comment 1 it sounds like this is actually safe, so marking wontfix for 54.
Whiteboard: gfx-noted → [adv-main55-] gfx-noted
Whiteboard: [adv-main55-] gfx-noted → [post-critsmash-triage][adv-main55-] gfx-noted
You need to log in before you can comment on or make changes to this bug.