Last Comment Bug 532246 - (CVE-2010-1198) Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-0049)
(CVE-2010-1198)
: Investigate crash caused by Java plug-in using deleted WMP plug-in object (MS...
Status: RESOLVED FIXED
[sg:critical?][fixed-lorentz]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on: 558414
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-01 17:08 PST by Brandon Sterne (:bsterne)
Modified: 2010-07-15 18:31 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.4-fixed
.10+
.10-fixed


Attachments
Test (6.35 KB, patch)
2009-12-02 09:06 PST, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review
Double-wrap NPObject* when they belong to a different instance, rev. 1 (1.08 KB, patch)
2009-12-02 09:07 PST, Benjamin Smedberg [:bsmedberg]
jaas: review+
Details | Diff | Review
Double-wrap NPObject* when they belong to a different instance, rev. 2 (1.80 KB, patch)
2009-12-02 12:56 PST, Benjamin Smedberg [:bsmedberg]
jst: superreview+
mbeltzner: approval1.9.2.4+
brandon: approval1.9.1.10+
Details | Diff | Review
Test, rev. 2 (6.73 KB, patch)
2009-12-02 12:57 PST, Benjamin Smedberg [:bsmedberg]
jst: review+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2009-12-01 17:08:54 PST
Created attachment 415525 [details]
MSVR advisory

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.
Comment 1 Benjamin Smedberg [:bsmedberg] 2009-12-02 06:56:44 PST
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?
Comment 2 Benjamin Smedberg [:bsmedberg] 2009-12-02 09:06:22 PST
Created attachment 415647 [details] [diff] [review]
Test
Comment 3 Benjamin Smedberg [:bsmedberg] 2009-12-02 09:07:40 PST
Created attachment 415648 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 1
Comment 4 Brandon Sterne (:bsterne) 2009-12-02 10:21:05 PST
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.
Comment 5 Benjamin Smedberg [:bsmedberg] 2009-12-02 12:46:29 PST
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.
Comment 6 Benjamin Smedberg [:bsmedberg] 2009-12-02 12:56:29 PST
Created attachment 415695 [details] [diff] [review]
Double-wrap NPObject* when they belong to a different instance, rev. 2
Comment 7 Benjamin Smedberg [:bsmedberg] 2009-12-02 12:57:28 PST
Created attachment 415697 [details] [diff] [review]
Test, rev. 2

This tests both conditions: that we pass the same NPObject back to a plugin instance, and that we double-wrap for other instances.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-02 17:10:46 PST
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
Comment 9 Benjamin Smedberg [:bsmedberg] 2009-12-04 07:17:39 PST
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.
Comment 10 Reed Loden [:reed] (use needinfo?) 2009-12-16 16:28:08 PST
Can we get this landed?
Comment 12 Daniel Veditz [:dveditz] 2010-03-02 08:00:50 PST
Arg, we needed to schedule this for upcoming releases.
Comment 13 Benjamin Smedberg [:bsmedberg] 2010-03-02 09:59:10 PST
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 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-03 12:41:49 PST
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."
Comment 15 Benjamin Smedberg [:bsmedberg] 2010-03-03 12:56:20 PST
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 Daniel Veditz [:dveditz] 2010-03-05 13:08:19 PST
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.
Comment 17 Benjamin Smedberg [:bsmedberg] 2010-03-24 08:24:10 PDT
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.
Comment 18 Benjamin Smedberg [:bsmedberg] 2010-03-24 08:24:30 PDT
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.
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-24 12:55:35 PDT
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
Comment 20 Daniel Veditz [:dveditz] 2010-03-24 12:56:00 PDT
What about 1.9.1?
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-06 16:10:59 PDT
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
Comment 22 Benjamin Smedberg [:bsmedberg] 2010-04-07 06:11:49 PDT
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
Comment 23 Al Billings [:abillings] 2010-04-07 15:06:56 PDT
(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 Al Billings [:abillings] 2010-04-07 15:07:55 PDT
Is the mochitest for this going to land on 1.9.2 after we ship 1.9.2.4?
Comment 25 Benjamin Smedberg [:bsmedberg] 2010-04-08 06:35:30 PDT
The mochitest already landed on 1.9.2 as part of the lorentz backport.
Comment 26 Al Billings [:abillings] 2010-04-08 11:38:39 PDT
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.
Comment 27 Benjamin Smedberg [:bsmedberg] 2010-04-08 14:39:09 PDT
Since neither of those reports have any useful data, it's hard to say.
Comment 28 Al Billings [:abillings] 2010-04-08 14:45:29 PDT
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.
Comment 29 Benjamin Smedberg [:bsmedberg] 2010-04-09 10:47:03 PDT
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.
Comment 30 Benjamin Smedberg [:bsmedberg] 2010-04-09 10:49:31 PDT
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 Al Billings [:abillings] 2010-04-09 11:10:29 PDT
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?
Comment 32 Brandon Sterne (:bsterne) 2010-04-09 11:18:46 PDT
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.
Comment 33 Reed Loden [:reed] (use needinfo?) 2010-04-12 12:21:20 PDT
(In reply to comment #20)
> What about 1.9.1?

re-poke.
Comment 34 Brandon Sterne (:bsterne) 2010-04-19 15:42:48 PDT
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?
Comment 35 Benjamin Smedberg [:bsmedberg] 2010-04-20 10:14:45 PDT
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 Josh Aas 2010-04-20 10:18:50 PDT
Taking this on 1.9.1 looks fine to me but jst should sign off on it.
Comment 37 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-26 22:30:33 PDT
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.
Comment 38 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-27 10:55:32 PDT
I say we should take this patch on 1.9.1 as well.
Comment 39 Brandon Sterne (:bsterne) 2010-04-27 11:23:28 PDT
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.
Comment 40 Brandon Sterne (:bsterne) 2010-04-27 11:24:06 PDT
That should have been approval for 1.9.1.10, of course.
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-04-27 11:52:15 PDT
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/eab5aba75009
Comment 42 Al Billings [:abillings] 2010-05-07 11:33:11 PDT
Is the mochitest for this going to land on 1.9.1 after we ship?
Comment 43 Benjamin Smedberg [:bsmedberg] 2010-05-07 13:58:34 PDT
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.

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