Disable SAB + Atomics on Central + Beta

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug, {sec-other})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57+ verified, firefox58+ verified, firefox59+ verified)

Details

(Whiteboard: mitigation for sec-high bug [go-faster-system-addon])

Attachments

(5 attachments, 4 obsolete attachments)

Assignee

Description

2 years ago
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.
Attachment #8934557 - Flags: review?(luke)
Attachment #8934557 - Flags: review?(james)
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.

Updated

2 years ago
Attachment #8934557 - Flags: review?(luke) → review+
Assignee

Comment 2

2 years ago
(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?
Flags: needinfo?(james)
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.
Flags: needinfo?(james)
Assignee

Comment 4

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

New patch coming in a bit.
Attachment #8934557 - Flags: review?(james)
Assignee

Comment 5

2 years ago
Now with ini files instead of changing the test cases.
Attachment #8934557 - Attachment is obsolete: true
Attachment #8935011 - Flags: review?(james)
Assignee

Comment 6

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

Carry luke's r+ for the non-WPT parts of the test.
Attachment #8935011 - Flags: review+
Attachment #8935011 - Flags: review?(james) → review+
Group: core-security → javascript-core-security
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.
Keywords: sec-other
Whiteboard: mitigation for sec-high bug
Assignee

Comment 8

2 years ago
Assignee

Updated

2 years ago
Whiteboard: mitigation for sec-high bug → mitigation for sec-high bug [go-faster-system-addon]
Assignee

Comment 9

2 years ago
Comment on attachment 8935377 [details]
disable-js-shared-memory@mozilla.org.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.
Flags: needinfo?(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.
Flags: needinfo?(rhelmer) → needinfo?(lhansen)
Comment on attachment 8935377 [details]
disable-js-shared-memory@mozilla.org.xpi

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!
Attachment #8935377 - Attachment description: disable-js-shared-memory.xpi → disable-js-shared-memory@mozilla.org.xpi
Attachment #8935377 - Attachment filename: disable-js-shared-memory.xpi → disable-js-shared-memory@mozilla.org.xpi
Flags: needinfo?(lhansen) → needinfo?(jthomas)
Please see attached.
Assignee

Comment 13

2 years ago
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?
Flags: needinfo?(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)
Flags: needinfo?(rhelmer)
Flags: needinfo?(lhansen)
Flags: needinfo?(jthomas)
Assignee

Comment 15

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

Needs review, signing, and testing.
Attachment #8935377 - Attachment is obsolete: true
Attachment #8935639 - Attachment is obsolete: true
Flags: needinfo?(lhansen)
Attachment #8936030 - Flags: review?(rhelmer)
Comment on attachment 8936030 [details]
disable-js-shared-memory@mozilla.org-v2.xpi

lgtm, and tested locally with nightly.

Jason could you please sign this? Thanks!
Attachment #8936030 - Attachment description: disable-js-shared-memory.xpi → disable-js-shared-memory@mozilla.org-v2.xpi
Attachment #8936030 - Attachment filename: disable-js-shared-memory.xpi → disable-js-shared-memory@mozilla.org-v2.xpi
Flags: needinfo?(jthomas)
Attachment #8936030 - Flags: review?(rhelmer) → review+
Comment hidden (typo)
Comment hidden (typo)
Posted file notes for QA
Attaching notes for QA so we can update the steps if needed.
Please see attached.
Flags: needinfo?(jthomas)
Attachment #8936086 - Attachment is obsolete: true
Posted file 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.
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?
Flags: needinfo?(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!
Flags: needinfo?(rhelmer) → needinfo?(cornel.ionce)
Yes, double checked right now and still got the same results.
Flags: needinfo?(cornel.ionce)
(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!
Flags: needinfo?(cornel.ionce)
(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 disable-js-shared-memory@mozilla.org-v2.xpi 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 disable-js-shared-memory@mozilla.org: 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.
Flags: needinfo?(cornel.ionce) → needinfo?(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
> disable-js-shared-memory@mozilla.org-v2.xpi 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 disable-js-shared-memory@mozilla.org: 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.
Flags: needinfo?(rhelmer) → needinfo?(bogdan.maris)
(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!
(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
> > disable-js-shared-memory@mozilla.org-v2.xpi 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 disable-js-shared-memory@mozilla.org: 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.
Flags: needinfo?(bogdan.maris) → needinfo?(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 :)
Flags: needinfo?(rhelmer)
Blocks: 1415303
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").
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)
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 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]:
Attachment #8935011 - Flags: sec-approval?
Attachment #8935011 - Flags: approval-mozilla-release?
Attachment #8935011 - Flags: approval-mozilla-beta?
[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 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)
Attachment #8935011 - Flags: sec-approval? → sec-approval+
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)
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
https://hg.mozilla.org/mozilla-central/rev/2368bc8e65daba4232b31e607edc666d2f47a63b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
Attachment #8935011 - Flags: approval-mozilla-release?
Attachment #8935011 - Flags: approval-mozilla-release+
Attachment #8935011 - Flags: approval-mozilla-beta?
Attachment #8935011 - Flags: approval-mozilla-beta+
Correction on comment 40, we will build beta with this patch as 58.0b14 build 2.
I can confirm that on nightly 20180104100157, it is fixed.
Status: RESOLVED → VERIFIED
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.
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.