Heap-use-after-free in mozilla::dom::CropDataSourceSurface

VERIFIED FIXED in Firefox 42, Firefox OS master

Status

()

Core
Canvas: 2D
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: Abhishek Arya, Assigned: kaku)

Tracking

({csectype-uaf, regression, sec-critical})

Trunk
mozilla43
csectype-uaf, regression, sec-critical
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox40 unaffected, firefox41 unaffected, firefox42+ verified, firefox43+ verified, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.5-])

Attachments

(8 attachments, 4 obsolete attachments)

619 bytes, text/html
Details
13.42 KB, patch
kaku
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
12.96 KB, patch
Details | Diff | Splinter Review
10.38 KB, patch
kaku
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
3.87 KB, patch
kaku
: review+
Details | Diff | Splinter Review
1.81 KB, patch
kaku
: review+
Details | Diff | Splinter Review
719 bytes, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Created attachment 8642198 [details]
canvas.html

=================================================================
==29983==ERROR: AddressSanitizer: heap-use-after-free on address 0x61b0000c7480 at pc 0x0000004a0b28 bp 0x7ffd442c4bd0 sp 0x7ffd442c4380
READ of size 120 at 0x61b0000c7480 thread T0 (Web Content)
    #0 0x4a0b27 in __asan_memcpy _asan_rtl_
    #1 0x7fd9a0fcf33d in mozilla::dom::CropDataSourceSurface(mozilla::gfx::DataSourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) dom/canvas/ImageBitmap.cpp:71:5
    #2 0x7fd9a0fd078e in mozilla::dom::CreateImageFromRawData(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, mozilla::gfx::SurfaceFormat, unsigned char*, unsigned int, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:132:38
    #3 0x7fd9a0fcfa3a in mozilla::dom::ImageBitmap::CreateInternal(nsIGlobalObject*, mozilla::dom::ImageData&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:622:12
    #4 0x7fd9a0fd3242 in mozilla::dom::ImageBitmap::Create(nsIGlobalObject*, mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:1069:19
    #5 0x7fd99efb7d7b in nsGlobalWindow::CreateImageBitmap(mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, int, int, int, int, mozilla::ErrorResult&) dom/base/nsGlobalWindow.cpp:14675:10
    #6 0x7fd9a062f7ae in mozilla::dom::WindowBinding::createImageBitmap_promiseWrapper(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/WindowBinding.cpp:11195:57
    #7 0x7fd9a061dee0 in mozilla::dom::WindowBinding::genericPromiseReturningMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/WindowBinding.cpp:13061:13
    #8 0x7fd9a60c26b8 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:235:15
    #9 0x7fd9a61027e0 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3073:18
    #10 0x7fd9a60e5035 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:752:12
    #11 0x7fd9a60c2dc2 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:829:15
    #12 0x7fd9a605f60b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:866:10
    #13 0x7fd9a6c61f8b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4624:12
    #14 0x7fd99fc7e698 in mozilla::dom::AnyCallback::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/PromiseBinding.cpp:78:8
    #15 0x7fd9a23fa5a3 in mozilla::dom::WrapperPromiseCallback::Call(JSContext*, JS::Handle<JS::Value>) objdir-ff-asan/dist/include/mozilla/dom/PromiseBinding.h:134:12
    #16 0x7fd9a2401fe9 in mozilla::dom::PromiseCallbackTask::Run() dom/promise/Promise.cpp:98:7
    #17 0x7fd9a23e664c in mozilla::dom::Promise::PerformMicroTaskCheckpoint() dom/promise/Promise.cpp:503:19
    #18 0x7fd99e1397af in non-virtual thunk to nsXPConnect::AfterProcessNextEvent(nsIThreadInternal*, unsigned int, bool) js/xpconnect/src/nsXPConnect.cpp:1017:5
    #19 0x7fd99cd186c3 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:885:5
    #20 0x7fd99cd8e4ac in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:277:10
    #21 0x7fd99d655eae in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95:21
    #22 0x7fd99d5df8a1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:234:3
    #23 0x7fd9a27e4baf in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:165:3
    #24 0x7fd9a4756b33 in XRE_RunAppShell toolkit/xre/nsEmbedFunctions.cpp:785:12
    #25 0x7fd99d5df8a1 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:234:3
    #26 0x7fd9a4756057 in XRE_InitChildProcess toolkit/xre/nsEmbedFunctions.cpp:621:7
    #27 0x4dbbf4 in content_process_main(int, char**) ipc/contentproc/plugin-container.cpp:237:19
    #28 0x7fd99a0b0ec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
