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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P1
normal
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: mrbkap)

Tracking

({verified1.8.0.1, verified1.8.1})

Trunk
mozilla1.9alpha1
x86
Linux
verified1.8.0.1, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.13 -
blocking-aviary1.0.8 -
blocking1.8.1 +
blocking1.8.0.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] aviary/moz1.7 not vulnerable)

Attachments

(5 attachments)

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
Created attachment 205141 [details]
testcase

testcase
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
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
Created attachment 205201 [details]
exploit enum3

exploit
no action, so adding management without offensing words.
management knows better.
good management.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

12 years ago
Assignee: nobody → general
Component: General → DOM
Priority: -- → P1
Product: Firefox → Core
QA Contact: general → ian
Target Milestone: --- → mozilla1.9alpha
Version: unspecified → Trunk
(Assignee)

Comment 7

12 years ago
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #205782 - Flags: review?(jst)
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.
Attachment #205788 - Flags: superreview?(brendan)
Attachment #205788 - Flags: review?(mrbkap)
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.

Updated

12 years ago
Attachment #205788 - Flags: review?(dbradley)
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.
Attachment #205782 - Flags: review?(jst) → review+
(Assignee)

Updated

12 years ago
Attachment #205782 - Flags: superreview?(brendan)
(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 :)

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?

Updated

12 years ago
Whiteboard: sg:fix

Updated

12 years ago
Flags: blocking1.8.0.1?
regarding blocking1.8.0.1 - i am ready to bet this is exploitable with enough reliability.
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
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
Flags: blocking1.8.0.1? → blocking1.8.0.1+

Comment 16

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

Updated

12 years ago
Whiteboard: sg:fix → sg:critial?

Updated

12 years ago
Whiteboard: sg:critial? → [sg:critical?]
(Assignee)

Comment 17

12 years ago
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.
Attachment #205788 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 18

12 years ago
(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).
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 on attachment 205782 [details] [diff] [review]
Don't use unchecked casts

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

/be
Attachment #205782 - Flags: superreview?(brendan)
Attachment #205782 - Flags: superreview+
Attachment #205782 - Flags: approval1.8.0.1?
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

sr=me.

/be
Attachment #205788 - Flags: superreview?(brendan) → superreview+
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 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.
Well, I tried to land this, but CVS is acting up here... I'll try again later today.
Fix checked in on the trunk now. Leaving bug open to track further reviews and possible branch changes.
latest trunk stops the crash for me.

Comment 27

12 years ago
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

12 years ago
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.
Attachment #205788 - Flags: review?(dbradley) → review+
(Assignee)

Comment 29

12 years ago
I checked in the nsDOMClassInfo.cpp back-up fixes. Marking this bug as FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 30

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

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?
Attachment #205782 - Flags: approval1.8.1+
Attachment #205782 - Flags: approval1.8.0.1?
Attachment #205782 - Flags: approval1.8.0.1+
(Assignee)

Comment 32

12 years ago
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.
Attachment #205788 - Flags: approval1.8.1?
Attachment #205788 - Flags: approval1.8.0.1?
Flags: blocking1.8.1+
Whiteboard: [sg:critical?] → [sg:critical?] needs branch landing
Comment on attachment 205788 [details] [diff] [review]
Fix for the underlying problem

a=dveditz
Attachment #205788 - Flags: approval1.8.1?
Attachment #205788 - Flags: approval1.8.1+
Attachment #205788 - Flags: approval1.8.0.1?
Attachment #205788 - Flags: approval1.8.0.1+
(Assignee)

Comment 34

12 years ago
Both fixes checked into the branches.
Keywords: fixed1.8.0.1, fixed1.8.1
QA could use some help in steps to verify this fix. Thanks.
(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

12 years ago
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
Status: RESOLVED → VERIFIED
Flags: testcase?
Keywords: fixed1.8.0.1, fixed1.8.1 → verified1.8.0.1, verified1.8.1
Created attachment 208358 [details]
testcase - either crashes or reports "fsck, not vulnerable"

testcase - either crashes or reports "fsck, not vulnerable"
(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"

Updated

11 years ago
Flags: testcase? → testcase+
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.
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
looks like 1.0.7 is not vulnerable

Updated

11 years ago
Summary: memory corruption via QueryInterface() → CVE-2006-0295 memory corruption via QueryInterface()
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.
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical?] needs branch landing → [sg:critical?]
Old branches don't appear vulnerable, minusing.
Flags: blocking1.7.13?
Flags: blocking1.7.13-
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8-
Group: security

Comment 44

11 years ago
*** Bug 326269 has been marked as a duplicate of this bug. ***
Whiteboard: [sg:critical?] → [sg:critical?] aviary/moz1.7 not vulnerable
Summary: CVE-2006-0295 memory corruption via QueryInterface() → memory corruption via QueryInterface() (CVE-2006-0295)

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
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.
You need to log in before you can comment on or make changes to this bug.