Closed Bug 691785 Opened 9 years ago Closed 8 years ago
Crash (NULL pointer dereference) [@ ns
Object Loading Content::Is Successful Request(ns IRequest*) ]
Attachment #564567 - Attachment mime type: text/plain → text/html
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).
Version: 1.9.2 Branch → Trunk
Attachment #564567 - Attachment description: testcase to crash firefox 3.6.23 → testcase to crash firefox
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.
Whiteboard: investigate OnDataAvailable/OnStopRequest.
Thanks for the investigation Kyle! I'll take this bug and dig into the remaining methods.
Assignee: nobody → jst
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.
Attachment #570196 - Flags: review?(khuey)
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.
Attachment #570196 - Flags: review?(khuey) → review+
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.
Whiteboard: investigate OnDataAvailable/OnStopRequest. → [sg:dos]
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Duplicate of this bug: 703560
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
blocking1.9.2: --- → .26+
Whiteboard: [sg:dos][qa+] → [sg:dos][qa+] [c-n: for 1.9.2
Removing the checkin-needed since 1.9.2 is no longer supported.
Whiteboard: [sg:dos][qa+] [c-n: for 1.9.2 → [sg:dos][qa+]
You need to log in before you can comment on or make changes to this bug.