Closed Bug 1292869 Opened 3 years ago Closed 3 years ago

Crash in imgRequestProxy::GetImagePrincipal, repeatable CRASH at particular web page "directory.libsyn.com"

Categories

(Core :: DOM: Security, defect, P1, critical)

50 Branch
Unspecified
All
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed

People

(Reporter: benl234, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, reproducible, Whiteboard: [domsecurity-active])

Crash Data

User Story

Some more investigation concerning possible DUPs from <http://shortlinks.de/tgs0> (or better <http://shortlinks.de/5vrm>) required. I doubt that one of them is a DUP.

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 SeaMonkey/2.47a1
Build ID: 20160731003204

Steps to reproduce:

Navigate to http://directory.libsyn.com/categories

Wait several seconds.  There will be on the page a spinny-thing that everyone knows means wait for something.




Actual results:

Seamonkey crashes.  Every time I've tried.


Expected results:

Seamonkey should show the webpage and whatever one is waiting for.

The page that crashes behaves like the main page, directory.libsyn.com , but that doesn't crash.

Crash reports (newest first):
bp-f63d4103-f08d-45db-9726-69a5f2160806
bp-4a24a1e6-3ef4-457c-9274-9662f2160806
bp-eb599c1b-8f38-4695-bfbc-cff152160806
REPRODUCIBLE with official English SeaMonkey 2.47a1  (NT 6.1; Win64; WOW64; rv:50.0)  Gecko/20100101 Firefox/50.0 Build 20160729184603  (Default Classic Theme)  on German WIN7 64bit
REPRODUCIBLE with official en-US SeaMonkey 2.48a1  (NT 6.1; WOW64; rv:51.0)  Gecko/20100101 Firefox/51.0 Build 20160804000913  (Default Classic Theme)  on German WIN7 64bit (also with newly created User Profile in Safe Mode)

Crash reports (latest at the top):
<http://crash-stats.mozilla.com/report/index/bp-5fb733f1-0fdd-490d-b430-fc7742160806>
<http://crash-stats.mozilla.com/report/index/bp-3a8330fe-35ab-4740-a53c-9c5a52160806>
<http://crash-stats.mozilla.com/report/index/bp-fbfaeb70-135c-482a-8047-f7a202160806>

NOT reproducible with  en-US SeaMonkey 2.40 final Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) from official download page,  Gecko/20100101  Firefox/ 43.0  Build 20160120202951, (Default Classic Theme) on German WIN7 64bit
Severity: normal → major
OS: Unspecified → Windows 7
Already REPRODUCIBLE with official English SeaMonkey 2.47a1  (NT 6.1; Win64; WOW64; rv:50.0)  Gecko/20100101 Firefox/50.0 Build 20160729184603  (Default Classic Theme)  on German WIN7 64bit

a) The error shown in screenshot does not appear with 2.40, but I can't 
   tell whether the error is related to crash.
b) No DUPs found with <http://shortlinks.de/ed4h>
c) Also FF 51.0a1 (2016-08-04) affected, crashes because of 
   "EXCEPTION_ACCESS_VIOLATION_READ":
   bp-5de6bd3e-73d1-4093-a547-2f9ca2160806

Some more investigation required
Severity: major → critical
User Story: (updated)
NEW for now, but DUP research here is beyond my skills
Status: UNCONFIRMED → NEW
User Story: (updated)
Ever confirmed: true
d) currently only 1 crashing page known, so "major"
d) I am pretty sure that this is a new CORE bug, Version of appearance
   calculated from SM version
Severity: critical → major
Product: SeaMonkey → Core
Summary: repeatable crash at directory.libsyn.com → repeatable CRASH at particular web page "directory.libsyn.com"
Version: SeaMonkey 2.47 Branch → 50 Branch
Severity: major → critical
Keywords: crash
Summary: repeatable CRASH at particular web page "directory.libsyn.com" → Crash in imgRequestProxy::GetImagePrincipal, repeatable CRASH at particular web page "directory.libsyn.com"
Crashes on Nightly51.0a1 and Aurora50.0a2 ubuntu14.04 64bit as well

