Closed Bug 1496159 Opened 2 years ago Closed 2 years ago

We can end up trying to load modules from the bytecode cache

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main63+][adv-esr60.3+])

Attachments

(3 files)

Which won't work until bug 1436400 is fixed.  Right now we end up with bytecode for a script but treated as a module, which I am told is bad.

The fix for bug 1493449 hit this condition in our existing tests, but I will post a dedicated testcase that triggers the problem even without bug 1493449.
Actually, due to the async way in which we do the bytecode encoding, a dedicated testcase is not trivial.  Or at least I have not managed to write one yet.
Attachment #9014111 - Flags: review?(nicolas.b.pierron)
Comment on attachment 9014111 [details] [diff] [review]
Don't try to load modules from the bytecode cache

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Not very easily,  I suspect.  The commit message makes it  clear that this is about incorrectly reading modules as bytecode.  I could try to change that, but I think that's pretty clear from the patch anyway.

Once that's determined, the question is how easy it is to construct a script which would do bad things if this bug is hit.  That I do not know.

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

Which older supported branches are affected by this flaw?: All of them back to 60

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?: This doesn't need much testing and is not likely to cause regressions.  However it does block some changes to module loading that we'd like to happen sooner rather than later.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1240072

User impact if declined: Possibly exploitable.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Add crossorigin="with-credentials" to all the <script type="module"> inline scripts in browser/components/payments/test/mochitest/

2. Run "mach mochitest -f plain browser/components/payments/test/mochitest/"

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This just makes sure we don't even try to read modules from the bytecode cache.

String changes made/needed: None.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: I suspect this is at least sec:high.

User impact if declined: Possibly exploitable.

Fix Landed on Version: 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This just makes sure we don't even try to read modules from the bytecode cache.

String or UUID changes made by this patch: None.
Attachment #9014111 - Flags: sec-approval?
Attachment #9014111 - Flags: approval-mozilla-esr60?
Attachment #9014111 - Flags: approval-mozilla-beta?
Keywords: sec-high
Marking sec-high per comment 3.
Attachment #9014111 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)
> Actually, due to the async way in which we do the bytecode encoding, a
> dedicated testcase is not trivial.  Or at least I have not managed to write
> one yet.

When the dom.expose_test_interfaces preference is set, each script tag emit a list of events which can be collected for writing test cases.

This test case [1] uses that with a script tag which has the "watchme" id, to wait until the bytecode is encoded and saved.
Then another script can be loaded in another iframe to request the same file as a module instead.

[1] https://searchfox.org/mozilla-central/source/dom/base/test/test_script_loader_js_cache.html#126-128
We only have two betas left. This bug will need release management approval, so we can backport to affected branches, before I can give sec-approval.

Ritu?
Flags: needinfo?(rkothari)
We still have enough time to take this, yes.
Flags: needinfo?(rkothari) → needinfo?(abillings)
Comment on attachment 9014111 [details] [diff] [review]
Don't try to load modules from the bytecode cache

approvals given.
Flags: needinfo?(abillings)
Attachment #9014111 - Flags: sec-approval?
Attachment #9014111 - Flags: sec-approval+
Attachment #9014111 - Flags: approval-mozilla-esr60?
Attachment #9014111 - Flags: approval-mozilla-esr60+
Attachment #9014111 - Flags: approval-mozilla-beta?
Attachment #9014111 - Flags: approval-mozilla-beta+
Attached patch testSplinter Review
This test shouldn't be checked in until the bug is opened up.  Without the patch for this bug, we crash on this test in opt and assert in debug.
Attachment #9014678 - Flags: review?(nicolas.b.pierron)
Attachment #9014678 - Flags: review?(nicolas.b.pierron) → review+
Flags: in-testsuite?
QA Contact: overholt
https://hg.mozilla.org/mozilla-central/rev/0cf6d7594c48
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
It appears that this bug got the qe-verify+ tag and got on our verification lists, however, it does not seem to have an easily understandable test case. 
@BZ: Can you tell me how I can reproduce so I can verify it?
Flags: needinfo?(bzbarsky)
(In reply to Bodea Daniel [:danibodea] from comment #13)
> It appears that this bug got the qe-verify+ tag and got on our verification
> lists, however, it does not seem to have an easily understandable test case. 
> @BZ: Can you tell me how I can reproduce so I can verify it?

To test it you would have to create 2 web pages. One web page would have the following script tag:

  <script src="script.js"></script>

And the other would have:

  <script src="script.js" type="module" crossorigin="use-credentials"></script>

Load the first web page multiple times (at least 5), and between each reload, ensure that Firefox becomes idle again.
Then load the second web page.

The file script.js should contain valid JavaScript code, and it should be cached. When after 5 reloads, the code would be encoded as bytecode. The bug should appear while loading the second page, when the JS code is encoded as bytecode.

Firefox should crash before this fix, and it should not crash after this fix.
Flags: needinfo?(bzbarsky)
Another option is the steps from comment 3:

1. Add crossorigin="with-credentials" to all the <script type="module"> inline scripts in browser/components/payments/test/mochitest/

2. Run "mach mochitest -f plain browser/components/payments/test/mochitest/"
And to be clear, when following the steps from comment 16 I suspect you need to make sure the loads happen over http://.  Loads over file:// probably don't hit this issue.
> And to be clear, when following the steps from comment 16

I meant comment 14.
I have attempted to reproduce the issue on Nightly v64.0a1 from 2018-10-03 by creating the 2 pages loaded via http using a local server with a script loaded in both pages; I used the steps from comment 14, however, I am sure it's much more complicated than it seems and I might really be missing bits and pieces of development information. I did not succeed in reproducing a crash.

Boris, can you please write a dedicated test case that I could use? 
I would be really wasting time trying to reproduce it any more than this.
Flags: needinfo?(bzbarsky)
> Boris, can you please write a dedicated test case that I could use? 

I already did.  It's attached to this bug.  See the "test" attachment.

But really, the steps from comment 3 are the easy way to go here from my point of view.

I do suggest reproducing in a debug build.  That should guarantee a fatal assertion failure when you reproduce, whereas the crash in an opt build may depend on random factors...
Flags: needinfo?(bzbarsky)
Whiteboard: [adv-main63+][adv-esr60.3+]
Tried to manually reproduce this issue and verify it with the STR from comment 14 adding the additional info from comment 16, with no real results. 

We've also fumbled with comment 3 testcase, but since running mochitests is something that is not in the manual QA scope and we have no real experience with it, we stopped investing time into it when we got into environment configuration issues. That is an area that manual QA could definitely get some experience in, but I don't see that happen in due time for this bug verification. 

Based on the above, I am replacing the qe+ flag with a qe? flag. Boris, do you reckon the verification of this bug would be enough to be done on your side and if so, could you assist with it?
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(bzbarsky)
I have already done all the verification steps in question, multiple times.  So if we're ok with it just being on my say-so we're done here.
Flags: needinfo?(bzbarsky)
Marking this issue as verified as per comment 21.
Status: RESOLVED → VERIFIED
Flags: qe-verify?
Component: DOM → DOM: Core & HTML
Group: core-security-release
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.