0x61b0000c7480 is located 768 bytes inside of 1424-byte region [0x61b0000c7180,0x61b0000c7710)
freed by thread T0 (Web Content) here:
    #0 0x4b6020 in __interceptor_free _asan_rtl_
    #1 0x7fd99e7a808f in mozilla::gfx::DrawTargetCairo::~DrawTargetCairo() gfx/2d/DrawTargetCairo.cpp:597:3
    #2 0x7fd99e7a82fd in mozilla::gfx::DrawTargetCairo::~DrawTargetCairo() gfx/2d/DrawTargetCairo.cpp:596:1
    #3 0x7fd99ed706ac in mozilla::image::RasterImage::CopyFrame(unsigned int, unsigned int) objdir-ff-asan/dist/include/mozilla/RefPtr.h:142:7
    #4 0x7fd99ed70f7b in mozilla::image::RasterImage::GetFrameInternal(unsigned int, unsigned int) image/RasterImage.cpp:736:17
    #5 0x7fd99ed70c4d in mozilla::image::RasterImage::GetFrame(unsigned int, unsigned int) image/RasterImage.cpp:697:10
    #6 0x7fd9a3125296 in nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, unsigned int, mozilla::gfx::DrawTarget*) layout/base/nsLayoutUtils.cpp:6765:29
    #7 0x7fd9a3125c8b in nsLayoutUtils::SurfaceFromElement(mozilla::dom::HTMLImageElement*, unsigned int, mozilla::gfx::DrawTarget*) layout/base/nsLayoutUtils.cpp:6805:10
    #8 0x7fd9a0fcb942 in mozilla::dom::ImageBitmap::CreateInternal(nsIGlobalObject*, mozilla::dom::HTMLImageElement&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:293:5
    #9 0x7fd9a0fd2e57 in mozilla::dom::ImageBitmap::Create(nsIGlobalObject*, mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:1059:19
    #10 0x7fd99efb7b48 in nsGlobalWindow::CreateImageBitmap(mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, mozilla::ErrorResult&) dom/base/nsGlobalWindow.cpp:14667:10
    #11 0x7fd9a062fc7f in mozilla::dom::WindowBinding::createImageBitmap_promiseWrapper(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/WindowBinding.cpp:11142:57
    #12 0x7fd9a061dee0 in mozilla::dom::WindowBinding::genericPromiseReturningMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/WindowBinding.cpp:13061:13
    #13 0x7fd9a60c26b8 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:235:15
    #14 0x7fd9a61027e0 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3073:18
    #15 0x7fd9a60e5035 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:752:12
    #16 0x7fd9a60c2dc2 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:829:15
    #17 0x7fd9a605f60b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:866:10
    #18 0x7fd9a6c61f8b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4624:12
    #19 0x7fd9a0a0575f in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/EventHandlerBinding.cpp:259:37
    #20 0x7fd9a12a4c0b in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) objdir-ff-asan/dist/include/mozilla/dom/EventHandlerBinding.h:351:12
    #21 0x7fd9a125dea4 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:998:16
    #22 0x7fd9a125f6fd in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1147:15
    #23 0x7fd9a124cffc in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:299:5
    #24 0x7fd9a12517cb in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) dom/events/EventDispatcher.cpp:635:9
    #25 0x7fd9a122948a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) dom/events/EventDispatcher.cpp:702:12
    #26 0x7fd99f347711 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) dom/base/nsINode.cpp:1294:5
    #27 0x7fd9a120aeba in mozilla::AsyncEventDispatcher::Run() dom/events/AsyncEventDispatcher.cpp:51:3
    #28 0x7fd99cd181d6 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:867:7
    #29 0x7fd99cd8e4ac in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:277:10

