When running the attached testcase file, firefox 3.6.23 crashes in Linux(Ubuntu 10.04) and Windows XP caused by a NULL pointer dereference. I assume all OS platforms are affected. Crash ID Windows XP: bp-ef264216-4f0c-4b36-8c24-6d9c22111004 Ubuntu 10.04: Program received signal SIGSEGV, Segmentation fault. 0x01057804 in nsObjectLoadingContent::IsSuccessfulRequest (aRequest=0x0) at /mozilla-1.9.2/content/base/src/nsObjectLoadingContent.cpp:1520 1520 nsresult rv = aRequest->GetStatus(&status); backtrace: (gdb) bt 5 #0 0x01057804 in nsObjectLoadingContent::IsSuccessfulRequest (aRequest=0x0) at /mozilla-1.9.2/content/base/src/nsObjectLoadingContent.cpp:1520 #1 0x01054086 in nsObjectLoadingContent::OnStartRequest (this=0xb00d84e8, aRequest=0x0, aContext=0xac39b8f0) at /mozilla-1.9.2/content/base/src/nsObjectLoadingContent.cpp:506 #2 0x01d4935d in NS_InvokeByIndex_P () from ./libxul.so #3 0x00a278af in XPCWrappedNative::CallMethod (ccx=..., mode=CALL_METHOD) at /mozilla-1.9.2/js/src/xpconnect/src/xpcwrappednative.cpp:2722 #4 0x00a35027 in XPC_WN_CallMethod (cx=0xb2beb000, obj=0xb4feb220, argc=2, argv=0xb2dda0b8, vp=0xbf8971e4) at /mozilla-1.9.2/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740 Using an object html tag, the nsIRequestObserver interface can be queried via QueryInterface of <object>. Then the method OnStartRequest can be called in javascript using null for aRequest (see testcase.html). In /mozilla-1.9.2/content/base/src/nsObjectLoadingContent.cpp : 490 // nsIRequestObserver 491 NS_IMETHODIMP 492 nsObjectLoadingContent::OnStartRequest(nsIRequest *aRequest, 493 nsISupports *aContext) 494 { 495 if (aRequest != mChannel) { 496 // This is a bit of an edge case - happens when a new load starts before the 497 // previous one got here 498 return NS_BINDING_ABORTED; 499 } 500 501 // We're done with the classifier 502 mClassifier = nsnull; 503 504 AutoNotifier notifier(this, PR_TRUE); 505 506 if (!IsSuccessfulRequest(aRequest)) { 507 LOG(("OBJLC [%p]: OnStartRequest: Request failed\n", this)); 508 Fallback(PR_FALSE); 509 return NS_BINDING_ABORTED; 510 } As mChannel == 0x0 and aRequest == 0x0 the aborting in line 495 is not entered. In line 506 IsSuccessfulRequest is called with aRequest = 0x0: 1516 /* static */ PRBool 1517 nsObjectLoadingContent::IsSuccessfulRequest(nsIRequest* aRequest) 1518 { 1519 nsresult status; 1520 nsresult rv = aRequest->GetStatus(&status); 1521 if (NS_FAILED(rv) || NS_FAILED(status)) { 1522 return PR_FALSE; 1523 } This leads to the NULL pointer dereference when trying to call the GetStatus method of aRequest in line 1520: (gdb) i r eax 0x0 0 ecx 0xb00d8508 -1341291256 edx 0xbf896b60 -1081513120 ebx 0x23c7ff4 37519348 esp 0xbf896ab4 0xbf896ab4 ebp 0xbf896aec 0xbf896aec esi 0xac373594 -1405667948 edi 0xb4feb220 -1258376672 eip 0x1057804 0x1057804 <nsObjectLoadingContent::IsSuccessfulRequest(nsIRequest*)+10> eflags 0x210286 [ PF SF IF RF ID ] cs 0x73 115 ss 0x7b 123 ds 0x7b 123 es 0x7b 123 fs 0x0 0 gs 0x33 51 (gdb) disas Dump of assembler code for function _ZN22nsObjectLoadingContent19IsSuccessfulRequestEP10nsIRequest: 0x010577fa <+0>: push %ebp 0x010577fb <+1>: mov %esp,%ebp 0x010577fd <+3>: push %esi 0x010577fe <+4>: sub $0x34,%esp 0x01057801 <+7>: mov 0x8(%ebp),%eax => 0x01057804 <+10>: mov (%eax),%eax 0x01057806 <+12>: add $0x14,%eax 0x01057809 <+15>: mov (%eax),%edx 0x0105780b <+17>: lea -0xc(%ebp),%eax Steps to reproduce: 1) Open testcase file 2) Click the button The simplest patch would be to nullcheck aRequest in /mozilla-1.9.2/content/base/src/nsObjectLoadingContent.cpp line 495: 495c495 < if (aRequest != mChannel) { --- > if (aRequest != mChannel || !aRequest) { I assume this is a [sg:dos], but I don't know yet if other attack vectors are possible (e.g.: 1) Filling mChannel with a custom nsIRequest object via data=".." in the html object tag, 2) setting an aRequest pointer equal to mChannel and passing it to OnStartRequest and 3) executing arbitrary code via aRequest->GetStatus(&status) ). There are already bugs which address issues with scripts using random interfaces, and increasing attack surface (bug 605271 and references in its description, bug 634986, bug 634983). Some of them are [sg:critical?] and [sg:critical] and can be used to execute arbitrary code. I would appreciate, if you correct me in case something is wrong with this analysis or if you add something to it. Thanks :) Regards, Rh0

