1234458 - Cache API crash by NULL pointer write in CacheChild::ExecuteOp()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Looben Yang]

Reproduce test case (just service worker code, full repro is in attachment NullPtr_ExecuteOp_repro.js):

caches.open("cacheName").then(function(cache) {cache.match("AA");});


Steps to reproduce:
1. Run server side script NullPtr_ExecuteOp_repro.js in Node.js (node NullPtr_ExecuteOp_repro.js).
2. Enter http://localhost:12345 in Firefox browser.
3. Firefox crashes in CacheChild::ExecuteOp():

Firefox version: 46.0a1 (2015-12-21)

First-chance exception at 0x107DA98D (xul.dll) in firefox.exe: 0xC0000005: Access violation writing location 0x00000060.
Unhandled exception at 0x107DA98D (xul.dll) in firefox.exe: 0xC0000005: Access violation writing location 0x00000060.

Variables:

+		this	0x00000000 <NULL>	mozilla::dom::cache::CacheChild *
+		aGlobal	0x07d24fb8 {mScope={...} mClients={mRawPtr=0x00000000 <NULL> } mRegistration={mRawPtr=0x00000000 <NULL> } }	nsIGlobalObject *
+		aPromise	0x07dfb550 {mRefCnt={mRefCntAndFlags=9 } _mOwningThread={mThread=0x0ad67210 } mGlobal={mRawPtr=0x07d24fb8 {...} } ...}	mozilla::dom::Promise *
+		aParent	0x0c9ff1c0 {mGlobal={...} mActor=0x00000000 <NULL> mRefCnt={mRefCntAndFlags=7 } ...}	nsISupports *
+		aArgs	{mValue={VCacheMatchArgs={u={mBytes=0x1fe3de68 "XHÀ\x16\x3" mDummy=13266602072 } } VCacheMatchAllArgs=...} ...}	const mozilla::dom::cache::CacheOpArgs &


Call Stack:

>	xul.dll!mozilla::dom::cache::CacheChild::ExecuteOp(nsIGlobalObject * aGlobal, mozilla::dom::Promise * aPromise, nsISupports * aParent, const mozilla::dom::cache::CacheOpArgs & aArgs) Line 70	C++
 	xul.dll!mozilla::dom::cache::Cache::ExecuteOp(mozilla::dom::cache::AutoChildOpArgs & aOpArgs, mozilla::ErrorResult & aRv) Line 538	C++
 	xul.dll!mozilla::dom::cache::Cache::Match(const mozilla::dom::RequestOrUSVString & aRequest, const mozilla::dom::CacheQueryOptions & aOptions, mozilla::ErrorResult & aRv) Line 252	C++
 	xul.dll!mozilla::dom::CacheBinding::match(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::cache::Cache * self, const JSJitMethodCallArgs & args) Line 511	C++
 	xul.dll!mozilla::dom::CacheBinding::match_promiseWrapper(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::cache::Cache * self, const JSJitMethodCallArgs & args) Line 530	C++
 	xul.dll!mozilla::dom::GenericPromiseReturningBindingMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 2760	C++
 	xul.dll!js::Invoke(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 460	C++
 	xul.dll!Interpret(JSContext * cx, js::RunState & state) Line 2781	C++
 	xul.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 407	C++
 	xul.dll!js::Invoke(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 481	C++
 	xul.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, const JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 512	C++
 	xul.dll!JS::Call(JSContext * cx, JS::Handle<JS::Value> thisv, JS::Handle<JS::Value> fval, const JS::HandleValueArray & args, JS::MutableHandle<JS::Value> rval) Line 2837	C++
 	xul.dll!mozilla::dom::AnyCallback::Call(JSContext * cx, JS::Handle<JS::Value> aThisVal, JS::Handle<JS::Value> value, JS::MutableHandle<JS::Value> aRetVal, mozilla::ErrorResult & aRv) Line 79	C++
 	xul.dll!mozilla::dom::AnyCallback::Call(JS::Handle<JS::Value> value, JS::MutableHandle<JS::Value> aRetVal, mozilla::ErrorResult & aRv, const char * aExecutionReason, mozilla::dom::CallbackObject::ExceptionHandling aExceptionHandling, JSCompartment * aCompartment) Line 149	C++
 	xul.dll!mozilla::dom::WrapperPromiseCallback::Call(JSContext * aCx, JS::Handle<JS::Value> aValue) Line 341	C++
 	xul.dll!mozilla::dom::PromiseReactionJob::Run() Line 106	C++
 	xul.dll!mozilla::dom::Promise::PerformMicroTaskCheckpoint() Line 568	C++
 	xul.dll!mozilla::CycleCollectedJSRuntime::AfterProcessTask(unsigned int aRecursionDepth) Line 1101	C++
 	xul.dll!`anonymous namespace'::WorkerJSRuntime::AfterProcessTask(unsigned int aRecursionDepth) Line 969	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 982	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 297	C++
 	xul.dll!mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext * aCx) Line 4543	C++
 	xul.dll!`anonymous namespace'::WorkerThreadPrimaryRunnable::Run() Line 2724	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 964	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 297	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 355	C++
 	xul.dll!MessageLoop::RunHandler() Line 228	C++
 	xul.dll!MessageLoop::Run() Line 202	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 378	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 419	C
 	nss3.dll!pr_root(void * arg) Line 90	C
 	[External Code]	
 	[Frames below may be incorrect and/or missing, no symbols loaded for msvcr120.dll]

Comment 1

[Looben Yang]

Attachment: NullPtr_ExecuteOp_repro.js

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I'll take a look at this.  I think the edge case here is that the cache.open() is being run at the top level of the service worker and the worker thread is being killed in the middle.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

My best guess here is that the CacheOpChild actor is being created after we've signalled its ok to terminate the worker thread.  The code assumes if it can create an Op actor, then it can hold the worker thread alive.  We probably need to avoid creating the op if the feature has already signaled that thread destruction can proceed.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

What is happening here is:

1) We enter Cache::Match() with an mActor, so we pass the initial mActor nullptr check.
2) We construct a Request object which uses the URL sync loop operations.
3) The sync loop executes control runnables and triggers a WorkerFeature::Notify()
4) The Cache mActor doesn't have an open operation yet, so it thinks its ok to shutdown.  It releases its Feature.
5) The mActor pointer is cleared
6) Cache::Match() tries to use the mActor pointer to create an op actor.
7) boom

