Boris Zbarsky [:bzbarsky] Assignee
	Description • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 6 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 4 • 6 years ago

Marking sec-high per comment 3.

	Nicolas B. Pierron [:nbp]
	Comment 5 • 6 years ago

(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

	Al Billings [:abillings - ex-MoCo]
	Comment 6 • 6 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 6 years ago

We still have enough time to take this, yes.

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 6 years ago

Comment on attachment 9014111 [details] [diff] [review]
Don't try to load modules from the bytecode cache

approvals given.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf6d7594c4897d43321b825ab663a7b8a4e17af

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/0cf6d7594c48

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/503d94e63a24
https://hg.mozilla.org/releases/mozilla-esr60/rev/e965f6f6ed75

	Daniel Bodea [:danibodea]
	Comment 13 • 6 years ago

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?

	Nicolas B. Pierron [:nbp]
	Comment 14 • 6 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 6 years ago

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/"

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 6 years ago

> And to be clear, when following the steps from comment 16

I meant comment 14.

	Daniel Bodea [:danibodea]
	Comment 18 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 6 years ago

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

	Adrian Florinescu [:aflorinescu]
	Comment 20 • 6 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 6 years ago

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.

	Adrian Florinescu [:aflorinescu]
	Comment 22 • 6 years ago

Marking this issue as verified as per comment 21.

	Pulsebot
	Comment 24 • 4 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/997dc466ff9d
test.