I wrongly was assuming the problem with QueryInterface exists only in the 3.6 branch so i didn't test higher versions. But I realized that higher versions are also affected: WinXP Fx 10.0a1 nightly: bp-6c34b32c-eede-4b36-9dbd-cccef2111004 WinXP Fx 7.0.1: bp-7380d112-da03-47f1-84a1-72f422111004 Linux (x86_64) Fx 10.0a1 bp-ff55de89-7a71-4079-9599-9ff5f2111004 backported Iceweasel 7.0.1 in Debian squeeze crashes aswell (no crashreporter).

Would we be able to QI to a fake request and do Bad Stuff, or would wrappers save us? What other functions can we call to do unexpected stuff? The attached testcase is sg:dos but we'll wait to investigate other possibilities. Jst thinks we need something like the fix in bug 604262 while we're waiting for bug 605271 to save us.

Well, at least on trunk, the only way we can get past the first conditional is if aRequest == mChannel, which can only happen if mChannel is null, so it's DOS only there. I didn't look at the other branches. Note that it's totally possible that there are security issues associated with other methods on this interface, or with other interfaces nsObjectLoadingContent implements. This needs an audit ... I really hate XPConnect sometimes.

The non-classinfo-interfaces entry points from script on nsObjectLoadingContent are: http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l501 sg:dos. See comment 3. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l776 Unknown. If we pass in a non-null request (e.g. a fake object) we'll bounce out of the function with NS_BINDING_ABORTED. If we pass in a null request and mChannel is already null (and mFinalListener is non-null) we'll pass an arbitrary attacker-controlled nsISupports to mFinalListener::OnStopRequest. This needs further investigation by somebody who knows this code for: 1) Can mFinalListener ever be non-null if mChannel is null. 2) Can all listeners that might be mFinalListener handle a null aRequest without crashing? 3) Can all listeners that might be mFinalListener not do bad things with an attacker-controlled aContext that does crazy stuff? http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l798 Probably nothing: Can't fake an nsIInputStream through XPConnect (it has noscript methods) so unless there are DOM objects implementing nsIInputStream that hostile script can pass in here this method can't be called. If the script can get its hands on an nsIInputStream, then this has the same problems as the previous method. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l814 Nothing: This is a getter returning an object that should not have class info and should fail. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l829 Nothing: Method is not implemented. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l836 Probably nothing: String getter, shouldn't hurt anything. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l843 Probably nothing: Integer getter, shouldn't hurt anything. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l1011 Probably nothing: Method can be called, but shouldn't hurt anything. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l1019 Nothing: method is harmless. http://hg.mozilla.org/mozilla-central/annotate/0f872fecb5a8/content/base/src/nsObjectLoadingContent.cpp#l1033 Nothing: We'll error out on the first conditional every time. Hope that was useful. Somebody who knows this code better should investigate OnDataAvailable/OnStopRequest.

Thanks for the investigation Kyle! I'll take this bug and dig into the remaining methods.

This bug here I found with a custom dumb fuzzer. I could also reproduce bug 634986 and bug 604262. It was kind of messy and worked only with Fx 3.6 so I rewrote it. If it helps, I can provide it here. It enumerates interfaces on some html elements as well as the interfaces functions and tries to call them.

I believe this covers what's important here. It's not even clear that this is all necessary, but better to be safe than sorry until we have a system in place that lets us do this right.

Comment on attachment 570196 [details] [diff] [review] Fix. Review of attachment 570196 [details] [diff] [review]: ----------------------------------------------------------------- I worry that we'll see fallout like Bug 631421 from this, but I'm not sure that there's a better way to find that fallout other than checking this in :-/ ::: content/base/src/nsObjectLoadingContent.cpp @@ +802,5 @@ > NS_IMETHODIMP > +nsObjectLoadingContent::OnDataAvailable(nsIRequest *aRequest, > + nsISupports *aContext, > + nsIInputStream *aInputStream, > + PRUint32 aOffset, PRUint32 aCount) Ah, much nicer.

Yup, perfectly valid worry :( One thing that makes me feel a little bit better about this than what happened in bug 604262 is that this passed on try right away whereas the fix for bug 604262 took many attempts and many fixes in random places before we were able to land it. But by a little, I really do mean only a little.

I don't think there's any security risk here from what I can tell in the code, so I'm marking this an sg:dos.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fdae8a1acdb https://hg.mozilla.org/mozilla-central/rev/7fdae8a1acdb

http://www.learninnigeria.com/info/wp-content/themes/default/fuck_yeah.html http://ipv1.cba.pl/ http://www.lucraridelicenta.net/fun.html reproduces this on Beta/9 Mac OS X. Note the source in each: <object id="dupa"> <script> RIINDC=document.getElementById("dupa"); RIINDC.QueryInterface(Components.interfaces.nsIRequestObserver); //RIINDC.mchannel=SHELLCODE_ADDR RIINDC.onStartRequest(null,RIINDC.QueryInterface(Components.interfaces.nsISupports)); //RIINDC.onStartRequest(RIINDC.mchannel,DWCJWL.QueryInterface(Components.interfaces.nsISupports)); </script> </body> </html>

We should try to land this in 3.6.26

Removing the checkin-needed since 1.9.2 is no longer supported.