I'm investigating the best ways to fix this so it covers all our mActor uses, not just Cache::Match().

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow the CacheChild to be "locked" into memory so it will delay destruction. r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

This fixes the crash for me locally.  I'll work on translating the test case in comment 0 into a mochitest.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8701211 [details] [diff] [review]
P1 Allow the CacheChild to be "locked" into memory so it will delay destruction. r=ehsan

Since this will likely be reviewed while I'm on PTO, I'm going to go ahead and ask for sec-approval now.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It allows content to consistently trigger a null pointer de-reference.  Its unclear to me how actionable that is in practice.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I think they reflect some kind of lifetime issue, but perhaps not the method for forcing it to happen.  You need the combination of a rapidly destroyed worker thread and a path through Cache API that uses a sync loop.

I plan to land a test in a follow-up bug.

Which older supported branches are affected by this flaw?

FF39+

If not all supported branches, which bug introduced the flaw?

It landed with the initial Cache API.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This should backport cleanly.  The Cache API has been relatively stable.

How likely is this patch to cause regressions; how much testing does it need?

The Cache API has a stable test suite with good coverage.  I don't expect any regressions here.  If this caused a leak it would have shown up locally.

Comment 9

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

Comment on attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

sec-approval+ assuming it passes review.

We'll want this on Aurora and Beta as well.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Ben, could you fill the uplift requests? Thanks

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

It still needs review.  Ehsan?

Comment 12

[(no longer active)]

FWIW, I'm not convinced that this is sec-critical, or security sensitive at all.  AFAICT this will only result in a null pointer crash which is quite safe.  mActor cannot be a garbage value here.

Comment 13

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #12)
> FWIW, I'm not convinced that this is sec-critical, or security sensitive at
> all.  AFAICT this will only result in a null pointer crash which is quite
> safe.  mActor cannot be a garbage value here.

Al, should we open up the bug?

Comment 14

[(no longer active)]

Comment on attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

Review of attachment 8701212 [details] [diff] [review]:
-----------------------------------------------------------------

(Sorry for the delay in review!)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8701211 [details] [diff] [review]
P1 Allow the CacheChild to be "locked" into memory so it will delay destruction. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Cache API (shipped in FF 39)
[User impact if declined]: Crash can be triggered with carefully crafted client script.
[Describe test coverage new/current, TreeHerder]: Tested locally with provided test case.  Will land tests in separate bug 1234946.
[Risks and why]: Minimal as it only affects the Cache API which is mainly used with Service Workers.
[String/UUID change made/needed]: None.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

See comment 15.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701211 [details] [diff] [review]
P1 Allow the CacheChild to be "locked" into memory so it will delay destruction. r=ehsan

Crash fix, beta44+, aurora45+

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

beta44+, aurora45+

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Landed:

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/482240e6ab0d
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/de0d8df586dc

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/98bc42206c56
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9a5b82d99605

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a7704ec681b3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a37226fb8470

Comment 20

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

Opening this up is Dveditz's decision.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/482240e6ab0d
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/de0d8df586dc

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7704ec681b3
https://hg.mozilla.org/mozilla-central/rev/a37226fb8470