Closed
Bug 532246
(CVE-2010-1198)
Opened 15 years ago
Closed 15 years ago
Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-0049)
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(blocking1.9.2 needed, status1.9.2 .4-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)
People
(Reporter: bsterne, Assigned: benjamin)
References
Details
(Whiteboard: [sg:critical?][fixed-lorentz])
Attachments
(2 files, 2 obsolete files)
1.80 KB,
patch
|
jst
:
superreview+
beltzner
:
approval1.9.2.4+
bsterne
:
approval1.9.1.10+
|
Details | Diff | Splinter Review |
6.73 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
Microsoft Vulnerability Research reported this issue to security@m.o today. They included a advisory paper as well as a proof-of-concept for an exploit, though I was unable to get it to work (the author does say it works only about 20% of the time). The PoC requires Windows Media Player and JRE plug-ins to be installed. I will upload the attached materials shortly.
The short summary is that plug-in A can get a reference to a NPObject from plug-in B and wait for plug-in B to be unloaded from a page, resulting in plug-in A having a dangling pointer to the freed NPObject.
Assignee | ||
Comment 1•15 years ago
|
||
If this is indeed a bug, it seems like it would be easy enough to fix: instead of unwrapping an NPObject which is passed to a different instance (NPP), we should double-wrap it. This means that the other plugin would obtain a reference to a nsJSObjWrapper, instead of the other-plugin-implemented NPObject* which is destroyed when the plugin is destroyed.
This would involve a fairly simple change to nsJSObjWrapper::GetNewOrUsed so that we only return the wrapped NPObject* if the NPP matches, here:
http://hg.mozilla.org/mozilla-central/annotate/872dcbf2c9c6/modules/plugin/base/src/nsJSNPRuntime.cpp#l1038
jst, does that sound reasonable? And more importantly, does that sound safe enough to take in a dot-release?
Assignee: nobody → benjamin
Assignee | ||
Comment 2•15 years ago
|
||
Attachment #415647 -
Flags: review?(jst)
Assignee | ||
Comment 3•15 years ago
|
||
Attachment #415648 -
Flags: superreview?(jst)
Attachment #415648 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #415648 -
Flags: review? → review?(joshmoz)
Reporter | ||
Comment 4•15 years ago
|
||
If you need the testcase, which is too large for Bugzilla to accept, send me an email and I'll get it to you. I'll also see if I can reduce the testcase so it will fit here.
Attachment #415648 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 5•15 years ago
|
||
Comment on attachment 415648 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 1
I'm a dummy, the cast is wrong, new patch forthcoming.
Attachment #415648 -
Attachment is obsolete: true
Attachment #415648 -
Flags: superreview?(jst)
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #415695 -
Flags: superreview?(jst)
Assignee | ||
Comment 7•15 years ago
|
||
This tests both conditions: that we pass the same NPObject back to a plugin instance, and that we double-wrap for other instances.
Attachment #415647 -
Attachment is obsolete: true
Attachment #415697 -
Flags: review?(jst)
Attachment #415647 -
Flags: review?(jst)
Comment 8•15 years ago
|
||
Comment on attachment 415695 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 2
Yeah, this does the double wrapping that's been talked about, but never tried out before. This needs some baking before we can ship to the masses IMO.
sr=jst
Attachment #415695 -
Flags: superreview?(jst) → superreview+
Updated•15 years ago
|
Attachment #415697 -
Flags: review?(jst) → review+
Assignee | ||
Comment 9•15 years ago
|
||
This sounds like it is probably exploitable with heap-smashing, as long as content has some control over what plugins do with the shared objects, which is likely possible with Flash and Silverlight.
Whiteboard: [sg:critical?]
Updated•15 years ago
|
Summary: Investigate crash caused by Java plug-in using deleted WMP plug-in object → Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-49)
Updated•15 years ago
|
Summary: Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-49) → Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-0049)
Comment 10•15 years ago
|
||
Can we get this landed?
Assignee | ||
Comment 11•15 years ago
|
||
code: http://hg.mozilla.org/mozilla-central/rev/91fbd4442e60
test: http://hg.mozilla.org/mozilla-central/rev/0c10530400aa
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 12•15 years ago
|
||
Arg, we needed to schedule this for upcoming releases.
Assignee | ||
Comment 13•15 years ago
|
||
I'm really nervous about this one, FWIW... the possibility of subtle bugs which only show up in the field is pretty high.
Comment 14•15 years ago
|
||
What if anything could we do to make you less nervous? You'd previously thought this was going to be fairly simple, and drivers are having trouble figuring out whether we think this fix is "essential but risky" or "valuable and slightly risky."
Assignee | ||
Comment 15•15 years ago
|
||
The change is simple in terms of code, but perhaps risky in terms of effects. The main risk is that two instances of the same plugin will now be getting wrapped objects instead of native objects.
Comment 16•15 years ago
|
||
If this patch applies cleanly to the branches please request approval on them. I don't see how we're going to get much more feedback with nightly users than we've already gotten.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
Assignee | ||
Comment 17•15 years ago
|
||
http://hg.mozilla.org/projects/firefox-lorentz/rev/bdc85ebc0587
Since I needed it for Lorentz I think it'll just coast into 1.9.2.3 anyway.
Whiteboard: [sg:critical?] → [sg:critical?][fixed-lorentz]
Assignee | ||
Comment 18•15 years ago
|
||
Comment on attachment 415695 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 2
But setting approval flag in case Lorentz bounces.
Attachment #415695 -
Flags: approval1.9.2.3?
Comment 19•15 years ago
|
||
Comment on attachment 415695 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 2
a=beltzner for 1.9.2.3, please make sure that the test lands as well
Attachment #415695 -
Flags: approval1.9.2.3? → approval1.9.2.3+
Comment 20•15 years ago
|
||
What about 1.9.1?
Comment 21•15 years ago
|
||
Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed
Assignee | ||
Comment 22•15 years ago
|
||
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
Comment 23•15 years ago
|
||
(In reply to comment #11)
> code: http://hg.mozilla.org/mozilla-central/rev/91fbd4442e60
> test: http://hg.mozilla.org/mozilla-central/rev/0c10530400aa
I thought that we didn't check in security tests when they were still unpatched on branch releases?
Comment 24•15 years ago
|
||
Is the mochitest for this going to land on 1.9.2 after we ship 1.9.2.4?
Assignee | ||
Comment 25•15 years ago
|
||
The mochitest already landed on 1.9.2 as part of the lorentz backport.
Comment 26•15 years ago
|
||
Interesting. While the mochitest passes, the original PoC still crashes in current 1.9.2 builds.
With a pre-fix build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)), I get this crash:
http://crash-stats.mozilla.com/report/index/f5c6a832-b91d-4057-b485-95d9b2100408
With a post-fix build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100408 Lorentz/3.6.4pre (.NET CLR 3.5.30729)), I get this crash:
http://crash-stats.mozilla.com/report/index/0e2ca9b2-f88f-4c8c-975e-a793d2100408
Neither of these crash reports give good data. The PoC does a bunch of heap spraying with Java but the crash looks the same. Brandon has the PoC if you want to get it from him.
status1.9.2:
.4-fixed → ---
Assignee | ||
Comment 27•15 years ago
|
||
Since neither of those reports have any useful data, it's hard to say.
Comment 28•15 years ago
|
||
I've submitted a few more and they are all the same.
I'd recommend getting the PoC from BSterne. I can't mark this as verified for 1.9.2 at this point since the original PoC still crashes.
Assignee | ||
Comment 29•15 years ago
|
||
On mozilla-central if I turn IPC off, I get the following crash with the testcase:
mozcrt19.dll!free(void * ptr=0x07e0a800) Line 6017 + 0x16 bytes C
> mozjs.dll!js::TraceRecorder::~TraceRecorder() Line 2334 + 0x1e bytes C++
mozjs.dll!js::TraceRecorder::finishAbort(const char * reason=0x72146414) Line 2417 + 0xd bytes C++
mozjs.dll!js::AbortRecordingImpl(JSContext * cx=) Line 7157 C++
mozjs.dll!js::ResetJITImpl(JSContext * cx=0x00000000) + 0x8f0b6 bytes C++
There are a few crash signatures like that in crash-stats, but I don't think they have anything to do with this bug. It's also a null-deref crash. I think we should mark this .4-fixed again, and file a new bug for the other issues if necessary.
Assignee | ||
Comment 30•15 years ago
|
||
Different crash, in JS engine, still not obviously plugin-related: > mozjs.dll!js::TraceRecorder::checkForGlobalObjectReallocation() Line 3696 + 0x10 bytes C++
mozjs.dll!js::TraceRecorder::snapshot(js::ExitType exitType=TIMEOUT_EXIT) Line 3968 + 0x5 bytes C++
mozjs.dll!js::TraceRecorder::TraceRecorder(JSContext * cx=0x06563400, js::VMSideExit * anchor=0x00000000, js::VMFragment * fragment=0x062032e0, unsigned int stackSlots=0x0000000e, unsigned int ngslots=0x00000000, js::TraceType_ * typeMap=0x062033a0, js::VMSideExit * innermost=0x00000000, unsigned char * outer=0x00000000, unsigned long outerArgc=0x00000000, js::RecordReason recordReason=Record_Branch) Line 2307 + 0x2b bytes C++
mozjs.dll!js::RecordTree(JSContext * cx=0x00000000, js::TreeFragment * peer=0x00000000, unsigned char * outer=0x00000000, unsigned long outerArgc=0x00000000, js::Queue<unsigned short> * globalSlots=0x0070e028, js::RecordReason reason=Record_Branch) Line 5653 + 0x5e bytes C++
mozjs.dll!js::MonitorLoopEdge(JSContext * cx=0x06563400, unsigned int & inlineCallCount=0x00000000, js::RecordReason reason=Record_Branch) Line 6937 + 0x21 bytes C++
mozjs.dll!js_Interpret(JSContext * cx=) Line 911 + 0xc7 bytes C++
mozjs.dll!js_Invoke(JSContext * cx=0x06563400, unsigned int argc=0x00000004, int * vp=0x0654d048, unsigned int flags=0x00000000) Line 843 + 0x6 bytes C++
xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x061a1700, unsigned short methodIndex=0x0003, const XPTMethodDescriptor * info=0x03be8e20, nsXPTCMiniVariant * nativeParams=0x0039c9b4) Line 1697 C++
xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=0x0003, const XPTMethodDescriptor * info=0x03be8e20, nsXPTCMiniVariant * params=0x0039c9b4) Line 571 C++
xul.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x0614da70, unsigned int methodIndex=0x00000003, unsigned int * args=0x0039ca6c, unsigned int * stackBytesToPop=0x0039ca5c) Line 114 + 0x15 bytes C++
xul.dll!SharedStub() Line 142 C++
xul.dll!nsBrowserStatusFilter::OnStateChange(nsIWebProgress * aWebProgress=0x03b67024, nsIRequest * aRequest=0x62ecbeac, unsigned int aStateFlags=0x000f0001, unsigned int aStatus=0x00000000) Line 183 + 0x13 bytes C++
xul.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x03b67024, nsIRequest * aRequest=0x62ecbeac, int aStateFlags=0x000f0001, unsigned int aStatus=0x00000000) Line 1314 + 0x1c bytes C++
xul.dll!nsDocLoader::OnStartRequest(nsIRequest * request=0x62ecbeac, nsISupports * aCtxt=0x00000000) Line 555 + 0x1c bytes C++
xul.dll!nsLoadGroup::AddRequest(nsIRequest * request=0x03b67014, nsISupports * ctxt=0x00000000) Line 596 C++
xul.dll!nsBaseChannel::AsyncOpen(nsIStreamListener * listener=0x07144a00, nsISupports * ctxt=0x00000000) Line 564 C++
xul.dll!nsURILoader::OpenURI(nsIChannel * channel=0x07144a00, int aIsContentPreferred=0x00000000, nsIInterfaceRequestor * aWindowContext=0x03b67028) Line 843 C++
xul.dll!nsDocShell::DoChannelLoad(nsIChannel * aChannel=0x62ecbeac, nsIURILoader * aURILoader=0x0426a9c0, int aBypassClassifier=0x00000000) Line 8641 C++
xul.dll!nsDocShell::DoURILoad(nsIURI * aURI=0x05b79ab0, nsIURI * aReferrerURI=0x00000000, int aSendReferrer=0x00000001, nsISupports * aOwner=0x00000000, const char * aTypeHint=0x6ea80e80, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, int aFirstParty=0x00000001, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x0039cc68, int aIsNewWindowTarget=0x00000000, int aBypassClassifier=0x00000000, int aForceAllowCookies=0x00000000) Line 8492 C++
xul.dll!nsDocShell::InternalLoad(nsIURI * aURI=0x05b79ab0, nsIURI * aReferrer=0x00000000, nsISupports * aOwner=0x00000000, unsigned int aFlags=0x00000000, const wchar_t * aWindowTarget=0x00000000, const char * aTypeHint=0x6ea80e80, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, unsigned int aLoadType=0x00000002, nsISHEntry * aSHEntry=0x05b79a00, int aFirstParty=0x00000001, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x00000000) Line 8176 + 0x6e bytes C++
xul.dll!nsDocShell::LoadHistoryEntry(nsISHEntry * aEntry=0x00000000, unsigned int aLoadType=0x00000000) Line 9654 C++
xul.dll!nsCOMPtr_base::assign_from_qi(nsQueryInterface qi={...}, const nsID & iid={...}) Line 99 C++
xul.dll!XPCCallContext::~XPCCallContext() Line 428 + 0xa bytes C++
xul.dll!XPC_WN_OnlyIWrite_Proto_PropertyStub(JSContext * cx=0x03b67010, JSObject * obj=0x6e0aec2b, int idval=0x6e910910, int * vp=0x0039cf70) Line 2026 + 0x9 bytes C++
xul.dll!nsCOMPtr_base::~nsCOMPtr_base() Line 82 C++
xul.dll!nsQueryReferent::operator()(const nsID & aIID={...}, void * * answer=0x0039cf48) Line 88 + 0x2e bytes C++
xul.dll!nsCOMPtr_base::assign_from_helper(const nsCOMPtr_helper & helper={...}, const nsID & iid={...}) Line 150 + 0x11 bytes C++
xul.dll!nsCOMPtr<nsPIDOMWindow>::nsCOMPtr<nsPIDOMWindow>(const nsCOMPtr_helper & helper={...}) Line 623 C++
xul.dll!nsLocation::Reload(int aForceget=0x00000000) Line 813 C++
xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x05bb6ea0, unsigned int methodIndex=0x00000013, unsigned int paramCount=0x00000001, nsXPTCVariant * params=0x0039d038) Line 103 C++
xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD) Line 2750 + 0x1e bytes C++
xul.dll!XPC_WN_CallMethod(JSContext * cx=0x06563400, JSObject * obj=0x0330e680, unsigned int argc=0x00000000, int * argv=0x0654d034, int * vp=0x0039d370) Line 1770 + 0x12 bytes C++
mozjs.dll!js_Invoke(JSContext * cx=, unsigned int argc=, int * vp=, unsigned int flags=) Line 835 + 0x15 bytes C++
mozjs.dll!js_Interpret(JSContext * cx=0x06563400) Line 2271 C++
mozjs.dll!js_Invoke(JSContext * cx=0x06563400, unsigned int argc=0x00000001, int * vp=0x0654d020, unsigned int flags=0x00000000) Line 843 + 0x6 bytes C++
mozjs.dll!js_InternalInvoke(JSContext * cx=0x06563400, JSObject * obj=0x03301620, int fval=0x04173020, unsigned int flags=0x00000000, unsigned int argc=0x00000001, int * argv=0x0072edac, int * rval=0x0039d6c8) Line 900 + 0x12 bytes C++
mozjs.dll!JS_CallFunctionValue(JSContext * cx=0x06563400, JSObject * obj=0x03301620, int fval=0x04173020, unsigned int argc=0x00000001, int * argv=0x0072edac, int * rval=0x0039d6c8) Line 4957 C++
xul.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x00753ec0, void * aScope=0x03301620, void * aHandler=0x04173020, nsIArray * aargv=0x5e5ff964, nsIVariant * * arv=0x0039d77c) Line 2164 C++
xul.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x74ac6e40) Line 8407 C++
xul.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x07111880, void * aClosure=0x74ac6e40) Line 8752 C++
xul.dll!nsTimerImpl::Fire() Line 427 + 0x7 bytes C++
nspr4.dll!_PR_MD_UNLOCK(_MDLock * lock=0x007b8140) Line 347 C
xul.dll!nsThread::ProcessNextEvent(int mayWait=, int * result=) Line 528 C++
Comment 31•15 years ago
|
||
Huh, ok. If it is a different crash, then I does make sense to mark it as fixed for 1.9.2. I'm not sure what to do about opening a new case since the testcase is a PoC for a security bug. Brandon?
Reporter | ||
Comment 32•15 years ago
|
||
Yeah, clearly a different crash. bsmedberg, would you be willing to file the new JS bug and mark it security-sensitive initially? You can make a comment that I (or you) have the testcase, since it will be too big to attach (that is, unless you managed to reduce it).
We can mark this bug VERIFIED but we need to leave it closed until the to-be-filed bug is fixed or determined to not be exploitable.
Assignee | ||
Updated•15 years ago
|
status1.9.2:
--- → .4-fixed
Comment 33•15 years ago
|
||
(In reply to comment #20)
> What about 1.9.1?
re-poke.
Reporter | ||
Comment 34•15 years ago
|
||
Okay, I'll ask a third time: what about 1.9.1?
The patch applies cleanly to 1.9.1. bsmedberg, can we take this patch there as well, or do we need to make a new patch?
Assignee | ||
Comment 35•15 years ago
|
||
I really don't know how much the code has changed between 1.9.1 and 1.9.2, or whether there might be other dependent bugs. I'm sure that most of the testsuite was developed after 1.9.1. You'll have to ask jst/josh.
Comment 36•15 years ago
|
||
Taking this on 1.9.1 looks fine to me but jst should sign off on it.
Comment 37•15 years ago
|
||
This is fixed on 1.9.2, so we should definitely try to get it on 1.9.1 at the same time to reduce the window of exposure.
blocking1.9.1: needed → .10+
Comment 38•15 years ago
|
||
I say we should take this patch on 1.9.1 as well.
Assignee | ||
Updated•15 years ago
|
Attachment #415695 -
Flags: approval1.9.1.10?
Reporter | ||
Updated•15 years ago
|
Attachment #415695 -
Flags: approval1.9.1.10? → approval1.9.1.10+
Reporter | ||
Comment 39•15 years ago
|
||
Comment on attachment 415695 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 2
a=bsterne for 1.9.2.10.
Reporter | ||
Comment 40•15 years ago
|
||
That should have been approval for 1.9.1.10, of course.
Comment 41•15 years ago
|
||
Updated•15 years ago
|
Alias: CVE-2010-1198
Comment 42•15 years ago
|
||
Is the mochitest for this going to land on 1.9.1 after we ship?
Assignee | ||
Comment 43•15 years ago
|
||
No, the 1.9.1 branch doesn't have any of the test infrastructure (the testplugin) which would make it possible to add that test.
Updated•15 years ago
|
Group: core-security
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•