Closed Bug 1529203 Opened 5 years ago Closed 5 years ago

Crash when importing a data URL in the Live DOM Viewer

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 66+ verified
firefox65 --- wontfix
firefox66 + verified
firefox67 + verified

People

(Reporter: annevk, Assigned: jonco)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main66+][adv-esr60.6+])

Crash Data

Attachments

(6 files)

bug1529203-beta-fix
(deleted), patch
bzbarsky
: review-
Details | Diff | Splinter Review
bug1529203-assertions
(deleted), patch
jandem
: review+
Details | Diff | Splinter Review
bug1529203-tests
(deleted), patch
bzbarsky
: feedback+
Details | Diff | Splinter Review
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!
(deleted), text/x-phabricator-request
abillings
: approval-mozilla-beta+
abillings
: sec-approval+
Details
Bug 1529203 - Add tests to check that the module map is cleared when using document.open r=bzbarsky
(deleted), text/x-phabricator-request
Details
bug1529203-esr60-combined
(deleted), patch
jonco
: review+
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review
  1. Go to https://software.hixie.ch/utilities/js/live-dom-viewer/
  2. Replace the text in the top text field with 
    <script>
x = import("data:text/javascript,")
</script>
  3. Add a space or some such.
  4. ...
  5. Crash.

I cannot reproduce it outside the Live DOM Viewer unfortunately.

Adding the crash signature as this is easily reproducible on Mac.

Crash Signature: [@ nsJSUtils::ModuleEvaluate]
Keywords: crash, reproducible
Assignee: nobody → jcoppeard

I can't reproduce this myself, but here's the stack:

MOZ_CrashPrintf
js::ContextChecks::check(JS::Value const&, int)
JS_SetPendingException(JSContext*, JS::Handle<JS::Value>)
mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*)
mozilla::dom::ScriptRequestProcessor::Run()
mozilla::dom::ScriptLoader::ProcessLoadedModuleTree(mozilla::dom::ModuleLoadRequest*)
mozilla::dom::ModuleLoadRequest::ModuleErrored()
mozilla::MozPromise<bool, nsresult, false>::ThenValue<mozilla::dom::ModuleLoadRequest*, void (mozilla::dom::ModuleLoadRequest::)(), void (mozilla::dom::ModuleLoadRequest::)()>::DoResolveOrRejectInternal(mozilla::MozPromise<bool, nsresult, false>::ResolveOrRejectValue&)

From: https://crash-stats.mozilla.com/report/index/71bd0b74-8686-45ac-be6a-a19d00190220

Setting s-s for compartment mismatch assertion.

Group: javascript-core-security

I still can't reproduce this or see how this can happen.

Marcia, does it still reproduce for you? STR would be invaluable.

Flags: needinfo?(mozillamarcia.knous)

It's possible that this might be related to bug 1530146.

I was able to reproduce this I guess in the nightly from 2-20, but not yet using today's nightly. As far as STR, I followed the steps exactly in Comment 0.

Flags: needinfo?(mozillamarcia.knous)