previously allocated by thread T0 (Web Content) here:
    #0 0x4b6338 in __interceptor_malloc _asan_rtl_
    #1 0x7fd9a501fbdd in INT__moz_cairo_create gfx/cairo/cairo/src/cairo.c:250:13
    #2 0x7fd99e7b8ee9 in mozilla::gfx::DrawTargetCairo::InitAlreadyReferenced(_cairo_surface*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat*) gfx/2d/DrawTargetCairo.cpp:1578:14
    #3 0x7fd99e7ba5a1 in mozilla::gfx::DrawTargetCairo::Init(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat) gfx/2d/DrawTargetCairo.cpp:1671:10
    #4 0x7fd99e7e549b in mozilla::gfx::Factory::CreateDrawTargetForData(mozilla::gfx::BackendType, unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat) gfx/2d/Factory.cpp:397:11
    #5 0x7fd99ed6ff82 in mozilla::image::RasterImage::CopyFrame(unsigned int, unsigned int) image/RasterImage.cpp:657:5
    #6 0x7fd99ed70f7b in mozilla::image::RasterImage::GetFrameInternal(unsigned int, unsigned int) image/RasterImage.cpp:736:17
    #7 0x7fd99ed70c4d in mozilla::image::RasterImage::GetFrame(unsigned int, unsigned int) image/RasterImage.cpp:697:10
    #8 0x7fd9a3125296 in nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, unsigned int, mozilla::gfx::DrawTarget*) layout/base/nsLayoutUtils.cpp:6765:29
    #9 0x7fd9a3125c8b in nsLayoutUtils::SurfaceFromElement(mozilla::dom::HTMLImageElement*, unsigned int, mozilla::gfx::DrawTarget*) layout/base/nsLayoutUtils.cpp:6805:10
    #10 0x7fd9a0fcb942 in mozilla::dom::ImageBitmap::CreateInternal(nsIGlobalObject*, mozilla::dom::HTMLImageElement&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:293:5
    #11 0x7fd9a0fd2e57 in mozilla::dom::ImageBitmap::Create(nsIGlobalObject*, mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, mozilla::ErrorResult&) dom/canvas/ImageBitmap.cpp:1059:19
    #12 0x7fd99efb7b48 in nsGlobalWindow::CreateImageBitmap(mozilla::dom::HTMLImageElementOrHTMLVideoElementOrHTMLCanvasElementOrBlobOrImageDataOrCanvasRenderingContext2DOrImageBitmap const&, mozilla::ErrorResult&) dom/base/nsGlobalWindow.cpp:14667:10
    #13 0x7fd9a062fc7f in mozilla::dom::WindowBinding::createImageBitmap_promiseWrapper(JSContext*, JS::Handle<JSObject*>, nsGlobalWindow*, JSJitMethodCallArgs const&) objdir-ff-asan/dom/bindings/WindowBinding.cpp:11142:57
    #14 0x7fd9a061dee0 in mozilla::dom::WindowBinding::genericPromiseReturningMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/WindowBinding.cpp:13061:13
    #15 0x7fd9a60c26b8 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jscntxtinlines.h:235:15
    #16 0x7fd9a61027e0 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3073:18
    #17 0x7fd9a60e5035 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:752:12
    #18 0x7fd9a60c2dc2 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:829:15
    #19 0x7fd9a605f60b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:866:10
    #20 0x7fd9a6c61f8b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4624:12
    #21 0x7fd9a0a0575f in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) objdir-ff-asan/dom/bindings/EventHandlerBinding.cpp:259:37
    #22 0x7fd9a12a4c0b in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) objdir-ff-asan/dist/include/mozilla/dom/EventHandlerBinding.h:351:12
    #23 0x7fd9a125dea4 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:998:16
    #24 0x7fd9a125f6fd in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1147:15
    #25 0x7fd9a124cffc in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:299:5
    #26 0x7fd9a12517cb in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) dom/events/EventDispatcher.cpp:635:9
    #27 0x7fd9a122948a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) dom/events/EventDispatcher.cpp:702:12
    #28 0x7fd99f347711 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) dom/base/nsINode.cpp:1294:5
    #29 0x7fd9a120aeba in mozilla::AsyncEventDispatcher::Run() dom/events/AsyncEventDispatcher.cpp:51:3

