Closed Bug 1483120 Opened 3 years ago Closed 3 years ago

heap-buffer-overflow in [@ SkDashPath::InternalFilter]

Categories

(Core :: Canvas: 2D, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 62+ verified
firefox61 --- wontfix
firefox62 + verified
firefox63 + verified

People

(Reporter: tsmith, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main62+][adv-esr60.2+][post-cristsmash-triage])

Attachments

(2 files, 1 obsolete file)

Attached file testcase.html
==81806==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000408388 at pc 0x7f0f9a7ef6c9 bp 0x7ffe5a065370 sp 0x7ffe5a065368
READ of size 4 at 0x603000408388 thread T0 (file:// Content)
    #0 0x7f0f9a7ef6c8 in SkDashPath::InternalFilter(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*, float const*, int, float, int, float, SkDashPath::StrokeRecApplication) src/gfx/skia/skia/src/utils/SkDashPath.cpp:378:31
    #1 0x7f0f9a1fee50 in SkDashImpl::filterPath(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*) const src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:40:12
    #2 0x7f0f9a8b90a6 in SkPaint::getFillPath(SkPath const&, SkPath*, SkRect const*, float) const src/gfx/skia/skia/src/core/SkPaint.cpp:1498:37
    #3 0x7f0f9a4ceab3 in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*, SkInitOnceData*) const src/gfx/skia/skia/src/core/SkDraw.cpp:1120:25
    #4 0x7f0f9a4cc1dd in drawPath src/gfx/skia/skia/src/core/SkDraw.h:58:15
    #5 0x7f0f9a4cc1dd in draw_rect_as_path(SkDraw const&, SkRect const&, SkPaint const&, SkMatrix const*) src/gfx/skia/skia/src/core/SkDraw.cpp:735
    #6 0x7f0f9a4cb525 in SkDraw::drawRect(SkRect const&, SkPaint const&, SkMatrix const*, SkRect const*) const src/gfx/skia/skia/src/core/SkDraw.cpp:762:9
    #7 0x7f0f9a18bc4d in drawRect src/gfx/skia/skia/src/core/SkDraw.h:44:15
    #8 0x7f0f9a18bc4d in SkBitmapDevice::drawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkBitmapDevice.cpp:296
    #9 0x7f0f9a1bd641 in SkCanvas::onDrawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkCanvas.cpp:2041:27
    #10 0x7f0f9a1b4c2c in SkCanvas::drawRect(SkRect const&, SkPaint const&) src/gfx/skia/skia/src/core/SkCanvas.cpp:1712:11
    #11 0x7f0f907ff4f2 in mozilla::gfx::DrawTargetSkia::StrokeRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) src/gfx/2d/DrawTargetSkia.cpp:936:12
    #12 0x7f0f94f666ce in mozilla::dom::CanvasRenderingContext2D::StrokeRect(double, double, double, double) src/dom/canvas/CanvasRenderingContext2D.cpp:3151:5
    #13 0x7f0f9367320d in mozilla::dom::CanvasRenderingContext2D_Binding::strokeRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:5476:9
    #14 0x7f0f94e24759 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3311:13
    #15 0x7f0f9c0a1b0e in CallJSNative src/js/src/vm/Interpreter.cpp:445:15
    #16 0x7f0f9c0a1b0e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:533
    #17 0x7f0f9c08c3f7 in CallFromStack src/js/src/vm/Interpreter.cpp:590:12
    #18 0x7f0f9c08c3f7 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3239
    #19 0x7f0f9c07280e in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:425:12
    #20 0x7f0f9c0a23e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:557:15
    #21 0x7f0f9c0a3972 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:603:10
    #22 0x7f0f9cb26b4a in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2915:12
    #23 0x7f0f944297fe in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:51:8
    #24 0x7f0f9569983e in HandleEvent<mozilla::dom::EventTarget *> src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #25 0x7f0f9569983e in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1108
    #26 0x7f0f9569b9c6 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1342:20
    #27 0x7f0f9567f5c9 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #28 0x7f0f9567f5c9 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:418
    #29 0x7f0f9567d883 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:635:16
    #30 0x7f0f956840de in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1110:9
    #31 0x7f0f98284f7f in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1169:7
    #32 0x7f0f9aff7a4c in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7054:21
    #33 0x7f0f9aff262a in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6847:7
    #34 0x7f0f9affc207 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #35 0x7f0f905ca365 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1309:3
    #36 0x7f0f905c8f8c in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
    #37 0x7f0f905c4a91 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
    #38 0x7f0f905c7578 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:627:5
    #39 0x7f0f905c8ab4 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #40 0x7f0f8e0db467 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
    #41 0x7f0f91e84c27 in DoUnblockOnload src/dom/base/nsDocument.cpp:8301:18
    #42 0x7f0f91e84c27 in nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8223
    #43 0x7f0f91e6168d in nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5095:3
    #44 0x7f0f91f232cb in applyImpl<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1168:12
    #45 0x7f0f91f232cb in apply<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1174
    #46 0x7f0f91f232cb in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1219
    #47 0x7f0f8de37062 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #48 0x7f0f8de74420 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1235:14
    #49 0x7f0f8de7d185 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #50 0x7f0f8f052f1e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #51 0x7f0f8ef5520c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #52 0x7f0f8ef5520c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #53 0x7f0f8ef5520c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #54 0x7f0f97a191a6 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #55 0x7f0f9bd79e8e in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:940:22
    #56 0x7f0f8ef5520c in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #57 0x7f0f8ef5520c in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #58 0x7f0f8ef5520c in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #59 0x7f0f9bd78f42 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:766:34
    #60 0x4f5b11 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #61 0x4f5b11 in main src/browser/app/nsBrowserApp.cpp:287
    #62 0x7f0fb38a282f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #63 0x424ee8 in _start (firefox+0x424ee8)

0x603000408388 is located 0 bytes to the right of 24-byte region [0x603000408370,0x603000408388)
allocated by thread T0 (file:// Content) here:
    #0 0x4c5633 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x51a34d in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f0f9a201473 in sk_malloc_throw src/gfx/skia/skia/include/private/SkMalloc.h:59:12
    #3 0x7f0f9a201473 in SkDashImpl src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:23
    #4 0x7f0f9a201473 in SkDashPathEffect::Make(float const*, int, float) src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:397
    #5 0x7f0f907fe3e5 in mozilla::gfx::StrokeOptionsToPaint(SkPaint&, mozilla::gfx::StrokeOptions const&) src/gfx/2d/HelpersSkia.h:156:32
    #6 0x7f0f907ff470 in mozilla::gfx::DrawTargetSkia::StrokeRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) src/gfx/2d/DrawTargetSkia.cpp:932:8
    #7 0x7f0f94f666ce in mozilla::dom::CanvasRenderingContext2D::StrokeRect(double, double, double, double) src/dom/canvas/CanvasRenderingContext2D.cpp:3151:5
    #8 0x7f0f9367320d in mozilla::dom::CanvasRenderingContext2D_Binding::strokeRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:5476:9
    #9 0x7f0f94e24759 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3311:13
    #10 0x7f0f9c0a1b0e in CallJSNative src/js/src/vm/Interpreter.cpp:445:15
    #11 0x7f0f9c0a1b0e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:533
    #12 0x7f0f9c08c3f7 in CallFromStack src/js/src/vm/Interpreter.cpp:590:12
    #13 0x7f0f9c08c3f7 in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3239
    #14 0x7f0f9c07280e in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:425:12
    #15 0x7f0f9c0a23e4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:557:15
    #16 0x7f0f9c0a3972 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:603:10
    #17 0x7f0f9cb26b4a in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2915:12
    #18 0x7f0f944297fe in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:51:8
    #19 0x7f0f9569983e in HandleEvent<mozilla::dom::EventTarget *> src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
    #20 0x7f0f9569983e in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1108
    #21 0x7f0f9569b9c6 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1342:20
    #22 0x7f0f9567f5c9 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #23 0x7f0f9567f5c9 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:418
    #24 0x7f0f9567d883 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:635:16
    #25 0x7f0f956840de in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1110:9
Flags: in-testsuite?
Attached file prefs-default.js (obsolete) —
Attached prefs are required to trigger the testcase (e10s disabled).
Comment on attachment 9001365 [details]
prefs-default.js

This works with and without e10s.
Attachment #9001365 - Attachment is obsolete: true
Flags: needinfo?(lsalzman)
Keywords: sec-high
Is there a Skia-specific testcase that you could report upstream? This looks like a Skia internal bug.
Flags: needinfo?(lsalzman)
This was found with a canvas fuzzer targeting the browser. So no skia specific test is available.
The problem here is that endPhase is a really big number, such that due to floating point imprecision subtracting a small non-zero number from it can still yield the exact same number, i.e. ReallyBigNumber - Epsilon == ReallyBigNumber.

We know that endPhase will start as a value less than intervalLength, but due to above-mentioned problem, it may never hit 0 via subtraction.

intervalLength is calculated as a sum of all the intervals here: https://dxr.mozilla.org/mozilla-central/source/gfx/skia/skia/src/utils/SkDashPath.cpp?q=SkDashPath%3A%3ACalcDashParameters&redirect_type=direct#39

So to fix this, we need to sum the intervals until they exceed endPhase, which ensures that they are summed in the same order as intervalLength and will eventually get there given that endPhase is within bounds.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #9001742 - Flags: review?(rhunt)
Has this been fixed in upstream Skia, or do we need to notify them of this bug?
It's still broken upstream.
Added awhalley from Chromium; not if there's anything else involved in notifying them?
Attachment #9001742 - Flags: review?(rhunt) → review+
Thanks - I've filed crbug.com/875494 to track this upstream
ni? test for abillings (please ignore)
Flags: needinfo?(abillings)
Flags: needinfo?(abillings)
Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

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

> 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?
60+

> If not all supported branches, which bug introduced the flaw?
Bug 1444506

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should already apply.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #9001742 - Flags: sec-approval?
I'd love to take this but we only have a single beta left, building tomorrow. I need to ni? release management and get their approval.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Let's take this. If it lands on inbound today, we'll have time to uplift in time for tomorrow's build.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

Sec-approval+ for trunk. We'll need a beta patch and an ESR60 patch nominated ASAP to try to get tomorrow's final beta for 62.
Flags: needinfo?(lsalzman)
Attachment #9001742 - Flags: sec-approval? → sec-approval+
Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1444506
[User impact if declined]: Heap overread
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: Just a small tweak to some Skia interval math to avoid floating-point precision issues.
[String changes made/needed]: None
Flags: needinfo?(lsalzman)
Attachment #9001742 - Flags: approval-mozilla-esr60?
Attachment #9001742 - Flags: approval-mozilla-beta?
Blocks: 1444506
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/54934de382c5
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 9001742 [details] [diff] [review]
sum SkDashPath intervals instead of subtracting

Fixes a Skia sec issue, approved for 62.0b20 and ESR 60.2.
Attachment #9001742 - Flags: approval-mozilla-esr60?
Attachment #9001742 - Flags: approval-mozilla-esr60+
Attachment #9001742 - Flags: approval-mozilla-beta?
Attachment #9001742 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main62+][adv-esr60.2+]
Chromium/Skia bug fixed upstream
For reference, which commit?
Flags: qe-verify+
Whiteboard: [adv-main62+][adv-esr60.2+] → [adv-main62+][adv-esr60.2+][post-cristsmash-triage]
Thanks!
Hi,
I have verified the fix for this bug using the latest ASAN builds for Fx 62 and Fx 63 and Fx esr60 on Windows 10 x64, Ubuntu 16.04 LTS and Mac OS 10.13. The bug is Verified Fixed. Marking it accordingly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.