532246 - (CVE-2010-1198) Investigate crash caused by Java plug-in using deleted WMP plug-in object (MSVR-09-0049)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Brandon Sterne (:bsterne)]

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

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]

Attachment: Test

Comment 3

[Benjamin Smedberg]

Attachment: Double-wrap NPObject* when they belong to a different instance, rev. 1

Comment 4

[Brandon Sterne (:bsterne)]

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]

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]

Attachment: Double-wrap NPObject* when they belong to a different instance, rev. 2

Comment 7

[Benjamin Smedberg]

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

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]

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

Can we get this landed?

Comment 11

[Benjamin Smedberg]

code: http://hg.mozilla.org/mozilla-central/rev/91fbd4442e60
test: http://hg.mozilla.org/mozilla-central/rev/0c10530400aa

Comment 12

[Daniel Veditz [:dveditz]]

Arg, we needed to schedule this for upcoming releases.

Comment 13

[Benjamin Smedberg]

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

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]

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

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]

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]

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

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

What about 1.9.1?

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

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]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 23

[Al Billings [:abillings - ex-MoCo]]

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

Is the mochitest for this going to land on 1.9.2 after we ship 1.9.2.4?

Comment 25

[Benjamin Smedberg]

The mochitest already landed on 1.9.2 as part of the lorentz backport.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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]

Since neither of those reports have any useful data, it's hard to say.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

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]

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]

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

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

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

(In reply to comment #20)
> What about 1.9.1?

re-poke.

Comment 34

[Brandon Sterne (:bsterne)]

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]

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]

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

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

I say we should take this patch on 1.9.1 as well.

Comment 39

[Brandon Sterne (:bsterne)]

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

That should have been approval for 1.9.1.10, of course.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/eab5aba75009

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Is the mochitest for this going to land on 1.9.1 after we ship?

Comment 43

[Benjamin Smedberg]

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.