(In reply to Marcia Knous [:marcia - needinfo? me] from comment #6)
Thanks. I get an immediate crash using a build from that date.

I bisected this and found it was fixed by bug 1523843.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Assignee: jcoppeard → bzbarsky
Blocks: 1523843
Group: javascript-core-security → core-security-release
status-firefox65: --- → unaffected
status-firefox66: --- → unaffected
status-firefox67: --- → fixed
status-firefox-esr60: --- → unaffected
Resolution: WORKSFORME → FIXED
Target Milestone: --- → mozilla67
No longer blocks: 1523843
Depends on: 1523843

(In reply to Jon Coppeard (:jonco) from comment #8)

I bisected this and found it was fixed by bug 1523843.

Hm that bug made us have fewer compartments but are we sure the underlying bug is fixed too? There are still compartment boundaries elsewhere..

Flags: needinfo?(jcoppeard)

(In reply to Jan de Mooij [:jandem] from comment #9)
OK that's a really good point.

bz, is it possible for a Document object to be used with more than one global in its lifetime?

Flags: needinfo?(jcoppeard) → needinfo?(bzbarsky)

bz, is it possible for a Document object to be used with more than one global in its lifetime?

Yes, with document.open, which is exactly what's going on here. A minimized testcase for this bug:

  <!DOCTYPE HTML>
  <iframe></iframe>
  <script>
    function doIt() {
      var doc = document.querySelector("iframe").contentDocument;
      doc.open();
      doc.write('<script>import("data:text/javascript,")</' + 'script>');
      doc.close();
    }
  </script>
  <button type="button" onclick="doIt()">
    Click this button, then wait a second and click it again.
  </button>

Even with bug 1523843 fixed, this is an issue for the moment, as this testcase shows:

  <!DOCTYPE HTML>
  <script>
    var win;
    function doIt() {
      if (!win) {
        win = window.open();
      }
      var doc = win.document;
      doc.open();
      doc.write('<script>import("data:text/javascript,")</' + 'script>');
      doc.close();
    }
  </script>
  <button type="button" onclick="doIt()">
    Click this button, then wait a second and click it again.
  </button>

Bug 1489308 will fix the issue by removing the "single document with multiple globals" situation. But that's not landed yet, so we need a different fix for Firefox 66, right?

Flags: needinfo?(bzbarsky) → needinfo?(jcoppeard)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bzbarsky → nobody

Maybe document.open should clear out the module map bits in the scriptloader at the point where it changes the global for the document...

Is 66 affected?

Group: core-security-release → javascript-core-security
status-firefox67: fixed → affected
Target Milestone: mozilla67 → ---

That is what the needinfo is for, to a large extent.

Is this only a problem for dynamic imports, or also for static ones? Dynamic imports seem to be enabled in nightly only at the moment. Static imports have been shipping for a while.

The testcases above are using dynamic imports and don't crash on 66b11: they throw an exception about dynamic import not being supported, as expected.

I tried creating something like that with static imports, but it does not crash in my testing so far. That said, maybe I'm just managing to exercise the right codepaths...

If this is a nightly-only issue, then we're done here; I just landed bug 1489308 earlier today. Maybe it's worth adding a check of some sort to ensure that we don't run modules in the wrong compartment, though...

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)

Thanks for the testcases!

The bad news is that this problem does apply to static imports and hence everything back to FF 60 is affected.

Flags: needinfo?(jcoppeard)
Attached patch bug1529203-beta-fix (deleted) — — Splinter Review

Patch to fix the problem by clearing the module map when we change global for a Document.

Assignee: nobody → jcoppeard
Attachment #9047465 - Flags: review?(bzbarsky)
Attached patch bug1529203-assertions (deleted) — — Splinter Review

Patch to change the compartment checks around modules (including JS_SetPendingException which is called when a module load failure is reported at a later time) into release build checks.

Jan what do you think? Most of our compartment checks are nightly-only but I don't think this is going to be too much overhead.

Attachment #9047466 - Flags: review?(jdemooij)
Attached patch bug1529203-tests (deleted) — — Splinter Review

This is where I got to with tests. The 1529203-1.html test functions correctly but the other two which use window.open() still don't work, with window.open() returning null even with the pref set.

Any more ideas on how to make this work?

Attachment #9047467 - Flags: feedback?(bzbarsky)
Comment on attachment 9047467 [details] [diff] [review]
bug1529203-tests

>+++ b/dom/base/crashtests/1529203-1.html

This test will fire the load event and end the test before the second time doIt runs, I expect.  You probably want to add reftest-wait to the test and remove it once the second import has actually happened.

>+++ b/dom/base/crashtests/1529203-2.html

I've seen some vague references to window.open() only working in non-e10s reftests.... Maybe this should just be a mochitest instead, where all this stuff works correctly.  Crashes in mochitests do count as failures, I believe.

Same thing for the -3 version.
Attachment #9047467 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 9047465 [details] [diff] [review]
bug1529203-beta-fix

I'm not sure SetScriptGlobalObject is the right place to do this.  In particular, I would expect that to break documents that go into bfcache and then come out of it; suddenly their module map will be cleared.  Note that in this case we do SetScriptGlobalObject(nullptr) followed by SetScriptGlobalObject(the-thing-we-used-to-have-before).

I'd prefer we do this in the place where the badness with changing globals actually happens: document.open.
Attachment #9047465 - Flags: review?(bzbarsky) → review-

It's also possible that setting pref(browser.link.open_newwindow,2) for the tests (to open in new windows, not new tabs) will work in crashtests...

Comment on attachment 9047466 [details] [diff] [review]
bug1529203-assertions

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

Good idea.
Attachment #9047466 - Flags: review?(jdemooij) → review+

Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I'm not sure how to estimate this. Possible but not easy I guess.
  • 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?: Everything back to FF 60
  • If not all supported branches, which bug introduced the flaw?: Bug 1240072
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Should be simple to create and low risk.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this is a quite a simple change.
Attachment #9047746 - Flags: sec-approval?

Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1240072
  • User impact if declined: Possible crash / security vulnerability.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • 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): This is a simple change to clear the module map when a document's global is changed via window.open. I'd say it's low risk.
  • String changes made/needed:
Attachment #9047746 - Flags: approval-mozilla-beta?

I'm planning to submit all patches to beta, but only the tests and extra assertions to trunk as this issue is no longer present there due to the changes in bug 1489308.

Note, it's probably best not to check in the tests until the bug is unhidden.

Comment on attachment 9047746 [details]
Bug 1529203 - Clear the module map when changing a Document's global r=bzbarsky!

Sec-approval+ for trunk and beta approval.

Since you state this goes back to Firefox 60, we should get an ESR60 patch made and nominated as well.

Attachment #9047746 - Flags: sec-approval?
Attachment #9047746 - Flags: sec-approval+
Attachment #9047746 - Flags: approval-mozilla-beta?
Attachment #9047746 - Flags: approval-mozilla-beta+

https://hg.mozilla.org/mozilla-central/rev/7f2bfa2b5fb9

Group: javascript-core-security → core-security-release
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Attached patch bug1529203-esr60-combined (deleted) — — Splinter Review

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fix for sec-high bug.
  • User impact if declined: Possible crash / security vulnerability.
  • Fix Landed on Version: 66
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a simple change and shouldn't cause problems.
  • String or UUID changes made by this patch: None
Attachment #9048157 - Flags: review+
Attachment #9048157 - Flags: approval-mozilla-esr60?
Comment on attachment 9048157 [details] [diff] [review]
bug1529203-esr60-combined

Fixes a JS sec-high. Approved for 60.6esr.
Attachment #9048157 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

I managed to reproduce the issue on an older version of Nightly (2019-02-20) on Windows 10 x64.
I retested everything using latest Nightly 67.0a1, beta 66.0b13 and esr 60.5.3 and the tab doesn't crash anymore.
However, I tried the steps on Firefox 65.0.2 and the bug is not reproducing. Considering the fact that the flag at the moment is wontfix, did something happened or that build wasn't affected in the first place?

Flags: needinfo?(jcoppeard)

(In reply to Oana Botisan, Desktop Release QA from comment #36)
These STR will only work on FF 66 and later, but the problem itself exists from FF 60.

Flags: needinfo?(jcoppeard)

(In reply to Jon Coppeard (:jonco) from comment #37)

These STR will only work on FF 66 and later, but the problem itself exists from FF 60.

Thank you for the info. Considering the fact that firefox65 was flagged with wontfix, I assume it's not an issue.

Then I will mark this bug as verified fixed as per comment 36.

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: fixed → verified
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+][adv-esr60.6+]
Group: core-security-release
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9133eeb54db8
Add tests to check that the module map is cleared when using document.open r=bzbarsky
Regressions: 1640150
Attachment #9047747 - Attachment description: Bug 1529203 - Add tests to check that the module map is cleared when using document.open r=bzbarsky! → Bug 1529203 - Add tests to check that the module map is cleared when using document.open r=bzbarsky
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f12d21ad24e4
Add tests to check that the module map is cleared when using document.open r=bzbarsky
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: