319296 - memory corruption via QueryInterface() (CVE-2006-0295)

Status: VERIFIED FIXED

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

Description

[georgi - hopefully not receiving bugspam]

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]

Attachment: testcase

testcase

Comment 2

[georgi - hopefully not receiving bugspam]

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]

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]

Attachment: exploit enum3

exploit

Comment 5

[georgi - hopefully not receiving bugspam]

no action, so adding management without offensing words.
management knows better.
good management.

Comment 6

[Brendan Eich [:brendan]]

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) (inactive)]

Attachment: 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)]

Attachment: 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)]

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)]

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]

(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]

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]

regarding blocking1.8.0.1 - i am ready to bet this is exploitable with enough reliability.

Comment 14

[Brendan Eich [:brendan]]

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]]

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]

I think I've fixed georgi's [can confirm] problem.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

(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]]

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]]

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]]

Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

sr=me.

/be

Comment 22

[georgi - hopefully not receiving bugspam]

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)]

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)]

Well, I tried to land this, but CVS is acting up here... I'll try again later today.

Comment 25

[Johnny Stenback (:jst)]

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]

latest trunk stops the crash for me.

Comment 27

[David Bradley]

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]

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) (inactive)]

I checked in the nsDOMClassInfo.cpp back-up fixes. Marking this bug as FIXED.

Comment 30

[Mike Schroepfer]

Please let bake in the trunk for a few more days and then we'll approve.

Comment 31

[Daniel Veditz [:dveditz]]

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) (inactive)]

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]]

Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

a=dveditz

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Both fixes checked into the branches.

Comment 35

[Marcia Knous [:marcia]]

QA could use some help in steps to verify this fix. Thanks.

Comment 36

[georgi - hopefully not receiving bugspam]

(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] (inactive)]

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]

Attachment: testcase - either crashes or reports "fsck, not vulnerable"

testcase - either crashes or reports "fsck, not vulnerable"

Comment 39

[georgi - hopefully not receiving bugspam]

(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]]

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]

looks like 1.0.7 is not vulnerable

Comment 42

[georgi - hopefully not receiving bugspam]

regarding CVE in the summary:

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

mitre's  "Steven M. Christey" <[e-mail]>
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]]

Old branches don't appear vulnerable, minusing.

Comment 44

[timeless]

*** Bug 326269 has been marked as a duplicate of this bug. ***

Comment 45

[georgi - hopefully not receiving bugspam]

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.