Shadow bytes around the buggy address:
  0x0c3680010e40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010e50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010e60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010e70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010e80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c3680010e90:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010ea0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010eb0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010ec0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010ed0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3680010ee0: fd fd fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==29983==ABORTING
Blocks: 1044102
Flags: needinfo?(tkuo)
Keywords: sec-critical
[Tracking Requested - why for this release]:
New feature in 42. This security critical issue needs to be fixed, or the API implementation backed out or put behind some flag which is disabled by default.
I suggest fixing this ;)
tracking-firefox42: --- → ?
(Assignee)

Comment 2

2 years ago
(In reply to Olli Pettay [:smaug] from comment #1)
> [Tracking Requested - why for this release]:
> New feature in 42. This security critical issue needs to be fixed, or the
> API implementation backed out or put behind some flag which is disabled by
> default.
> I suggest fixing this ;)

Yes, I would like to figure out and fix it! Let me check first.
Flags: needinfo?(tkuo)
(Assignee)

Updated

2 years ago
Assignee: nobody → tkuo
Keywords: csectype-uaf

Comment 3

2 years ago
Tracking for 42 because critical, let's try to get this fixed in Nightly this week.
status-firefox41: --- → unaffected
tracking-firefox42: ? → +
(Assignee)

Comment 4

2 years ago
So, here is the root cause:
Previously, we did not crop the source in the CreateInternal(), and the cropping area, _aCropRect_, is handled in the SetPictureRect() (for negative dimension) and in the PrepareForDrawTarget() (for out of scope). But, while I was trying to do cropping in the CreateInternal(), I missed the checking (see Bug1044102, Comment210).
(Assignee)

Comment 5

2 years ago
As described in comment#4 that the root cause is accessing illeadgel memory while cropping and copying in CropAndCopyDataSourceSurface() method. So, in part1.patch, I add a set of test cases which do different kinds of cropping combined with all possible sources. Then, I try to solve this bug in part2.patch.

However, the test cases also capture other bugs that is not really related to the topic of this bug. I also address them in part3.patch and part4.patch.

Part3.patch deals with the issue that we should create an ImageBitmap whose size is equal to the given cropping area (if given). 

Part4.patch gets rid of an assertion on the surface type of decoded blob. The original assumption is not right.
(Assignee)

Comment 6

2 years ago
Created attachment 8644495 [details] [diff] [review]
Bug1190210-Part0-Test-cases.patch

Test cases which do different kinds of cropping combined with all possible sources.
Attachment #8644495 - Flags: review?(bugs)
(Assignee)

Comment 7

2 years ago
Created attachment 8644496 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch

Solve the illegal memory access in the CropAndCopyDataSourceSurface() function.
Attachment #8644496 - Flags: review?(bugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8644497 [details] [diff] [review]
Bug1190210-Part2-Make-sure-the-size-of-created-Image.patch

Part3.patch deals with the issue that we should create an ImageBitmap whose size is equal to the given cropping area (if given).
Attachment #8644497 - Flags: review?(bugs)
(Assignee)

Comment 9

2 years ago
Created attachment 8644499 [details] [diff] [review]
Bug1190210-Part3-Fix-the-assertion-in-create-from-bl.patch

Part4.patch gets rid of an assertion on the surface type of decoded blob. The original assumption is not right.
Attachment #8644499 - Flags: review?(bugs)
Comment on attachment 8644495 [details] [diff] [review]
Bug1190210-Part0-Test-cases.patch

>+var TEST_BITMAPS = [
It took some time to understand what 'test' object means in TEST_BITMAPS.
Could you add some comment about the structure of TEST_BITMAPS

>+    // Cropping area is eaactlly the same of source surface.
exactly the same as ... 
or something like that.

>+    {'rect': [0,    0,    320, 240],  'test': [[0,    0,    [255, 255, 255, 255], 5],
>+                                               [50,   0,    [255, 255, 0,   255], 5]]},
>+    // Cropping area compltetly covers the source surface.
completely
Attachment #8644495 - Flags: review?(bugs) → review+
Comment on attachment 8644496 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch


> /*
>+ * If either aRect.x or aRect.y are negative, then return a new IntRect which
>+ * represents the same rectangle as the aRect does but with positive width and
>+ * height.
>+ */
>+static IntRect
>+FixUpNegativeDimension(const IntRect& aRect, ErrorResult& aRv)
This method doesn't do what the comment says.
The comment talks about negative aRect.x/y, but the method deals with negative rect.width/height 
So, please fix the comment.


>+  const SurfaceFormat format = SurfaceFormat::B8G8R8A8;
>+  const int BYTES_PER_PIXEL = BytesPerPixel(format);
unusual to use capital letters for a variable.
Why not just bytesPerPixel

>+  const IntSize  dstSize = gfx::IntSize(positiveCropRect.width,
extra space before dstSize
Attachment #8644496 - Flags: review?(bugs) → review+
Comment on attachment 8644497 [details] [diff] [review]
Bug1190210-Part2-Make-sure-the-size-of-created-Image.patch

I know very little about different backends, so some gfx person should look at this too.
Attachment #8644497 - Flags: review?(bugs) → review?(roc)
Comment on attachment 8644499 [details] [diff] [review]
Bug1190210-Part3-Fix-the-assertion-in-create-from-bl.patch

>+    //
>+    // The _sruface_ might
surface
Attachment #8644499 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

2 years ago
I tried these patches before uploading for review:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1044961d7b6c

The test_imagebitmap_cropping.html all failed since "Result logged after SimpleTest.finish()".

Here is the problem, in the resolve-callback of Promise.then(), I should also return a promise.
> function runTests() {
>  
>   prepareSources().
>     then(function() { testCropping(gImage); } ).
>     then(function() { testCropping(gVideo); } ).
>     then(function() { testCropping(gCanvas); } ).
>     then(function() { testCropping(gCtx); } ).
>     then(function() { testCropping(gImageData); } ).
>     then(function() { testCropping(gImageBitmap); } ).
>     then(function() { testCropping(gPNGBlob); } ).
>     then(function() { testCropping(gJPEGBlob); } ).
>     then(SimpleTest.finish, function(ev) { failed(ev); SimpleTest.finish(); });
> }

And since the different sources do not need to be tested in order, I will modify the above code as:

function runTests() {

  prepareSources().
    then( function() { return Promise.all([testCropping(gImage),
                                           testCropping(gVideo),
                                           testCropping(gCanvas),
                                           testCropping(gCtx),
                                           testCropping(gImageData),
                                           testCropping(gImageBitmap),
                                           testCropping(gPNGBlob),
                                           testCropping(gJPEGBlob)]); }).
    then(SimpleTest.finish, function(ev) { failed(ev); SimpleTest.finish(); });
}
Comment on attachment 8644497 [details] [diff] [review]
Bug1190210-Part2-Make-sure-the-size-of-created-Image.patch

Review of attachment 8644497 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/ImageBitmap.cpp
@@ +484,5 @@
> +
> +      mSurface = CropAndCopyDataSourceSurface(dataSurface, mPictureRect);
> +    } else {
> +      target->CopySurface(mSurface, surfPortion, dest);
> +      mSurface = target->Snapshot();

I don't think we should be fixing this D2D bug here in this code. This seems like something that should be fixed in Moz2D CopySurface. But I guess we can do this for now.
Attachment #8644497 - Flags: review?(roc) → review+
(Assignee)

Comment 16

2 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> ::: dom/canvas/ImageBitmap.cpp
> @@ +484,5 @@
> > +
> > +      mSurface = CropAndCopyDataSourceSurface(dataSurface, mPictureRect);
> > +    } else {
> > +      target->CopySurface(mSurface, surfPortion, dest);
> > +      mSurface = target->Snapshot();
> 
> I don't think we should be fixing this D2D bug here in this code. This seems
> like something that should be fixed in Moz2D CopySurface. But I guess we can
> do this for now.
Yes, I was waiting for the ImageBitmap landed into mozilla-central so that I could open another bug for fixing the behavior of DrawTargetD2D1::CopySurface() with sample codes in ImageBitmap.cpp to better explain the issue. However, this bug came first so I did this work-around. I will open the seperated bug after this one is landed.
(Assignee)

Comment 17

2 years ago
(In reply to Olli Pettay [:smaug] from comment #10)
> >+var TEST_BITMAPS = [
> It took some time to understand what 'test' object means in TEST_BITMAPS.
> Could you add some comment about the structure of TEST_BITMAPS
Sure, I will also associate a property name to each items in this structure.

Ans Ok to all other parts, thanks!
(Assignee)

Comment 18

2 years ago
Created attachment 8644836 [details] [diff] [review]
Bug1190210-Part0-Test-cases.-r-smaug.patch (carry r+:smaug)
Attachment #8644495 - Attachment is obsolete: true
Attachment #8644836 - Flags: review+
(Assignee)

Comment 19

2 years ago
Created attachment 8644837 [details] [diff] [review]
interdiff-part0.patch
(Assignee)

Comment 20

2 years ago
Created attachment 8644838 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)
Attachment #8644496 - Attachment is obsolete: true
Attachment #8644838 - Flags: review+
(Assignee)

Comment 21

2 years ago
Created attachment 8644839 [details] [diff] [review]
interdiff-part1.patch
(Assignee)

Comment 22

2 years ago
Created attachment 8644840 [details] [diff] [review]
Bug1190210-Part2-Make-sure-the-size-of-created-Image.patch (carry r+:roc)
Attachment #8644497 - Attachment is obsolete: true
Attachment #8644840 - Flags: review+
(Assignee)

Comment 23

2 years ago
Created attachment 8644841 [details] [diff] [review]
Bug1190210-Part3-Fix-the-assertion-in-create-from-bl.patch (carry r+:smaug)
Attachment #8644499 - Attachment is obsolete: true
Attachment #8644841 - Flags: review+
(Assignee)

Comment 24

2 years ago
Created attachment 8644842 [details] [diff] [review]
interdiff-part3.patch
(Assignee)

Comment 25

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3554cfd24af
(Assignee)

Comment 26

2 years ago
Try looks good.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

2 years ago
Thanks for all the reviews!
I'm not going to be able to get this on to m-c before today's uplift. Please request sec-approval on this since it's soon to be affecting Aurora as well.
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-master: --- → affected
status-firefox40: --- → unaffected
status-firefox43: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Flags: needinfo?(tkuo)
Keywords: checkin-needed
Comment on attachment 8644838 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)

Review of attachment 8644838 [details] [diff] [review]:
-----------------------------------------------------------------

Are we sure that negative width and height on rectangles is OK in the first place?  For example, IsEmpty() returns true when those values are negative, a lot of other stuff seems like it wouldn't work.  I could be wrong though.
(Assignee)

Comment 30

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #29)
> Comment on attachment 8644838 [details] [diff] [review]
> Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)
> 
> Review of attachment 8644838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are we sure that negative width and height on rectangles is OK in the first
> place?  For example, IsEmpty() returns true when those values are negative,
> a lot of other stuff seems like it wouldn't work.  I could be wrong though.

The cropping rectangle is used only after the negative dimension is fixed. Before that, the rectangle is only passed around. So, it is ok in this code. However, I think it will be much more clear if we do the negative-dimension-fixing at the very beginning of ImageBitmap::create(), and maybe in another bug, what do you think?
Flags: needinfo?(milan)
As long as the "bad" rectangle is contained inside of this function, we should be good.
Flags: needinfo?(milan)
(Assignee)

Comment 32

2 years ago
Comment on attachment 8644836 [details] [diff] [review]
Bug1190210-Part0-Test-cases.-r-smaug.patch (carry r+:smaug)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Ans: This patch contains only test cases. People might be able to know what we want to test and avoid.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Ans: So, test cases in this patch try to crop the ImageBitmap with various rectangles. The cropping rectangles might not overlap with the ImageBitmap, which try to test the illegal memory access in the original implementation.

Which older supported branches are affected by this flaw?
Ans: The original implementation of ImageBitmap (Bug1044102) has landed into m-c.

If not all supported branches, which bug introduced the flaw?
Ans: Bug1044102.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Ans: Bug1044102 is the very first implementation of ImageBitmap, just backout it should be enough.

How likely is this patch to cause regressions; how much testing does it need?
Ans: This patch will only affect the ImageBitmap implementation (Bug1044102) which was landed just one week ago and there should has no one users. All the test cases are in the Bug1044102 and this patch.
Flags: needinfo?(tkuo)
Attachment #8644836 - Flags: sec-approval?
(Assignee)

Comment 33

2 years ago
Comment on attachment 8644838 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Ans: This patch modify the CropAndCopyDataSourceSurface() function to avoid illegal memory access. People should have no way to touch an illegal memory address anymore.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Ans: The diff of the implementation of CropAndCopyDataSourceSurface() function shows that we are going to avoid illegal memory access..

Which older supported branches are affected by this flaw?
Ans: The original implementation of ImageBitmap (Bug1044102) has landed into m-c.

If not all supported branches, which bug introduced the flaw?
Ans: Bug1044102.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Ans: Bug1044102 is the very first implementation of ImageBitmap, just backout it should be enough.

How likely is this patch to cause regressions; how much testing does it need?
Ans: This patch will only affect the ImageBitmap implementation (Bug1044102) which was landed just one week ago and there should has no one users. All the test cases are in the Bug1044102 and this patch.
Attachment #8644838 - Flags: sec-approval?
(Assignee)

Comment 34

2 years ago
Patch part2 and part3 are not related to the security issue.
If this bug is only on Mozilla Central, it does not need sec-approval. See the policy for details at https://wiki.mozilla.org/Security/Bug_Approval_Process

If this bug is on more than Mozilla Central, the tests aren't going to be approved to land until well after we ship the fix in a release build to the public. We wouldn't want to check in public tests that expose a security problem until folks are protected from it. 

Is https://bugzilla.mozilla.org/show_bug.cgi?id=1044102#c265 the landing of the bug that causes the security issue?
Flags: needinfo?(tkuo)
(Assignee)

Comment 36

2 years ago
(In reply to Al Billings [:abillings] from comment #35)
> If this bug is only on Mozilla Central, it does not need sec-approval. See
> the policy for details at
> https://wiki.mozilla.org/Security/Bug_Approval_Process
> 
> If this bug is on more than Mozilla Central, the tests aren't going to be
> approved to land until well after we ship the fix in a release build to the
> public. We wouldn't want to check in public tests that expose a security
> problem until folks are protected from it. 
> 
> Is https://bugzilla.mozilla.org/show_bug.cgi?id=1044102#c265 the landing of
> the bug that causes the security issue?
Yes, it is and that's why I think it is one m-c only, is it?
Flags: needinfo?(tkuo) → needinfo?(abillings)
(Assignee)

Comment 37

2 years ago
Sorry for typo, one -> on.
The uplift was on Monday. This is on Aurora too now. See comment 28.
Comment on attachment 8644836 [details] [diff] [review]
Bug1190210-Part0-Test-cases.-r-smaug.patch (carry r+:smaug)

Giving sec-approval+.

You need to request Aurora approval and get it onto Aurora as soon as it is on trunk and trunk is green.
Flags: needinfo?(abillings)
Attachment #8644836 - Flags: sec-approval? → sec-approval+
Attachment #8644838 - Flags: sec-approval? → sec-approval+
tracking-firefox43: --- → +
This already has a test that triggers it, right?
(Assignee)

Comment 41

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #40)
> This already has a test that triggers it, right?

No test cases in Bug1044102 trigger this bug.
Arya posted one which is attachment 8642198 [details].
(Assignee)

Comment 42

2 years ago
(In reply to [On PTO until 8/24. Call :dveditz for help] from comment #39)
> Comment on attachment 8644836 [details] [diff] [review]
> Bug1190210-Part0-Test-cases.-r-smaug.patch (carry r+:smaug)
> 
> Giving sec-approval+.
> 
> You need to request Aurora approval and get it onto Aurora as soon as it is
> on trunk and trunk is green.

Just for double confirm, this bug now is ready for get into trunk (m-c), right?
Flags: needinfo?(ryanvm)
Once it has sec-approval, it's good to land, yes :)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8c950d08ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25a897f1e9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa165b74218
https://hg.mozilla.org/integration/mozilla-inbound/rev/230bde30c650
Keywords: checkin-needed
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/9a8c950d08ef
https://hg.mozilla.org/mozilla-central/rev/d25a897f1e9d
https://hg.mozilla.org/mozilla-central/rev/dfa165b74218
https://hg.mozilla.org/mozilla-central/rev/230bde30c650
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 46

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> Once it has sec-approval, it's good to land, yes :)

Got it, thank you!
(Assignee)

Comment 47

2 years ago
The try of m-c is green:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=db4616cd0721

Requesting Aurora approval.
(Assignee)

Comment 48

2 years ago
Comment on attachment 8644838 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)

Approval Request Comment
[Feature/regressing bug #]: Bug1044102
[User impact if declined]: The browser crashes while invoking createImageBitmap() with cropping area outside the source image.
[Describe test coverage new/current, TreeHerder]: Part0.patch contains various cropping areas which should be enough. The aurora try result is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=230792db1f61. B2G Desktop Linux x64 opt - Gu fails but does not related to this bug.
[Risks and why]: No risks since this bug only affect the code introduced in Bug1044102 and no one use the code of Bug1044102 yet.
[String/UUID change made/needed]: NONE.
Attachment #8644838 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 49

2 years ago
According to comment#35, we should not uplift the testing case path (Attachment #8644836 [details] [diff]) to Aurora, right?
Flags: needinfo?(ryanvm)
That's a question for Al, the person who left the comment :)
Flags: needinfo?(ryanvm) → needinfo?(abillings)
We can uplift the test to Aurora.
Flags: needinfo?(abillings)
(Assignee)

Comment 52

2 years ago
Comment on attachment 8644836 [details] [diff] [review]
Bug1190210-Part0-Test-cases.-r-smaug.patch (carry r+:smaug)

Approval Request Comment
[Feature/regressing bug #]: 
Bug1044102

[User impact if declined]: 
The browser crashes while invoking createImageBitmap() with cropping area outside the source image.

[Describe test coverage new/current, TreeHerder]: 
Part0.patch contains various cropping areas which should be enough. The aurora try result is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=230792db1f61. B2G Desktop Linux x64 opt - Gu fails but does not related to this bug.

[Risks and why]: 
No risks since this bug only affect the code introduced in Bug1044102 and no one use the code of Bug1044102 yet.

[String/UUID change made/needed]: NONE.
Attachment #8644836 - Flags: approval-mozilla-aurora?
Attachment #8644836 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8644838 [details] [diff] [review]
Bug1190210-Part1-Avoid-wrong-memory-accessing-in-Cro.patch (carry r+:smaug)

The patch is pretty big but as we never shipped with this, taking it.
Attachment #8644838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7f7113240b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/9596a39c3e78
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ba0588344a2
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3402c8dcdd4
status-b2g-master: affected → fixed
status-firefox42: affected → fixed

Updated

2 years ago
Group: core-security → core-security-release
Reproduced the initial assertion using old Nightly asan build from 2015-08-02 with the help of canvas.html. Verified that using Firefox 42.0 RC and latest Developer Edition 43.0a2, no assertion and no crash are recorded across platforms (Ubuntu 14.04 64-bit, Windows 10 32-bit and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
(Assignee)

Comment 56

2 years ago
Thanks for the verification!
Keywords: regression
Group: core-security-release
Whiteboard: [b2g-adv-main2.5-]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
You need to log in before you can comment on or make changes to this bug.