bp-97a6d07f-c23d-4c2b-83c3-8b6c42160807
bp-58ac0d16-8434-416a-90c9-5570a2160807
OS: Windows 7 → All
[Tracking Requested - why for this release]:

crashes also beta builds it seems
(In reply to Carsten Book [:Tomcat] from comment #8)
> [Tracking Requested - why for this release]:
> 
> crashes also beta builds it seems

Do you have some examples?  We can't see anything but 50 and 51?
Flags: needinfo?(cbook)
I can reproduce the crash, please see stacktrace underneath:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffe6c7987b in imgRequestProxy::GetImagePrincipal (this=0x7fffbc0fa680, aPrincipal=0x7fffffffabd0) at /home/ckerschb/moz/mc/image/imgRequestProxy.cpp:671
671	  NS_ADDREF(*aPrincipal = GetOwner()->GetPrincipal());
(gdb) bt
#0  0x00007fffe6c7987b in imgRequestProxy::GetImagePrincipal (this=0x7fffbc0fa680, aPrincipal=0x7fffffffabd0) at /home/ckerschb/moz/mc/image/imgRequestProxy.cpp:671
#1  0x00007fffe7f49004 in mozilla::dom::CanvasRenderingContext2D::CachedSurfaceFromElement (this=0x7fffb23ed800, aElement=0x7fffd11c0260)
    at /home/ckerschb/moz/mc/dom/canvas/CanvasRenderingContext2D.cpp:4517
#2  0x00007fffe7f49d6a in mozilla::dom::CanvasRenderingContext2D::DrawImage (this=0x7fffb23ed800, aImage=..., aSx=0, aSy=0, aSw=0, aSh=0, aDx=0, aDy=0, 
    aDw=210,58333333333337, aDh=55,570601851851862, aOptional_argc=2 '\002', aError=...) at /home/ckerschb/moz/mc/dom/canvas/CanvasRenderingContext2D.cpp:4719
#3  0x00007fffe77b7771 in mozilla::dom::CanvasRenderingContext2D::DrawImage (this=0x7fffb23ed800, aImage=..., aDx=0, aDy=0, aDw=210,58333333333337, 
    aDh=55,570601851851862, aError=...) at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/mozilla/dom/CanvasRenderingContext2D.h:225
#4  0x00007fffe779ecf6 in mozilla::dom::CanvasRenderingContext2DBinding::drawImage (cx=0x7fffdbec3000, obj=..., self=0x7fffb23ed800, args=...)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/CanvasRenderingContext2DBinding.cpp:4240
#5  0x00007fffe7ec1e31 in mozilla::dom::GenericBindingMethod (cx=0x7fffdbec3000, argc=5, vp=0x7fffd4068280)
    at /home/ckerschb/moz/mc/dom/bindings/BindingUtils.cpp:2812
#6  0x00007fffeb505566 in js::CallJSNative (cx=0x7fffdbec3000, native=0x7fffe7ec1c01 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, 
    args=...) at /home/ckerschb/moz/mc/js/src/jscntxtinlines.h:235
#7  0x00007fffeb4ccc33 in js::InternalCallOrConstruct (cx=0x7fffdbec3000, args=..., construct=js::NO_CONSTRUCT)
    at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:453
#8  0x00007fffeb4ccf51 in InternalCall (cx=0x7fffdbec3000, args=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:498
#9  0x00007fffeb4ccf7b in js::CallFromStack (cx=0x7fffdbec3000, args=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:504
#10 0x00007fffeb4da0f9 in Interpret (cx=0x7fffdbec3000, state=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:2873
#11 0x00007fffeb4cc8b1 in js::RunScript (cx=0x7fffdbec3000, state=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:399
#12 0x00007fffeb4cccf3 in js::InternalCallOrConstruct (cx=0x7fffdbec3000, args=..., construct=js::NO_CONSTRUCT)
    at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:471
