Crash in mozilla::dom::HostResolveImportedModule

RESOLVED FIXED in Firefox 66

Status

()

defect
--
critical
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: gsvelto, Assigned: jonco)

Tracking

(4 keywords)

unspecified
mozilla67
Unspecified
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 unaffected, firefox66+ fixed, firefox67+ fixed)

Details

(Whiteboard: [post-critsmash-triage], crash signature)

Attachments

(1 attachment)

Reporter

Description

5 months ago

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

Reporter

Comment 1

5 months ago

This is Android-specific and looks like an UAF.

Assignee

Updated

5 months ago
Assignee: nobody → jcoppeard
Assignee

Comment 2

5 months ago

Marking s-s for UAF.

Group: javascript-core-security
Assignee

Comment 3

5 months ago

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+
Assignee

Updated

5 months ago
Blocks: 1519140
Assignee

Comment 5

5 months ago

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: 5 months 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.