1524946 - Crash in mozilla::dom::HostResolveImportedModule

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-0748001c-efa7-4079-9083-e51570190203.

Top 10 frames of crashing thread:

0 libxul.so mozilla::dom::HostResolveImportedModule mfbt/RefPtr.h:44
1 libxul.so js::FinishDynamicModuleImport js/src/builtin/ModuleObject.cpp:1665
2 libxul.so JS::FinishDynamicModuleImport js/src/jsapi.cpp:3717
3 libxul.so mozilla::dom::ScriptLoader::FinishDynamicImport dom/script/ScriptLoader.cpp:940
4 libxul.so mozilla::dom::ScriptLoader::EvaluateScript dom/script/ScriptLoader.cpp:2594
5 libxul.so mozilla::dom::ScriptRequestProcessor::Run dom/script/ScriptLoader.cpp:2233
6 libxul.so nsContentUtils::AddScriptRunner dom/base/nsContentUtils.cpp:5278
7 libxul.so mozilla::dom::ModuleLoadRequest::LoadFinished dom/script/ScriptLoader.cpp:1032
8 libxul.so mozilla::MozPromise<nsTArray<bool>, nsresult, true>::ThenValue<mozilla::dom::ModuleLoadRequest*, void  xpcom/threads/MozPromise.h:510
9 libxul.so mozilla::MozPromise<nsTArray<bool>, nsresult, true>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:392

Comment 1

[Gabriele Svelto [:gsvelto]]

This is Android-specific and looks like an UAF.

Comment 2

[Jon Coppeard (:jonco)]

Marking s-s for UAF.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1524946-module-private-lifetime

I don't know for sure, but I think the problem is that there's nothing necessarily holding the referencing script (and hence the loader private value) alive while we do the import.

The patch increments the reference count while the import is happening. (It's an invariant that the module loader must always call js::FinishDynamicModuleImport if hook called StartDynamicModuleImport succeeds.)

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 9041470 [details] [diff] [review]
bug1524946-module-private-lifetime

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

Makes sense.

::: js/src/builtin/ModuleObject.cpp
@@ +1714,5 @@
>      }
>      return promise;
>    }
>  
> +

Nit: extra blank line

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 9041470 [details] [diff] [review]
bug1524946-module-private-lifetime

Security Approval Request

How easily could an exploit be constructed based on the patch?

I think triggering a crash would be reasonably straightforward based on this.

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

No

Which older supported branches are affected by this flaw?

Beta is affected

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

Bug 1519140

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

Same patch should apply

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

Very unlikely

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 9041470 [details] [diff] [review]
bug1524946-module-private-lifetime

[Triage Comment]
sec-approval+ and a=dveditz for beta uplift

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faded33ed22f63a16ed902c2f3e41b39cb4371f3

Comment 8

[Jon Coppeard (:jonco)]

Fix for rooting hazard:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9841e4b69dc2936cba17fb1c937b9eb8f3bb5d6d

Comment 9

[Jon Coppeard (:jonco)]

Backed out for rooting hazards:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6370b90702b776d95057c95a72d13909ed294fed

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7ad8bdd115a8f64c78057ebc327a099807108e
https://hg.mozilla.org/mozilla-central/rev/8a7ad8bdd115

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2305c930b771