691785 - Crash (NULL pointer dereference) [@ nsObjectLoadingContent::IsSuccessfulRequest(nsIRequest*) ]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Rh0]

Attachment: testcase to crash firefox

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

Comment 1

[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).

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Johnny Stenback (:jst)]

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

Comment 6

[Rh0]

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.

Comment 7

[Johnny Stenback (:jst)]

Attachment: Fix.

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 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 9

[Johnny Stenback (:jst)]

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.

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Johnny Stenback (:jst)]

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

Comment 13

[Bob Clary [:bc] (inactive)]

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>

Comment 14

[Daniel Veditz [:dveditz]]

We should try to land this in 3.6.26

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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