If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: Looben Yang, Assigned: bkelly)

Tracking

({crash, csectype-nullptr, testcase})

46 Branch
mozilla46
crash, csectype-nullptr, testcase
Points:
---

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44+ fixed, firefox45+ fixed, firefox46+ fixed, firefox-esr38 unaffected, b2g-v2.5 fixed)

Details

(Whiteboard: [sg:dos])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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]
(Reporter)

Comment 1

2 years ago
Created attachment 8700910 [details]
NullPtr_ExecuteOp_repro.js
(Assignee)

Comment 2

2 years ago
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.
Assignee: nobody → bkelly
Group: core-security
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Component: DOM: Service Workers → DOM
Summary: Service Worker - Crash by NULL pointer write in CacheChild::ExecuteOp() → Cache API crash by NULL pointer write in CacheChild::ExecuteOp()
(Assignee)

Comment 3

2 years ago
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.
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox45: --- → affected
(Assignee)

Comment 4

2 years ago
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().
(Assignee)

Comment 5

2 years ago
Created attachment 8701211 [details] [diff] [review]
P1 Allow the CacheChild to be "locked" into memory so it will delay destruction. r=ehsan
Attachment #8701211 - Flags: review?(ehsan)
(Assignee)

Comment 6

2 years ago
Created attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan
Attachment #8701212 - Flags: review?(ehsan)
(Assignee)

Comment 7

2 years ago
This fixes the crash for me locally.  I'll work on translating the test case in comment 0 into a mochitest.
(Assignee)

Comment 8

2 years ago
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.
Attachment #8701211 - Flags: sec-approval?
(Assignee)

Updated

2 years ago
Attachment #8701212 - Flags: sec-approval?
(Assignee)

Updated

2 years ago
Keywords: sec-critical
status-firefox42: affected → wontfix
status-firefox43: affected → wontfix
status-firefox-esr38: --- → unaffected
tracking-firefox44: --- → +
tracking-firefox45: --- → +
tracking-firefox46: --- → +
Attachment #8701211 - Flags: sec-approval? → sec-approval+
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.
Attachment #8701212 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

2 years ago
Blocks: 1234946
Ben, could you fill the uplift requests? Thanks
Flags: needinfo?(bkelly)
(Assignee)

Comment 11

2 years ago
It still needs review.  Ehsan?
Flags: needinfo?(bkelly) → needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(abillings)
Keywords: sec-critical
Attachment #8701211 - Flags: review?(ehsan) → review+
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!)
Attachment #8701212 - Flags: review?(ehsan) → review+
(Assignee)

Comment 15

2 years ago
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.
Attachment #8701211 - Flags: approval-mozilla-beta?
Attachment #8701211 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

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

See comment 15.
Attachment #8701212 - Flags: approval-mozilla-beta?
Attachment #8701212 - Flags: approval-mozilla-aurora?
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+
Attachment #8701211 - Flags: approval-mozilla-beta?
Attachment #8701211 - Flags: approval-mozilla-beta+
Attachment #8701211 - Flags: approval-mozilla-aurora?
Attachment #8701211 - Flags: approval-mozilla-aurora+
Comment on attachment 8701212 [details] [diff] [review]
P2 Lock the CacheChild actor while Cache DOM methods are running. r=ehsan

beta44+, aurora45+
Attachment #8701212 - Flags: approval-mozilla-beta?
Attachment #8701212 - Flags: approval-mozilla-beta+
Attachment #8701212 - Flags: approval-mozilla-aurora?
Attachment #8701212 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

2 years ago
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
status-firefox44: affected → fixed
status-firefox45: affected → fixed
Group: core-security → dom-core-security
Opening this up is Dveditz's decision.
Flags: needinfo?(abillings) → needinfo?(dveditz)
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/482240e6ab0d
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/de0d8df586dc
status-b2g-v2.5: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/a7704ec681b3
https://hg.mozilla.org/mozilla-central/rev/a37226fb8470
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → mozilla46
Group: dom-core-security
Flags: needinfo?(dveditz)
Keywords: csectype-nullptr, testcase
Whiteboard: [sg:dos]
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1376920
You need to log in before you can comment on or make changes to this bug.