#13 0x00007fffeb4ccf51 in InternalCall (cx=0x7fffdbec3000, args=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:498
#14 0x00007fffeb4ccfe2 in js::Call (cx=0x7fffdbec3000, fval=..., thisv=..., args=..., rval=...) at /home/ckerschb/moz/mc/js/src/vm/Interpreter.cpp:517
#15 0x00007fffeb2646a7 in JS::Call (cx=0x7fffdbec3000, thisv=..., fval=..., args=..., rval=...) at /home/ckerschb/moz/mc/js/src/jsapi.cpp:2852
#16 0x00007fffe7d2e962 in mozilla::dom::Function::Call (this=0x7fffc73a6400, cx=0x7fffdbec3000, aThisVal=..., arguments=..., aRetVal=..., aRv=...)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dom/bindings/FunctionBinding.cpp:37
#17 0x00007fffe6de1811 in mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > (this=0x7fffc73a6400, thisVal=..., arguments=..., aRetVal=..., aRv=..., 
    aExecutionReason=0x7fffebf7f214 "setTimeout handler", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0)
    at /home/ckerschb/moz/mc-obj-ff-dbg/dist/include/mozilla/dom/FunctionBinding.h:70
#18 0x00007fffe6dc9c78 in nsGlobalWindow::RunTimeoutHandler (this=0x7fffcdc18000, aTimeout=0x7fffbf57cc80, aScx=0x7fffbeccdb00)
    at /home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:12305
#19 0x00007fffe6dca504 in nsGlobalWindow::RunTimeout (this=0x7fffcdc18000, aTimeout=0x7fffd41f4380) at /home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:12548
#20 0x00007fffe6dcaff6 in nsGlobalWindow::TimerCallback (aTimer=0x7fffbf5d7630, aClosure=0x7fffd41f4380) at /home/ckerschb/moz/mc/dom/base/nsGlobalWindow.cpp:12794
#21 0x00007fffe5437bd6 in nsTimerImpl::Fire (this=0x7fffbf5d7630) at /home/ckerschb/moz/mc/xpcom/threads/nsTimerImpl.cpp:521
#22 0x00007fffe54107c5 in nsTimerEvent::Run (this=0x7fffc727d170) at /home/ckerschb/moz/mc/xpcom/threads/TimerThread.cpp:286
#23 0x00007fffe5416705 in nsThread::ProcessNextEvent (this=0x7ffff6b7b500, aMayWait=false, aResult=0x7fffffffc6bf)
    at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1058
#24 0x00007fffe547f939 in NS_ProcessNextEvent (aThread=0x7ffff6b7b500, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:290
#25 0x00007fffe5bd3629 in mozilla::ipc::MessagePump::Run (this=0x7fffe1d93040, aDelegate=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:96
#26 0x00007fffe5b43b2d in MessageLoop::RunInternal (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:232
#27 0x00007fffe5b43ac2 in MessageLoop::RunHandler (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:225
#28 0x00007fffe5b43a9b in MessageLoop::Run (this=0x7ffff6b426e0) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:205
#29 0x00007fffe8e9e170 in nsBaseAppShell::Run (this=0x7fffd3415320) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156
#30 0x00007fffe9e49e21 in nsAppStartup::Run (this=0x7fffd34102e0) at /home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:284
I'll take this one and have a closer look.
Flags: needinfo?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Hey Jeff, 

the img request in question gets blocked by XCTO (see console message underneath) which then crashes  imgRequestProxy::GetImagePrincipal because the principal is a nullptr. I suppose we should use NS_IF_ADDREF. The callsite (CanvasRenderingContext2D.cpp:4517) actually checks for receiving a nullptr principal.

Visiting http://directory.libsyn.com/categories works now as expected.


