Closed Bug 1287316 (CVE-2016-5275) Opened 9 years ago Closed 8 years ago

global-buffer-overflow in mozilla::gfx::FilterSupport::ComputeSourceNeededRegions

Categories

(Core :: Graphics: Canvas2D, defect)

Product:
Core
Component:
Graphics: Canvas2D
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 --- unaffected
firefox50 + fixed
firefox51 --- fixed

People

(Reporter: nils, Assigned: mstange)

References

Details

(4 keywords, Whiteboard: [adv-main49+])

Crash Data

Attachments

(2 files, 2 obsolete files)

testcase from comment 0
827 bytes, text/html
Details
Release assert or deal with empty arrays. Carry r=mstange
9.38 KB, patch
milan
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Diff | Splinter Review
The following testcase crashes the latest ASAN build of Firefox (BuildID=20160711143746) as follows: <script> function start() { o0=document; o13=(new DOMParser()).parseFromString('','text/html'); o14=o13.all[0]; o80=document.createElement('canvas'); o14.innerHTML='<svg style><filter><feConvolveMatrix>'; o133=o14.querySelectorAll('*')[2]; o134=o14.querySelectorAll('*')[3]; o254=o80.getContext('2d',{willReadFrequently: true,}); o134.id='id14'; try{while(document.removeChild(document.firstChild));}catch(e){} o298=document.implementation.createHTMLDocument(); o298.body.appendChild(o133); document.appendChild(o298.documentElement); o254.filter=' url(#id14) url(#id14) '; o0.write('<html><body></body></html>'); o254.fillRect(545259505,-1,8200,14942208); } </script> <body onload="start()"></body> ASAN output: ================================================================= ==18075==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f229cbe8f90 at pc 0x00000049df78 bp 0x7ffdcea2c650 sp 0x7ffdcea2be10 WRITE of size 16 at 0x7f229cbe8f90 thread T0 (Web Content) #0 0x49df77 in __asan_memcpy /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:393:3 #1 0x7f2297a0f642 in _moz_pixman_region32_copy /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/cairo/libpixman/src/pixman-region.c:523:25 #2 0x7f22909da2b4 in Copy /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/nsRegion.h:414:5 #3 0x7f22909da2b4 in operator= /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/nsRegion.h:77 #4 0x7f22909da2b4 in operator= /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/nsRegion.h:487 #5 0x7f22909da2b4 in operator= /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/nsRegion.h:842 #6 0x7f22909da2b4 in mozilla::gfx::FilterSupport::ComputeSourceNeededRegions(mozilla::gfx::FilterDescription const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/src/FilterSupport.cpp:1774 #7 0x7f22935402f5 in mozilla::dom::AdjustedTargetForFilter::AdjustedTargetForFilter(mozilla::dom::CanvasRenderingContext2D*, mozilla::gfx::DrawTarget*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::CompositionOp) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:353:5 #8 0x7f22934a3c76 in MakeUnique<mozilla::dom::AdjustedTargetForFilter, mozilla::dom::CanvasRenderingContext2D *&, RefPtr<mozilla::gfx::DrawTarget> &, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> &, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> &, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::CompositionOp &> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/gfx/gl/../../mfbt/UniquePtr.h:680:27 #9 0x7f22934a3c76 in mozilla::dom::AdjustedTarget::AdjustedTarget(mozilla::dom::CanvasRenderingContext2D*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:613 #10 0x7f22934a266a in mozilla::dom::CanvasRenderingContext2D::FillRect(double, double, double, double) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:2814:3 #11 0x7f22925ec2fd in mozilla::dom::CanvasRenderingContext2DBinding::fillRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:3185:3 #12 0x7f22933d3c07 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/bindings/BindingUtils.cpp:2784:13 #13 0x7f22992b36db in CallJSNative /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jscntxtinlines.h:232:15 #14 0x7f22992b36db in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:441 #15 0x7f229929a662 in CallFromStack /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:504:12 #16 0x7f229929a662 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:2873 #17 0x7f229928103b in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:399:12 #18 0x7f22992b3d80 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:471:15 #19 0x7f22992b43f1 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:517:10 #20 0x7f2298df8898 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:2839:12 #21 0x7f2292ee8540 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:259:37 #22 0x7f229381e97b in Call<nsISupports *> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:361:12 #23 0x7f229381e97b in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/events/JSEventHandler.cpp:214 #24 0x7f22937ebb6f in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/events/EventListenerManager.cpp:1116:16 #25 0x7f22937ed6ed in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/events/EventListenerManager.cpp:1288:17 #26 0x7f22937c910c in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/events/EventDispatcher.cpp:379:5 #27 0x7f22937cd470 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/events/EventDispatcher.cpp:710:9 #28 0x7f22958ebd65 in nsDocumentViewer::LoadComplete(nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsDocumentViewer.cpp:996:7 #29 0x7f229664ccd1 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:7577:5 #30 0x7f2296648d01 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:7378:7 #31 0x7f229664ff7f in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/docshell/base/nsDocShell.cpp:7275:13 #32 0x7f22906a3ba1 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsDocLoader.cpp:1252:3 #33 0x7f22906a2c54 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsDocLoader.cpp:836:5 #34 0x7f229069fadc in nsDocLoader::DocLoaderIsEmpty(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsDocLoader.cpp:726:9 #35 0x7f22906a1bb4 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsDocLoader.cpp:608:5 #36 0x7f22906a26ac in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/uriloader/base/nsDocLoader.cpp:464:14 #37 0x7f228ea7f79b in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsLoadGroup.cpp:633:18 #38 0x7f229159136f in nsDocument::DoUnblockOnload() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsDocument.cpp:9256:7 #39 0x7f22915d5e8f in nsUnblockOnloadEvent::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsDocument.cpp:9209:5 #40 0x7f228e8ac996 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1073:7 #41 0x7f228e92ad3c in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10 #42 0x7f228f671a1f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21 #43 0x7f228f5e62e8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235:3 #44 0x7f228f5e62e8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228 #45 0x7f228f5e62e8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208 #46 0x7f229503987f in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156:3 #47 0x7f22970bf8a7 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:837:12 #48 0x7f228f5e62e8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235:3 #49 0x7f228f5e62e8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228 #50 0x7f228f5e62e8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208 #51 0x7f22970bef43 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:667:7 #52 0x4e2c05 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:224:19 #53 0x7f228b95782f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291 #54 0x41e7b8 in _start (/home/nils/fuzzer3/firefox/plugin-container+0x41e7b8) 0x7f229cbe8f90 is located 16 bytes to the left of global variable 'nsTArrayHeader::sEmptyHdr' defined in '/builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsTArray.cpp:13:32' (0x7f229cbe8fa0) of size 8 0x7f229cbe8f90 is located 40 bytes to the right of global variable 'sStderrCallback' defined in '/builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsCRTGlue.cpp:316:23' (0x7f229cbe8f60) of size 8 SUMMARY: AddressSanitizer: global-buffer-overflow /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:393:3 in __asan_memcpy Shadow bytes around the buggy address: 0x0fe4d39751a0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 0x0fe4d39751b0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe4d39751c0: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe4d39751d0: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe4d39751e0: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 =>0x0fe4d39751f0: f9 f9[f9]f9 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 0x0fe4d3975200: 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 0x0fe4d3975210: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe4d3975220: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 0x0fe4d3975230: f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 0x0fe4d3975240: f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==18075==ABORTING
Group: core-security → gfx-core-security
Keywords: csectype-bounds, regressionwindow-wanted, sec-critical
This crashes a recent release copy of Dev Edition (Firefox 49) on Mac so hunting this down shouldn't be restricted to folks who have a linux ASAN build: bp-c826173c-2f50-41bc-b80e-c8faa2160719 Nils: if you have these testcases as files already it would be more convenient for us if you added them as attachments (which can be done as part of the initial filing; doesn't need to be a second step) rather than in comment text.
Beta (Firefox 48) and the latest ESR-45 don't crash with the attached testcase. Provisionally looks like this regressed during Firefox 49 development. Possibly the underlying problem found by ASAN goes back further and a second change turns it into an active crash so we'll need to double-check the affected versions once the bug has been identified.
status-firefox49: --- → affected
Flags: needinfo?(milan)
Keywords: regression
Right - out of bounds index: Assertion failure: aIndex < Length() (invalid array index), at /Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin15.5.0/dist/include/nsTArray.h:1003
Flags: needinfo?(milan)
The obvious regression range isn't that useful - it just points to us enabling canvas filters (bug 1173545.)
We should make sure this gets a crash test once the bug is public.
This is an overkill - we are checking in a few places. Canvas... change on its own will fix the crash, as would the change in Filter... on its own (though we have to remove an assert MOZ_ASSERT(!filter.mPrimitives.IsEmpty()); and replace it with an actual check) Don't know which one makes more sense, if either.
Version: 50 Branch → 49 Branch
Let's figure out a solution for this while 49 is aurora, so that we don't have to uplift a sec-critical to beta.
Flags: needinfo?(mstange)
So callers of nsFilterInstance::GetFilterDescription are supposed to handle empty filters, because nsFilterInstance::GetFilterDescription explicitly returns an empty description if the instance is in an error state (!IsInitialized()). Other parts of nsFilterInstance to use IsInitialized() to bail out before calling a FilterSupport function. If mPrimitiveDescriptions.IsEmpty(), mInitialized / IsInitialized() will always be false. So nsFilterInstance itself is fine. All the static FilterSupport methods assume that the description has at least one primitive. So our checks need to be in CanvasRenderingContext2D before we call any FilterSupport method, and ideally before we do other work that is unnecessary if we bail out. And we need to document in FilterSupport.h that you're not allowed to call the static methods with empty filter descriptions. In CanvasRenderingContext2D, we call NeedToApplyFilter() before we construct the AdjustedDrawTargetForFilter. NeedToApplyFilter() checks state.filter.mPrimitives.Length(). Why is that check not saving us here?
Flags: needinfo?(mstange)
Attachment #8772551 - Flags: feedback?(mstange)
(In reply to Markus Stange [:mstange] from comment #9) > ... > > In CanvasRenderingContext2D, we call NeedToApplyFilter() before we construct > the AdjustedDrawTargetForFilter. NeedToApplyFilter() checks > state.filter.mPrimitives.Length(). Why is that check not saving us here? Because AdjustedTargetForFilter constructor calls UpdateFilter() before calling FilterSupport::ComputeSourceNeededRegions()? If UpdateFilter() changes things so that there are no primitives, we'll still end up proceeding.
Flags: needinfo?(mstange)
Hah! Neat.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Can we call UpdateFilter() before we create AdjustedTargetForFilter? Maybe the whole updateFilterOnWriteOnly if should go into NeedToApplyFilter(). Not sure I understand the purpose of updateFilterOnWriteOnly completely.
Tobias, see the question in comment 13.
Flags: needinfo?(tschneider)
Tracking this sec critical issue for 49/50.
tracking-firefox49: ? → +
tracking-firefox50: ? → +
Flags: sec-bounty?
Jet, this is a sec-critical - can you help us get the answer to comment 13? We're stuck until Tobias answers Markus' question, and we're now deep into 49 beta with this bug.
Flags: needinfo?(bugs)
Sorry for the delay here, will look into it right now.
Flags: needinfo?(tschneider)
Yes we could UpdateFilter() before we create AdjustedTargetForFilter, tho this will then run before every draw. At the moment we only call it when setting the filter property. Some filters use SourceGraphic and thus need to be updated whenever the canvas gets tainted (e.g. after a drawImage command). Thats what the updateFilterOnWriteOnly flag is for. If we call UpdateFilter() before every AdjustedTargetForFilter, we don't need updateFilterOnWriteOnly.
Flags: needinfo?(bugs)
Markus is this something you think we may be able to fix for beta, since we haven't shipped the regression yet? We are heading into beta 5 build tomorrow and the RC build is Sept. 5, just so you have a sense of the timing.
Flags: needinfo?(mstange)
I hope I'll be able to take a look later today, but the fix probably won't be reviewed in time for Monday morning, so I'll probably miss beta 6.
Flags: needinfo?(mstange)
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8773476 [details] [diff] [review] This seems to stop the crashes for me If we don't the real fix, or the real fix isn't appropriate for an uplift (this is sec-critical, so we may end up going to ESR), let's get this band-aid patch reviewed.
Attachment #8773476 - Flags: review?(mstange)
How confident are you in the fix? I also don't see this signature showing up in any volume in crash-stats. I would rather we come up with a good fix rather than something unsure that we have to put into late beta (heading into beta 7 now with a week left till we build the RC). If we feel that the risk is higher of shipping this regression we could still take a patch with sec-approval. Is there a good reason we didn't back out the regressing patch?
Flags: needinfo?(milan)
Right - my patch is a wallpaper - just deals with the symptom (null pointer), and stops the crash. The real fix deals with the cause, and makes it so that we never get the null pointer in that location. In other words, the real fix will make the wallpaper patch unnecessary. We do not have any crashes on this - not really - the few that show up are in 47. I do not, however, know how bad of an attack vector this would be, so I don't want a zero day :) Maybe somebody from security can chime in and tell us if this worries them more than usual.
Flags: needinfo?(milan)
Also, this needs sec approval (even if it isn't the real fix) Can you request it? Thanks!
Flags: needinfo?(mstange)
Your patch fixes what you described in comment 3 and comment 6: an empty array leading to an attempt to access element[-1]. But that should end up in an intentional crash (InvalidArrayIndex_CRASH()) and doesn't jibe with my crash in pixman from comment 1 (bp-c826173c-2f50-41bc-b80e-c8faa2160719) nor Nils' stack writing to 0x7f229cbe8f90 (near nsTArray::sEmptyHdr). Does InvalidArrayIndex_CRASH() get compiled away in release builds? The wallpaper patch is safe enough to uplift to beta, if it's fixing the right problem. I'd prefer all those MOZ_ASSERT()s be MOZ_RELEASE_ASSERT() instead (if we hit them we know we're in trouble). _That_ change might be risky for beta, but we're not seeing crashes in those functions and if those conditions are hit then I'd expect we're already crashing. There are 40+ crashes since July. Not many, but some. We could be missing a whole bunch, though, because a direct query doesn't pick up my crash from comment 1. Is there a way to search for FilterSupports in the stack when it's not the crashing signature? https://crash-stats.mozilla.com/search/?signature=~FilterSupport%3A%3A&date=%3E2016-07-01&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=address&_columns=reason&_columns=build_id&_columns=platform#crash-reports
The main part of comment 27 is a question for Milan on his patch, though mstange could answer as part of his review if he wants.
status-firefox47: unaffected → wontfix
status-firefox48: unaffected → wontfix
status-firefox-esr45: unaffected → affected
Flags: needinfo?(milan)
Comment on attachment 8773476 [details] [diff] [review] This seems to stop the crashes for me Review of attachment 8773476 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay here. Yes, this patch will fix this bug, and I think we should upgrade the MOZ_ASSERTs to MOZ_RELEASE_ASSERTs. They shouldn't ever fail, and if they do, crashing is better than continuing. And the checks are cheap. I'm having my doubts about the whole updateFilterOnWriteOnly mechanism - as far as I can tell, if ctx.filter is set while the canvas is still in the readable state, and if it then turns write-only after the second draw, updateFilterOnWriteOnly will be false, and the filter will not be updated, so feDisplacementFilters that take the source graphic as their map input will not be replaced with pass-through filters the way they should be. But that's a different bug which I will file tomorrow.
Attachment #8773476 - Flags: review?(mstange) → review+
The other thing I'd like to understand is why CanvasFilterChainObserver::DoUpdate isn't called when the referenced element is removed from the document.
Comment on attachment 8773476 [details] [diff] [review] This seems to stop the crashes for me [Security approval request comment] How easily could an exploit be constructed based on the patch? Not sure Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The assertions paint a bulls-eye on the problem but they don't hint on how you'd construct an exploit for it. Which older supported branches are affected by this flaw? Central, Aurora, and Beta. This hasn't been released yet. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be identical, the code hasn't changed since 49 How likely is this patch to cause regressions; how much testing does it need? Very unlikely - it's a wallpaper patch that only kicks in if we know we're in a bad place. Shouldn't need much testing.
Flags: needinfo?(mstange)
Attachment #8773476 - Flags: sec-approval?
(In reply to Daniel Veditz [:dveditz] from comment #27) > ... > > There are 40+ crashes since July. Not many, but some. We could be missing a > whole bunch, though, because a direct query doesn't pick up my crash from > comment 1. Is there a way to search for FilterSupports in the stack when > it's not the crashing signature? I count 33 of these crashes since July 1st, and 57 (so, let's say double) that have FilterSupport::ComputeSourceNeededRegions anywhere in the signature: https://crash-stats.mozilla.com/search/?proto_signature=~FilterSupport%3A%3AComputeSourceNeededRegions&date=%3E2016-07-01&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=address&_columns=reason&_columns=build_id&_columns=platform#crash-reports
Flags: needinfo?(milan)
I'll get the updated patch with release asserts.
See the earlier approval request.
Attachment #8773476 - Attachment is obsolete: true
Attachment #8773476 - Flags: sec-approval?
Attachment #8785347 - Flags: sec-approval?
Attachment #8785347 - Flags: review+
sec-approva+. Please nominated beta and aurora patches as well to see if we can try to get this fixed before 49 goes out the door with the flaw. I notice that the flags say ESR45 is affected but the sec-approval comments don't mention it as affected. Is it unaffected as you imply?
Flags: needinfo?(mstange)
Attachment #8785347 - Flags: sec-approval? → sec-approval+
(In reply to Markus Stange [:mstange] from comment #29) > I'm having my doubts about the whole updateFilterOnWriteOnly mechanism - as > far as I can tell, if ctx.filter is set while the canvas is still in the > readable state, and if it then turns write-only after the second draw, > updateFilterOnWriteOnly will be false, and the filter will not be updated, > so feDisplacementFilters that take the source graphic as their map input > will not be replaced with pass-through filters the way they should be. But > that's a different bug which I will file tomorrow. I filed bug 1298552 about this. (In reply to Markus Stange [:mstange] from comment #30) > The other thing I'd like to understand is why > CanvasFilterChainObserver::DoUpdate isn't called when the referenced element > is removed from the document. And bug 1298509 about this.
Comment on attachment 8785347 [details] [diff] [review] Release assert or deal with empty arrays. Carry r=mstange Approval Request Comment [Feature/regressing bug #]: enabling canvas filters by default, bug 1173545 [User impact if declined]: exploitable crashes [Describe test coverage new/current, TreeHerder]: some, but no test is being added for this crash [Risks and why]: low, we just bail out when we detect something's wrong [String/UUID change made/needed]: none
Flags: needinfo?(mstange)
Attachment #8785347 - Flags: approval-mozilla-beta?
Attachment #8785347 - Flags: approval-mozilla-aurora?
Comment on attachment 8785347 [details] [diff] [review] Release assert or deal with empty arrays. Carry r=mstange Fix for sec-critical issue, regression from 49. This should make it into beta 8.
Attachment #8785347 - Flags: approval-mozilla-beta?
Attachment #8785347 - Flags: approval-mozilla-beta+
Attachment #8785347 - Flags: approval-mozilla-aurora?
Attachment #8785347 - Flags: approval-mozilla-aurora+
(In reply to Al Billings [:abillings] from comment #35) > I notice that the flags say ESR45 is affected but the sec-approval comments > don't mention it as affected. Is it unaffected as you imply? This is a regression in 49 (bug 1174278), so 45 should not be affected.
status-firefox-esr45: affected → unaffected
Not sure if this should be leave-open for a "real" fix on trunk or not, but we should at least get the wallpaper patch landed before uplifting it. https://hg.mozilla.org/integration/mozilla-inbound/rev/e0acf6746962
Bug 1298552 has the real fix.
https://hg.mozilla.org/mozilla-central/rev/e0acf6746962
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: gfx-core-security → core-security-release
Whiteboard: [adv-main49+]
This is still crashing on FX 49 - 51, Win 7. https://crash-stats.mozilla.com/report/index/bd9e05f4-db87-45a2-96dd-314902160907 https://crash-stats.mozilla.com/report/index/62e0a4ac-5cc3-4d09-af84-d68282160907
Status: RESOLVED → REOPENED
Flags: needinfo?(mstange)
Resolution: FIXED → ---
Oh. :-( Yes, looking at the code again, only the check in the AdjustedTargetForFilter constructor allows failure; the release asserts in the destructor all crash. However, the big difference is that these crashes are safe crashes. They are not exploitable. Bug 1298552 will fix these crashes, but it's not going to be in 49.
Flags: needinfo?(mstange)
The good news is those two reports seem to be the only two with that signature so it doesn't look like we're tripping those MOZ_RELEASE_ASSERT's in normal use (at least with the beta population). We can still consider this bug "fixed" and follow-up in bug 1298552
Crash Signature: [@ _moz_pixman_region32_copy ]
Alias: CVE-2016-5275
Can this be closed now?
Flags: needinfo?(mstange)
Oh, yes. We haven't fixed the crash but we have fixed the exploitability.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Flags: needinfo?(mstange)
Resolution: --- → FIXED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: