1423225 - Disable SAB + Atomics on Central + Beta

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Lars T Hansen [:lth]]

Attachment: disable-sab-atomics-on-beta.patch

Related to bug 1415303.  We may need to disable SAB+Atomics.

James, can you look at the WPT parts of this patch to find out if they're OK?  If we disable SAB+Atomics many WPT start failing because they assume that those objects are present.  My changes introduce the necessary guards.

Luke, can you run your eyes over the other parts of the patch?  Should be OK generally because we've been over this territory before but it's good to have an r+.

Patch applies to beta and central.

Comment 1

[James Graham [:jgraham]]

It seems like the right way to handle the wpt tests is just to set the expectation value to FAIL in the corresponding .ini file, unless SAB is considered an optional feature.

Comment 2

[Lars T Hansen [:lth]]

(In reply to James Graham [:jgraham] from comment #1)
> It seems like the right way to handle the wpt tests is just to set the
> expectation value to FAIL in the corresponding .ini file, unless SAB is
> considered an optional feature.

Sure, I can use the ini files, a quick test suggests that will work.

Whether SAB is an optional feature - in ECMAScript it is not, but if we merge these tests upstream again the ini files will surely conflict with browsers (or other environments) where SAB is still available.  Are ini files private to us?

Comment 3

[James Graham [:jgraham]]

The ini files are private to us, so updating them to reflect how we actually behave is the right thing to do vs adjusting the tests.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8934557 [details] [diff] [review]
disable-sab-atomics-on-beta.patch

New patch coming in a bit.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1423225-disable-shared-memory.patch

Now with ini files instead of changing the test cases.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8935011 [details] [diff] [review]
bug1423225-disable-shared-memory.patch

Carry luke's r+ for the non-WPT parts of the test.

Comment 7

[Daniel Veditz [:dveditz]]

Not giving this one a severity to avoid bloating our sec-bug stats, but mitigates (partially) a sec-high bug and the folks making release decisions should treat it that way.

Comment 8

[Lars T Hansen [:lth]]

Attachment: [e-mail]

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 8935377 [details]
[e-mail]

Rob, is this roughly right for the add-on?  I've been unable to test this at all; firefox just gives me some variation on the theme "incompatible with your browser".  I've tried about 10 different min/max version numbering schemes here w/o luck, and various browser versions...  Any help you can offer is welcome.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Lars T Hansen [:lth] from comment #9)
> Comment on attachment 8935377 [details]
> disable-js-shared-memory.xpi
> 
> Rob, is this roughly right for the add-on?  I've been unable to test this at
> all; firefox just gives me some variation on the theme "incompatible with
> your browser".  I've tried about 10 different min/max version numbering
> schemes here w/o luck, and various browser versions...  Any help you can
> offer is welcome.

Since this is a legacy add-on, you'll require a non-release/beta build with the extensions.legacy.enabled pref flipped. Since it's unsigned, you also need to either flip xpinstall.signatures.required (or use about:debugging which will load it temporarily and avoid the signature check)

Also, I just took a look at the add-on and if it's just a simple pref flip, we don't really need to bother with building a legacy add-on like this and could likely use Shield instead... I'll send an email about this.

If we do decide that a system add-on update is the way to go here, just a note that the way they get loaded is a little different http://firefox-source-docs.mozilla.org/toolkit/mozapps/extensions/addon-manager/SystemAddons.html and the bottom line is that you can't load unsigned legacy add-ons of any type on release/beta channel builds, so you'll need to use something else (nightly/dev edition/local build/unbranded) if you want to test before signing.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8935377 [details]
[e-mail]

Jason, could you please sign this system add-on update?

Please note that we need to not post this XPI (signed or unsigned versions) publicly anywhere - attaching the signed version to this bug as usual is fine, I just wanted to make sure I mention this.

I reviewed this and also verified locally that it works as expected on Nightly (since we can't test unsigned legacy add-ons on release), using both about:debugging and also the system add-on update mechanism (so we know the values in install.rdf etc. are all good)

Thanks!

Comment 12

[Jason Thomas [:jason]]

Attachment: [e-mail]

Please see attached.

Comment 13

[Lars T Hansen [:lth]]

It came to me this morning that we will need to flip this pref not only on 57, but on 56 and 55 as well, because shared memory was enabled by default in those releases.

Rob, will the same form of system add-on (with the min version number properly changed) work for those releases?

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to Lars T Hansen [:lth] from comment #13)
> It came to me this morning that we will need to flip this pref not only on
> 57, but on 56 and 55 as well, because shared memory was enabled by default
> in those releases.
> 
> Rob, will the same form of system add-on (with the min version number
> properly changed) work for those releases?

Yes, I'd suggest using minVersion 55.* and let's also bump the version number (so we don't get it confused with the version that's already been signed)

Comment 15

[Lars T Hansen [:lth]]

Attachment: [e-mail]

Updated with FF version number guards and version number of add-on.

Needs review, signing, and testing.

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8936030 [details]
[e-mail]

lgtm, and tested locally with nightly.

Jason could you please sign this? Thanks!

Comment 17

[Robert Helmer [:rhelmer]]

Attachment: update v2

Discussed with Rehan, we decided that the safest course here is local testing.

Notes for QA. These steps should work with the signed version of the add-on in release versions of Firefox:

1) In about:config, check that javascript.options.shared_memory pref is true (default)

2) download attached XML file and note full filesystem path (you'll need a file:// URL for the next step)

3) Open about:config and modify the extensions.systemAddon.update.url pref to point to the attached XML file (the default is an https:// URL, you can change this to a file:// URL pointing to the XML file from step 1)

4) In about:config, flip the extensions.logging.enabled and devtools.chrome.enabled prefs to true

5) Open the Browser Console (Tools->Web Developer->Browser Console)

6) At the prompt at the bottom of the Browser Console, force an update check by running:

Cu.import("resource://gre/modules/AddonManager.jsm");
AddonManagerPrivate.backgroundUpdateCheck();

There should be no errors in the Browser Console.

7) Open about:support and verify that system add-on appears and information is correct

8) In about:config that javascript.options.shared_memory is set to true.

This should persist across a restart.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #17)
> Created attachment 8936086 [details]
> update v2
> 
> Discussed with Rehan, we decided that the safest course here is local
> testing.
> 
> Notes for QA.
>
> 8) In about:config that javascript.options.shared_memory is set to true.
> 
> This should persist across a restart.

Correction on the final step:

8) In about:config check that javascript.options.shared_memory is set to *false*

Comment 19

[Robert Helmer [:rhelmer]]

Attachment: notes for QA

Attaching notes for QA so we can update the steps if needed.

Comment 20

[Jason Thomas [:jason]]

Attachment: [e-mail] signed

Please see attached.

Comment 21

[Robert Helmer [:rhelmer]]

Attachment: update v2 (signed)

Here's an update.xml for local testing which has the correct message digest and size for the signed XPI in attachment 8936030 [details]

Note that the URL in this file will be adjusted for the local machine, all other values should be left as-is.

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

I've tried to test this using Latest Nightly and 58.0b12 with the provided steps but unfortunately I still get an error that the XPI is unsigned when forcing the update.

Here is the full console log: https://goo.gl/3XP7kk

When loading the XPI via about:debugging the prefs are changed but they won't last until a restart (expected, since the add-on is temporary loaded).

Robert, is there something I'm missing here?

Comment 23

[Robert Helmer [:rhelmer]]

