Last Comment Bug 319296 - memory corruption via QueryInterface() (CVE-2006-0295)
: memory corruption via QueryInterface() (CVE-2006-0295)
Status: VERIFIED FIXED
[sg:critical?] aviary/moz1.7 not vuln...
: verified1.8.0.1, verified1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
: Hixie (not reading bugmail)
Mentors:
: 326269 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-12-06 06:25 PST by georgi - hopefully not receiving bugspam
Modified: 2007-05-02 07:47 PDT (History)
13 users (show)
dveditz: blocking1.7.13-
dveditz: blocking‑aviary1.0.8-
dveditz: blocking1.8.1+
brendan: blocking1.8.0.1+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (413 bytes, text/html)
2005-12-06 06:26 PST, georgi - hopefully not receiving bugspam
no flags Details
exploit enum3 (620 bytes, text/html)
2005-12-06 22:50 PST, georgi - hopefully not receiving bugspam
no flags Details
Don't use unchecked casts (4.14 KB, patch)
2005-12-13 17:51 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
brendan: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Review
Fix for the underlying problem (2.61 KB, patch)
2005-12-13 18:33 PST, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
dbradley: review+
brendan: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Review
testcase - either crashes or reports "fsck, not vulnerable" (453 bytes, text/html)
2006-01-13 00:17 PST, georgi - hopefully not receiving bugspam
no flags Details

Description georgi - hopefully not receiving bugspam 2005-12-06 06:25:08 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051205 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051205 Firefox/1.6a1

QueryInterface on the navigator object triggers memory corruption:
(on both 1.5 and latest cvs trunk on linux)

#7  0x080a9a1f in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#8  <signal handler called>
#9  0xb7e789b4 in nsQueryInterfaceWithError::operator() (this=0xbfffd608, 
    aIID=@0x8ba9ad8, answer=0xbfffd5b4) at nsCOMPtr.cpp:64
#10 0x08929910 in nsCOMPtr<nsIInterfaceRequestor>::assign_from_qi_with_error (
    this=0xbfffd610, qi=@0x6976614e, aIID=@0x80004003) at nsCOMPtr.h:1242
#11 0x0892982a in nsCOMPtr (this=0xbfffd610, qi=@0xbfffd608) at nsCOMPtr.h:653
#12 0xb7e79e83 in nsGetInterface::operator() (this=0xbfffd680, 
    aIID=@0x8c0cb6c, aInstancePtr=0xbfffd640)
    at nsIInterfaceRequestorUtils.cpp:49
#13 0x082bad36 in nsCOMPtr<nsIScriptGlobalObject>::assign_from_helper (
    this=0xbfffd690, helper=@0x6976614e, aIID=@0x80004003) at nsCOMPtr.h:1292
#14 0x082bad0a in nsCOMPtr (this=0xbfffd690, helper=@0xbfffd680)
    at nsCOMPtr.h:694

(gdb) frame 9
#9  0xb7e789b4 in nsQueryInterfaceWithError::operator() (this=0xbfffd608, 
    aIID=@0x8ba9ad8, answer=0xbfffd5b4) at nsCOMPtr.cpp:64
64                                      status = mRawPtr->QueryInterface(aIID, answer);
(gdb) p mRawPtr
$1 = (class nsISupports *) 0x6976614e
(gdb) p *mRawPtr
Cannot access memory at address 0x6976614e

mRawPtr looks like ascii.
exploitability is not clear at the moment, but with favorable heap layout
it may be possible ala the Window() winblows bug.

testcase to follow.


Reproducible: Always
Comment 1 georgi - hopefully not receiving bugspam 2005-12-06 06:26:34 PST
Created attachment 205141 [details]
testcase

testcase
Comment 2 georgi - hopefully not receiving bugspam 2005-12-06 07:28:07 PST
changing "navigator" with "location" jumps somewhere on linux:
(gdb) info stack
#0  0x61636f4c in ?? ()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#1  0xb7ef493e in nsQueryInterfaceWithError::operator() ()
   from /opt/firefox15/libxpcom_core.so
#2  0xb7ef4a06 in nsCOMPtr_base::assign_from_qi_with_error ()
   from /opt/firefox15/libxpcom_core.so
Comment 3 georgi - hopefully not receiving bugspam 2005-12-06 22:49:25 PST
this seems exploitable at least on linux.
it is possible to allocate memory, so the shellcode gets placed on the magic address.

enum3.html:
Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread -1220606272 (LWP 15234)]
0x61636f4d in ?? ()
(gdb) x/4x $eip
0x61636f4d:     0xcc00cc00      0xcc00cc00      0xcc00cc00      0xcc00cc00
Comment 4 georgi - hopefully not receiving bugspam 2005-12-06 22:50:28 PST
Created attachment 205201 [details]
exploit enum3

exploit
Comment 5 georgi - hopefully not receiving bugspam 2005-12-13 13:13:20 PST
no action, so adding management without offensing words.
management knows better.
good management.
Comment 6 Brendan Eich [:brendan] 2005-12-13 16:11:39 PST
Confirming (doesn't georgi have canconfirm privs?  I'll check).

This bug needs [sg:foo] and assignment.  mrbkap is likely to jump on it, as with any grenade tossed near him and his comrades.  He's a hero, but we need to get these bugs spread around better.  We should not leave them stewing as UNCO.

/be
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-12-13 17:51:16 PST
Created attachment 205782 [details] [diff] [review]
Don't use unchecked casts

jst helped me look at this. There are a couple of problems:

* The way we use nsIClassInfo as a singleton and the scriptable helper is coming back to bite us in the butt here. We end up actually violating xpconnect rules wrt which object is class info (which causes a couple of assertions about what the various scriptable helpers claim to do).

* We were using direct casts in various PreCreate hooks, which were incorrect because of the above problem (we were getting the scriptable helper as the nativeObj). Other hooks _can_ directly cast because of a lucky feature of the way we create wrappers: namely, we refuse to create them without class info. So we'll run the PreCreate hook with the wrong nativeObj, but then refuse to create the wrapper.

Johnny's looking into more "correct" fixes for this.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-13 18:33:00 PST
Created attachment 205788 [details] [diff] [review]
Fix for the underlying problem

This I believe is the fix for the underlying problem here. The problem is really that we end up thinking that a classinfo object itself has a scriptable helper that's intended to be used for the classinfo object when the scriptable helper is really only intended to be used for the class to which the classinfo belongs. This change makes us never QI the native we're wrapping to nsIXPCScriptable of we're in the process of wrapping an nsIClassInfo object.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-13 18:35:07 PST
and I meant to say too that that's what's actually fixing the crash with this patch, we simply never end up in the DOM's classinfo (or scriptable helper) methods with a wrapper/native that's a classinfo object itself, we only ever get into that code with a wrapper/native that's what the classinfo/scriptable hepler was written for.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-13 18:37:29 PST
Comment on attachment 205782 [details] [diff] [review]
Don't use unchecked casts

This fixes the crash, but not the underlying problem. We might want this for the branch as it's a safer change, but it also potentially leaves us with crashable code, at least when called from chrome code.
Comment 11 georgi - hopefully not receiving bugspam 2005-12-14 00:41:45 PST
(In reply to comment #6)
> Confirming (doesn't georgi have canconfirm privs?  I'll check).
> 

brendan: i find it bad practice me confirming my own bugs when there is easy to use testcase.

it's like me confirming i am nice :)

Comment 12 georgi - hopefully not receiving bugspam 2005-12-14 03:02:36 PST
i am not familiar with js/objects internals, but can someone give a (formal) way to look for similar bad casts?

what to look for?
Comment 13 georgi - hopefully not receiving bugspam 2005-12-14 08:54:22 PST
regarding blocking1.8.0.1 - i am ready to bet this is exploitable with enough reliability.
Comment 14 Brendan Eich [:brendan] 2005-12-14 09:31:00 PST
The canconfirm bugzilla capability exists entirely so that people who routinely file valid bugs with testcases don't create routine work rubber-stamping the bug as confirmed.  It's a way of grading newly filed bugs so the ones most likely to be valid appear with NEW status right away.  It has helped reduce costs to all parties in Mozilla since we instituted it way back when.

/be
Comment 15 Brendan Eich [:brendan] 2005-12-14 09:56:06 PST
The fix involves DOM and XPConnect code, and casts or coercions in C++, so it's not a question of "js/object" internals in the sense of SpiderMonkey internals.  So I'll duck and let jst answer georgi's question in comment 12.  Reviewing now.

/be
Comment 16 chris hofmann 2005-12-14 11:38:34 PST
I think I've fixed georgi's [can confirm] problem.
Comment 17 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-12-14 16:41:11 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

Add a comment that we need to do this because the nsIClassInfo object _is_ the scriptable helper (which causes unhappiness and suffering) and r=mrbkap.
Comment 18 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-12-14 16:59:41 PST
(In reply to comment #12)
> i am not familiar with js/objects internals, but can someone give a (formal)
> way to look for similar bad casts?

Unfortunately, I don't think that there is a way to look for similar bad casts. This isn't a bad pattern that we might repeat elsewhere (and other similar casts in the same file are safe for reasons that are entirely unrelated to the file itself, see my comment about why only PreCreate hooks need to be protected without jst's patch).

Even catching these bad casts at runtime would be hard (and perhaps be a perf hit) because the only safe way to do that is to use do_QueryInterface (perhaps turning RTTI on and using dynamic_cast<> might help, but is bloaty and may not be supported by all of the compilers we care about.

I suppose my point is that while some bad patterns do exist (such as the integer overflow problems that you've pointed out), there are other logic bugs that don't necessarily have a pattern (though developing ways to more quickly identify and fix them wouldn't necessarily be a bad thing).
Comment 19 Daniel Veditz [:dveditz] 2005-12-14 17:37:55 PST
I thought I confirmed this during the Firefox onsite.

Not reproducable using the testcases on 1.0.x but the code being patched in xpcwrappednative ("underlying problem") is the same in 1.0 and 1.5. I don't see the unsafe casts in the 1.0 nsDOMClass though. The added null check in nsContentListSH::PreCreate might prevent a null-deref crash, but isn't itself a 1.0 security problem.
Comment 20 Brendan Eich [:brendan] 2005-12-14 18:20:14 PST
Comment on attachment 205782 [details] [diff] [review]
Don't use unchecked casts

sr=me with comments about how the "Oops" cases arise.

/be
Comment 21 Brendan Eich [:brendan] 2005-12-14 18:23:57 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

sr=me.

/be
Comment 22 georgi - hopefully not receiving bugspam 2005-12-14 23:39:25 PST
thanks for the information.

i am thinking of (ab)using casts in xpconnect/xpcom if possible.

can someone please give pointers to the code that casts when invoking xpcom from javascript? i am interesting in the way that the correct type/class is determined.

looking at idl files, sometimes "primitive" c/c++ types are used as here:
nsIPluginInstanceOwner.idl:102:  ShowStatus(const PRUnichar *aStatusMsg) = 0;

Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-15 09:25:22 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

I decided to land this on the trung, but dbradley, I'd still very much appreciate if you could have a look at this change.
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-15 09:27:07 PST
Well, I tried to land this, but CVS is acting up here... I'll try again later today.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2005-12-15 16:40:06 PST
Fix checked in on the trunk now. Leaving bug open to track further reviews and possible branch changes.
Comment 26 georgi - hopefully not receiving bugspam 2005-12-16 04:11:36 PST
latest trunk stops the crash for me.
Comment 27 David Bradley 2005-12-16 06:28:21 PST
Just looking at the latest patch. So sciProto and sciWrapper locals were never assigned in the original code? Something's not making sense to me from just looking at the patch. I'll take a look at the entire function this evening and see if I see any other problems.
Comment 28 David Bradley 2005-12-17 09:15:24 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

r=dbradley

Sorry, I was seeing a + in the diff where there wasn't one. Looks fine to me.

So in essence we're guarding against asking a class info implementation for its class info.
Comment 29 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-12-19 13:07:51 PST
I checked in the nsDOMClassInfo.cpp back-up fixes. Marking this bug as FIXED.
Comment 30 Mike Schroepfer 2005-12-19 14:18:28 PST
Please let bake in the trunk for a few more days and then we'll approve.

Comment 31 Daniel Veditz [:dveditz] 2006-01-04 11:17:54 PST
Comment on attachment 205782 [details] [diff] [review]
Don't use unchecked casts

a=dveditz for the branch. Does the other attachment need approval as well?
Comment 32 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-04 11:31:42 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

Requesting approval for this patch too. I think that this patch only affects the case when we try to QI something to nsIClassInfo, and prevents us from using the wrong classinfo data on the actual classinfo object.
Comment 33 Daniel Veditz [:dveditz] 2006-01-05 11:11:09 PST
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

a=dveditz
Comment 34 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-06 15:51:36 PST
Both fixes checked into the branches.
Comment 35 Marcia Knous [:marcia - use ni] 2006-01-12 17:08:12 PST
QA could use some help in steps to verify this fix. Thanks.
Comment 36 georgi - hopefully not receiving bugspam 2006-01-12 23:20:15 PST
(In reply to comment #35)
> QA could use some help in steps to verify this fix. Thanks.
> 

just open the attachments "testcase" and "exploit enum3" with js on.

if you don't crash the bug is fixed.

"enum3" uses a lot of memory in order to get favorable heap layout on linux, so getting "out of memory" or temoporary firefox unresponsiveness is not related to the bug.

not crashing on "testcase" means it is fixed.


Comment 37 Bob Clary [:bc:] 2006-01-13 00:07:44 PST
no crash with either attachment in 1.8.0.1 Linux 20060111, 1.8.0.1 Windows 20060112, 1.8 Linux 20060112, 1.8 Windows 20060112, 1.9a1 Linux 20060112, 1.9a1 Windows 20060112
Comment 38 georgi - hopefully not receiving bugspam 2006-01-13 00:17:40 PST
Created attachment 208358 [details]
testcase - either crashes or reports "fsck, not vulnerable"

testcase - either crashes or reports "fsck, not vulnerable"
Comment 39 georgi - hopefully not receiving bugspam 2006-01-13 00:19:57 PST
(In reply to comment #35)
> QA could use some help in steps to verify this fix. Thanks.
> 

created better testcase that reports vulnerability status.

just open attachment:
testcase - either crashes or reports "fsck, not vulnerable"
Comment 40 Daniel Veditz [:dveditz] 2006-01-22 23:57:39 PST
Do we need this fix on the aviary1.0/moz1.7 branches? The third testcase (attachment 208358 [details]) says 'not vulnerable', but "exploit enum3" (attachment 205201 [details]) hangs the browser.
Comment 41 georgi - hopefully not receiving bugspam 2006-01-23 07:50:41 PST
looks like 1.0.7 is not vulnerable
Comment 42 georgi - hopefully not receiving bugspam 2006-01-27 06:21:05 PST
regarding CVE in the summary:

i consider mitre/CVE corporate/government's puppies and have low opinion of
them.

mitre's  "Steven M. Christey" <coley@linus.mitre.org>
co-authored the "responsible disclosure draft rfc" which was
obviously a corporate plot and was ditched by even the IETF - check the
ietf archives for this corporate fiasco.
Comment 43 Daniel Veditz [:dveditz] 2006-02-01 14:51:42 PST
Old branches don't appear vulnerable, minusing.
Comment 44 timeless 2006-02-07 17:07:42 PST
*** Bug 326269 has been marked as a duplicate of this bug. ***
Comment 45 georgi - hopefully not receiving bugspam 2007-05-02 07:47:51 PDT
since the summary has the romantic non-F word, CVE, just for the record:

http://www2.csoonline.com/blog_view.html?CID=32886

Most exploits will reinfect a host if a security hole, *******************also known as the Common Vulnerability and Exposure (CVE)***************, is not removed.

Note You need to log in before you can comment on or make changes to this bug.