413 bytes, text/html
620 bytes, text/html
4.14 KB, patch
|Details | Diff | Splinter Review|
2.61 KB, patch
|Details | Diff | Splinter Review|
453 bytes, text/html
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
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
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: nobody → general
Component: General → DOM
Priority: -- → P1
Product: Firefox → Core
QA Contact: general → ian
Target Milestone: --- → mozilla1.9alpha
Version: unspecified → Trunk
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)
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.
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 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+
(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?
regarding blocking18.104.22.168 - 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: blocking22.214.171.124? → blocking126.96.36.199+
I think I've fixed georgi's [can confirm] problem.
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+
(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
Comment on attachment 205788 [details] [diff] [review] Fix for the underlying problem sr=me. /be
Attachment #205788 - Flags: superreview?(brendan) → superreview+
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.
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 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+
I checked in the nsDOMClassInfo.cpp back-up fixes. Marking this bug as FIXED.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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?
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.
Whiteboard: [sg:critical?] → [sg:critical?] needs branch landing
Comment on attachment 205788 [details] [diff] [review] Fix for the underlying problem a=dveditz
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.
no crash with either attachment in 188.8.131.52 Linux 20060111, 184.108.40.206 Windows 20060112, 1.8 Linux 20060112, 1.8 Windows 20060112, 1.9a1 Linux 20060112, 1.9a1 Windows 20060112
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"
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.
looks like 1.0.7 is not vulnerable
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" <firstname.lastname@example.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.
Whiteboard: [sg:critical?] needs branch landing → [sg:critical?]
Old branches don't appear vulnerable, minusing.
*** 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)
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.