Closed Bug 1524946 Opened 1 year ago Closed 1 year ago

Crash in mozilla::dom::HostResolveImportedModule

Categories

(Core :: JavaScript Engine, defect)

Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 + fixed
firefox67 + fixed

People

(Reporter: gsvelto, Assigned: jonco)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file)

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

This is Android-specific and looks like an UAF.

Assignee: nobody → jcoppeard

Marking s-s for UAF.

Group: javascript-core-security

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

Attachment #9041470 - Flags: review?(jdemooij)
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
Attachment #9041470 - Flags: review?(jdemooij) → review+
Blocks: 1519140

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

Attachment #9041470 - Flags: sec-approval?
Comment on attachment 9041470 [details] [diff] [review]
bug1524946-module-private-lifetime

[Triage Comment]
sec-approval+ and a=dveditz for beta uplift
Attachment #9041470 - Flags: sec-approval?
Attachment #9041470 - Flags: sec-approval+
Attachment #9041470 - Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.