(In reply to Cornel Ionce [:cornel_ionce], Desktop Release QA from comment #22)
> I've tried to test this using Latest Nightly and 58.0b12 with the provided
> steps but unfortunately I still get an error that the XPI is unsigned when
> forcing the update.
> 
> Here is the full console log: https://goo.gl/3XP7kk
> 
> When loading the XPI via about:debugging the prefs are changed but they
> won't last until a restart (expected, since the add-on is temporary loaded).
> 
> Robert, is there something I'm missing here?

Hm, I'll try it again myself today... just to double-check, you are using the signed version in attachment 8936270 [details] ? Thanks!

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Yes, double checked right now and still got the same results.

Comment 25

[Robert Helmer [:rhelmer]]

(In reply to Cornel Ionce [:cornel_ionce], Desktop Release QA from comment #24)
> Yes, double checked right now and still got the same results.

Huh! I just ran through this on macOS release channel in a new profile (57.0.2, 20171206182557) and it WFM using the attached instructions.

Could you please attach the full output of the Browser Console? Thanks!

Comment 26

[Bogdan Maris, Desktop QA]

(In reply to Robert Helmer [:rhelmer] from comment #25)
> Huh! I just ran through this on macOS release channel in a new profile
> (57.0.2, 20171206182557) and it WFM using the attached instructions.
> 
> Could you please attach the full output of the Browser Console? Thanks!

[following up on behalf of Cornel, since he's on PTO]

Just tried this myself on 57.0.2 (20171206182557) using Windows 10 64-bit and it fails on my end as well. 

Here's what I did:
1. Downloaded update.xml from Comment 21 and [e-mail] from Comment 20.
 - note that I got the same error whether I used the xpi file from Comment 15 or the signed one from Comment 20
2. Edited the update.xml file and changed the value of the "URL" parameter to my local path (that points to the xpi file).
3. Pointed extensions.systemAddon.update.url to the downloaded update.xml file.
4. Ran the code snippet to force an add-on update check and got the following error:
> 1513845213191 addons.xpi ERROR Failed to download system add-on [e-mail]: NetworkError: A network error occurred. Stack trace: downloadFile/<()@resource://gre/modules/addons/ProductAddonChecker.jsm:333 < AMudownloadFile()@resource://gre/modules/addons/ProductAddonChecker.jsm:295 < downloadAddon()@resource://gre/modules/addons/ProductAddonChecker.jsm:449 < downloadAddon()@resource://gre/modules/addons/XPIProvider.jsm:2691 < updateSystemAddons()@resource://gre/modules/addons/XPIProvider.jsm:2698 Log.jsm:752

Full Browser Console logs since browser start-up: https://pastebin.com/vcwKmxTb.

Let me know if there's anything else I can help with here.

Comment 27

[Robert Helmer [:rhelmer]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #26)
> (In reply to Robert Helmer [:rhelmer] from comment #25)
> > Huh! I just ran through this on macOS release channel in a new profile
> > (57.0.2, 20171206182557) and it WFM using the attached instructions.
> > 
> > Could you please attach the full output of the Browser Console? Thanks!
> 
> [following up on behalf of Cornel, since he's on PTO]
> 
> Just tried this myself on 57.0.2 (20171206182557) using Windows 10 64-bit
> and it fails on my end as well. 
> 
> Here's what I did:
> 1. Downloaded update.xml from Comment 21 and
> [e-mail] from Comment 20.
>  - note that I got the same error whether I used the xpi file from Comment
> 15 or the signed one from Comment 20
> 2. Edited the update.xml file and changed the value of the "URL" parameter
> to my local path (that points to the xpi file).
> 3. Pointed extensions.systemAddon.update.url to the downloaded update.xml
> file.
> 4. Ran the code snippet to force an add-on update check and got the
> following error:
> > 1513845213191 addons.xpi ERROR Failed to download system add-on [e-mail]: NetworkError: A network error occurred. Stack trace: downloadFile/<()@resource://gre/modules/addons/ProductAddonChecker.jsm:333 < AMudownloadFile()@resource://gre/modules/addons/ProductAddonChecker.jsm:295 < downloadAddon()@resource://gre/modules/addons/ProductAddonChecker.jsm:449 < downloadAddon()@resource://gre/modules/addons/XPIProvider.jsm:2691 < updateSystemAddons()@resource://gre/modules/addons/XPIProvider.jsm:2698 Log.jsm:752

Hm what is the URL param in the update.xml file set to (step #2 above)?

> 
> Full Browser Console logs since browser start-up:
> https://pastebin.com/vcwKmxTb.
> 
> Let me know if there's anything else I can help with here.

Comment 28

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #27)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #26)
> > Full Browser Console logs since browser start-up:
> > https://pastebin.com/vcwKmxTb.


Also could you please delete this pastebin? I don't want to leak any info, would be better to attach any info like this to the bug in the future. Thanks!

Comment 29

[Bogdan Maris, Desktop QA]

(In reply to Robert Helmer [:rhelmer] from comment #28)
> (In reply to Robert Helmer [:rhelmer] from comment #27)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #26)
> > > Full Browser Console logs since browser start-up:
> > > https://pastebin.com/vcwKmxTb.
> 
> 
> Also could you please delete this pastebin? I don't want to leak any info,
> would be better to attach any info like this to the bug in the future.
> Thanks!

Sure, I deleted it. (I still saved the logs locally if there is any need for them)

(In reply to Robert Helmer [:rhelmer] from comment #27)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #26)
> > (In reply to Robert Helmer [:rhelmer] from comment #25)
> > > Huh! I just ran through this on macOS release channel in a new profile
> > > (57.0.2, 20171206182557) and it WFM using the attached instructions.
> > > 
> > > Could you please attach the full output of the Browser Console? Thanks!
> > 
> > [following up on behalf of Cornel, since he's on PTO]
> > 
> > Just tried this myself on 57.0.2 (20171206182557) using Windows 10 64-bit
> > and it fails on my end as well. 
> > 
> > Here's what I did:
> > 1. Downloaded update.xml from Comment 21 and
> > [e-mail] from Comment 20.
> >  - note that I got the same error whether I used the xpi file from Comment
> > 15 or the signed one from Comment 20
> > 2. Edited the update.xml file and changed the value of the "URL" parameter
> > to my local path (that points to the xpi file).
> > 3. Pointed extensions.systemAddon.update.url to the downloaded update.xml
> > file.
> > 4. Ran the code snippet to force an add-on update check and got the
> > following error:
> > > 1513845213191 addons.xpi ERROR Failed to download system add-on [e-mail]: NetworkError: A network error occurred. Stack trace: downloadFile/<()@resource://gre/modules/addons/ProductAddonChecker.jsm:333 < AMudownloadFile()@resource://gre/modules/addons/ProductAddonChecker.jsm:295 < downloadAddon()@resource://gre/modules/addons/ProductAddonChecker.jsm:449 < downloadAddon()@resource://gre/modules/addons/XPIProvider.jsm:2691 < updateSystemAddons()@resource://gre/modules/addons/XPIProvider.jsm:2698 Log.jsm:752
> 
> Hm what is the URL param in the update.xml file set to (step #2 above)?
> 
> > 
> > Full Browser Console logs since browser start-up:
> > https://pastebin.com/vcwKmxTb.
> > 
> > Let me know if there's anything else I can help with here.

I path I used was:
> file:///C:/Users/andrei.vaida/Desktop/update.xml

I retested again on other machines today and it worked for me without problems. The add-on is successfully installed and "javascript.options.shared_memory" pref is set to false after running the update snippet, also the values is kept after restarts. 
Testing was done across platforms (Window 10 64bit, macOS 10.13.2 and Ubuntu 16.04 64bit) on Firefox 57.0.2, Firefox 58.0b12 and latest Nightly 59.0a1.

Comment 30

[Robert Helmer [:rhelmer]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> I retested again on other machines today and it worked for me without
> problems. The add-on is successfully installed and
> "javascript.options.shared_memory" pref is set to false after running the
> update snippet, also the values is kept after restarts. 
> Testing was done across platforms (Window 10 64bit, macOS 10.13.2 and Ubuntu
> 16.04 64bit) on Firefox 57.0.2, Firefox 58.0b12 and latest Nightly 59.0a1.

OK great this sounds good then - let me know if you need anything. If you figure out the cause of the previous failure I'd be interested, but we can do that on irc/slack private message and not in this bug :)

Comment 31

[Luke Wagner [:luke]]

For anyone manually testing that the system addon worked: go to any normal webpage, open the web console, type in "SharedArrayBuffer".  With the addon, there should be an error ("ReferenceError: SharedArrayBuffer is not defined").

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like we are going to quickly do some functional testing right now, get this lined up for release and signoff and aim for releasing the system add-on today for Firefox versions 55-59 (but not ESR)

Comment 33

[Rehan Dalal (Inactive)]

The addon is now shipping to the test channel(s) "release-sysaddon" and "beta-sysaddon".

The rules have also been created for "release" and "beta" and are pending sign off from relman/QA.

Comment 34

[Selena Deckelmann :selenamarie :selena]

Comment on attachment 8935011 [details] [diff] [review]
bug1423225-disable-shared-memory.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

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

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? 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?

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 35

[Selena Deckelmann :selenamarie :selena]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 

No

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?

Yes

If not all supported branches, which bug introduced the flaw? 

Do you have backports for the affected branches? 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?

Test cases are included in attached file from rhelmer and in comment 31.

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 8935011 [details] [diff] [review]
bug1423225-disable-shared-memory.patch

sec-approval=dveditz

Please do not land until the blog post goes live later today (likely around 2pm)

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8935011 [details] [diff] [review]
bug1423225-disable-shared-memory.patch

We shouldn't merge/land this till later today (after 2pm-ish Pacific)

Comment 38

[Stefan [:StefanG_QA]]

We've finished testing of the addon on release-sysaddon and beta-sysaddon channels on Windows 7/10 x64, Win 10 x86 and Ubuntu 17.10 x64 with Fx 57.0.3 and 58.0b13. No issues found. 

Here are our findings: The add-on is successfully installed and "javascript.options.shared_memory" pref is set to false after running the update snippet, also the values is kept after restart. "SharedArrayBuffer" throws an "ReferenceError: SharedArrayBuffer is not defined" 

Our results are here: https://goo.gl/Xc7zwX

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2368bc8e65daba4232b31e607edc666d2f47a63b

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

QA and relman signed off in balrog. This should now be live.

We are also ready to land the patches for 57.0.4 and 58.0b15.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df3efa640b0f443abe5309d7655275da0017f5e1
https://hg.mozilla.org/releases/mozilla-release/rev/6caa457ebedc915b43dc1d054b8fe22e82ca7447

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Correction on comment 40, we will build beta with this patch as 58.0b14 build 2.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

I can confirm that on nightly 20180104100157, it is fixed.

Comment 44

[Ciprian Georgiu, Desktop QA]

We also managed to verify this fix on 57.0.4-build1 (20180103231032) and 58.0b14-build2 (20180103230655) by following the STR from comment 38 across platforms: Windows 10 x64, Windows 7 x86, macOS 10.13 and Ubuntu 16.04 x64.

Comment 45

[Ciprian Georgiu, Desktop QA]

Marking this verified fixed on latest Nightly 59.0a1 per comment 43.