%----snip----
The resource from “http://directory.libsyn.com/%3Cbr%20/%3E%3Cb%3EDeprecated%3C/b%3E:%20%20Non-static%20method%20Libsyn_Service_Asset::getCategoryImageUrl()%20should%20not%20be%20called%20statically,%20assuming%20$this%20from%20incompatible%20context%20in%20%3Cb%3E/www/libsyn/directory/application/views/scripts/categories/index.phtml%3C/b%3E%20on%20line%20%3Cb%3E37%3C/b%3E%3Cbr%20/%3Ehttps://ssl-static.libsyn.com/p/assets/platform/category/arts.png” was blocked due to MIME type mismatch (X-Content-Type-Options: nosniff).
Attachment #8779239 - Flags: review?(jmuizelaar)
Comment on attachment 8779239 [details] [diff] [review]
bug_1292869_imgrequestproxy_getimageprincipal.patch

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

::: image/imgRequestProxy.cpp
@@ +667,5 @@
>    if (!GetOwner()) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  NS_IF_ADDREF(*aPrincipal = GetOwner()->GetPrincipal());

Let's do this instead:

nsCOMPtr<nsIPrincipal> principal = GetOwner()->GetPrincipal();
principal.forget(aPrincipal);
Attachment #8779239 - Flags: review?(jmuizelaar) → review+
Thanks; incorporated nit; carrying over r+ from jmuizelaar
Attachment #8779239 - Attachment is obsolete: true
Attachment #8779658 - Flags: review+
(In reply to Milan Sreckovic [:milan] from comment #9)
> (In reply to Carsten Book [:Tomcat] from comment #8)
> > [Tracking Requested - why for this release]:
> > 
> > crashes also beta builds it seems
> 
> Do you have some examples?  We can't see anything but 50 and 51?

sorry seems the automation was wrong here, did some tests and beta is fine
Flags: needinfo?(cbook)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c39a0de6a1
Fix null deref in imgRequestProxy::GetImagePrincipal. r=jmuizelaar
Keywords: checkin-needed
adjusting the status flags according to comment #15
https://hg.mozilla.org/mozilla-central/rev/78c39a0de6a1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Tracking 50/51+ for this reproducible crash.
Sorry, the tracking flags didn't make the last submit.
Comment on attachment 8779658 [details] [diff] [review]
bug_1292869_imgrequestproxy_getimageprincipal.patch

Approval Request Comment
Given that 50 is affected we should uplift that small fix to avoid crashing if XCTO: nosniff header blocks certain loads that also go through imgRequestProxy.

[Feature/regressing bug #]:
Bug 471020 - Add X-Content-Type-Options: nosniff support to Firefox

[User impact if declined]:
Users visiting http://directory.libsyn.com/categories or any other site where our implementation of XCTO nosniff blocks a load that also goes through imgRequstProxy.

[Describe test coverage new/current, TreeHerder]:
Manual verification that browser doesn't crash when visiting http://directory.libsyn.com/categories.

[Risks and why]:
Low, only fixing a potential null deref.

[String/UUID change made/needed]:
no
Attachment #8779658 - Flags: approval-mozilla-beta?
Comment on attachment 8779658 [details] [diff] [review]
bug_1292869_imgrequestproxy_getimageprincipal.patch

This is already on 50, and I think 49 (in beta now) is unaffected (from looking at crash-stats and the regressing bug)
Attachment #8779658 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8779658 [details] [diff] [review]
bug_1292869_imgrequestproxy_getimageprincipal.patch

Let's bring this fix to aurora though! Thanks philipp for pointing out it wasn't in 50 yet.
Attachment #8779658 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> This is already on 50, and I think 49 (in beta now) is unaffected (from
> looking at crash-stats and the regressing bug)

Sorry, for the confusion. Obviously this was meant to be uplifted to 50 and not 49. Thanks for catching!
You need to log in before you can comment on or make changes to this bug.