1538015 - (CVE-2019-11732) Revisit pref sync in to mitigate sandbox escape vector

Status: VERIFIED FIXED

(Firefox :: Sync, enhancement)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

In light of bug 1538008 we should re-think the way we sync preferences. See bug 1538008 for more details.

Comment 1

[Daniel Veditz [:dveditz]]

In particular, process and debugging prefs that are often platform dependent anyway are dangerous to sync.

Comment 2

[:Gijs (he/him)]

Seems like an initial first step would be to limit the synced prefs to pref roots starting with:

accessibility.
browser.
lightweightThemes.
privacy.
network.cookie.
services.sync.syncedTabs.showRemoteIcons
signon.rememberSignons
spellchecker.dictionary

with explicit blocks for anything under browser.remote and browser.safebrowsing, perhaps with followup bugs to move those prefs somewhere else so we can remove the blocklist approach.

Could also do with a follow-up that allows syncing privacy.resistFingerprinting.* to turn it on, but doesn't allow syncing to turn it off, though how we'd implement that, I dunno.

This isn't a panacea because people adding security-sensitive prefs under browser. can still create holes, but it's better than nothing (and we can't really prevent people adding these, sadly?).

Note that there's a few synced-by-default things that I'm deliberately not including (some security. , xpinstall.whitelist.required, for one) that I think just need removing from the list of default synced items. I think that's the right tradeoff; people can still set these prefs themselves and enterprises have other ways of enforcing them that don't involve sync.

Johann, does that sound sane? Am I missing a better plan?

Comment 3

[Johann Hofmann [:johannh]]

I think limiting like that makes some sense, but, to take a step back, what makes us use preferences for this instead of a hard-coded list anyway? Has anyone ever needed to modify this on the fly, without touching m-c code? Could there be a chrome (parent) only API that stores these in memory for testing, then?

I'm trying to understand where the requirement for a pref list of things to sync comes from. Maybe that will also help us design the right security model for it.

Comment 4

[Johann Hofmann [:johannh]]

So, to answer my own question, the requirement may come from the fact that the user can choose different categories of settings to sync in about:preferences. Could we just make prefs for these (on my Nightly 8) categories and hard code their pref values?

Ryan, do you have any thoughts/further insights on that? :)

Comment 5

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #4)

No, these prefs are all specifically about syncing preferences, so "sub items" as it were of the "Preferences" checkbox that shows up in the 6/7/8 checkboxes in the, uh, preferences. To keep it confusing.

That is, when you sync preferences, we don't sync all preferences. That would be a Bad Idea. Instead, we sync a subset.

The subset is/was user-alterable, with some sensible defaults defined.

Removing the ability for users to alter these and keep the list in a jsm or whatever would be possible, but people do modify these lists themselves on an ad-hoc basis when people add new preferences and forget to add them to sync's list, or when there are longstanding / hidden preferences that they want to sync themselves but that we'd never sync from a product perspective. We could decide not to are about that usecase in favour of security, I guess, but I'm not convinced that really buys us much beside user outrage.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Johann Hofmann [:johannh] from comment #4)

No, these prefs are all specifically about syncing preferences, so "sub items" as it were of the "Preferences" checkbox that shows up in the 6/7/8 checkboxes in the, uh, preferences. To keep it confusing.

Ah!

That is, when you sync preferences, we don't sync all preferences. That would be a Bad Idea. Instead, we sync a subset.

The subset is/was user-alterable, with some sensible defaults defined.

Removing the ability for users to alter these and keep the list in a jsm or whatever would be possible, but people do modify these lists themselves on an ad-hoc basis when people add new preferences and forget to add them to sync's list, or when there are longstanding / hidden preferences that they want to sync themselves but that we'd never sync from a product perspective. We could decide not to are about that usecase in favour of security, I guess, but I'm not convinced that really buys us much beside user outrage.

How about allowing users to continue using these prefs to turn syncing a pref off, but not add new prefs? I.e. only support the "false" state of the sync prefs?

Comment 7

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)

How about allowing users to continue using these prefs to turn syncing a pref off, but not add new prefs? I.e. only support the "false" state of the sync prefs?

I mean, the default is false unless we ship a value true for it, so this amounts to restricting the list to items for which we ship a default "sync this" pref, right?

As additional context, I suppose part of the raison d'etre of the existing system is that old-style add-ons could define prefs of their own that they might want synced. That's obviously not a concern anymore now that we're webextensions only.

Comment 8

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #7)

(In reply to Johann Hofmann [:johannh] from comment #6)

How about allowing users to continue using these prefs to turn syncing a pref off, but not add new prefs? I.e. only support the "false" state of the sync prefs?

Yeah, that's the idea, my suggestion was simply to dampen the impact on folks who would like to be able to turn off syncing certain prefs.

Comment 9

[Haik Aftandilian [:haik]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

In particular, process and debugging prefs that are often platform dependent anyway are dangerous to sync.

I agree. Some sandboxing prefs might be valid on Windows, but break the browser on Mac (for example.)

Can we limit the sync'd prefs to just those settable via about:preferences?

People manually setting things in about:config are the more rare advanced users right? And we can add a note in the about:config warning that only prefs set via about:preferences are synced.

Comment 10

[Ryan Kelly [:rfkelly]]

Comment 5 answers my needinfo, likely better than I would have done; thanks :Gijs!

Comment 11

[Daniel Veditz [:dveditz]]

in addition to the list in comment 2, extensions or extension users want some of those settings to be sync'd as well. In the shorter term maybe we could skip those while locking this down, or add the extensions.* tree, but might be nice to add a WebExtension API or manifest.json section that lets an extension declare which settings are reasonable to sync and which are instance-specific.

Comment 12

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #11)

in addition to the list in comment 2, extensions or extension users want some of those settings to be sync'd as well. In the shorter term maybe we could skip those while locking this down, or add the extensions.* tree, but might be nice to add a WebExtension API or manifest.json section that lets an extension declare which settings are reasonable to sync and which are instance-specific.

My understanding is that webextension prefs aren't stored in the regular preference infrastructure, but elsewhere, so this is no longer a concern. Rob, can you confirm?

Comment 13

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #12)

My understanding is that webextension prefs aren't stored in the regular preference infrastructure, but elsewhere, so this is no longer a concern. Rob, can you confirm?

This is correct. Extensions cannot directly access any pref; they have to use the browser.storage.sync API to save data that needs to be synchronized across devices.

Some extension APIs also change well-known prefs (see https://searchfox.org/mozilla-central/search?q=symbol:ExtensionPreferencesManager%23addSetting&redirect=false ), but all of these preference names are static.

Thunderbird will receive the ability to read prefs for the sake of migrating prefs to extension storage (bug 1538590), but sync is probably not relevant there.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Paul, can you offer some guidance how we should proceed here?

Comment 15

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Mark Hammond [:markh] from comment #14)

Paul, can you offer some guidance how we should proceed here?

Well if we are in agreement that only allowing syncing a subset of prefs is a acceptable mitigation, then we just need to define what the list is, and then change the sync code such that we sync a whitelist of pref branches. IIUC, we currently only sync preferences that are listed under services.sync.prefs.sync.* [1]. So I assume the way forward here would be to:

a) create a new pref like services.sync.engine.pref.whitelist which is a whitelist of pref branches that are allowed to be sync
b) when we load prefs from sync, filter the list of prefs to be load to only those which match the whitelist (and of course, prohibit loading of services.sync.engine.pref.whitelist itself

So then we just need a whitelist. Gijs posted a start in comment 2, but I started going through browser.* and actually there seem to be a lot of preferences which would potentially be dangerous in the content process. For example, I assume change browser.hiddenWindowChromeURL would change the URL which is loaded when you start Firefox on OSX. Any pref that has a chrome: URI as its value would surely be dangerous to allow the child process to set.

HOWEVER! The bigger problem for me is 1538024. The fundamental issue is that that the content process should NOT be allowed to set preferences. And therefore the child process must not be allowed to login to sync (or otherwise change sync state in such a way that causes pref changes). We will always have risks when the content process can set arbitrary prefs (even under a whitelist of branches).

So what I am saying here is that if we can implement this filter in a relatively simple way (using the list from comment 2) that we don't think will cause breakage, then it might be worth it as a defense in depth control, but its only a secondary control. But if I had to choose I'd say we should focus on 1538024 as the fix for this issue.

[1] What is services.engine.sync.prefs though?

Comment 16

[Mark Hammond [:markh] [:mhammond]]

(In reply to Paul Theriault [:pauljt] from comment #15)

Well if we are in agreement that only allowing syncing a subset of prefs is a acceptable mitigation,

Sorry Paul, I should have been clearer. I'm not really in agreement about this - many people find this useful and there are many articles on the internet explaining how to customize this so any pref you choose can be synced.

That said though, I understand we want to keep users safe. So I guess I'm asking whether we feel the risks here outweigh the benefits this facility offers to a small-but-vocal subset of our users? Should some of the "dangerous" prefs mentioned in bug 1538008 be unavailable in the release channel instead, thereby also avoiding social-engineering attacks?

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Mark Hammond [:markh] from comment #16)

(In reply to Paul Theriault [:pauljt] from comment #15)

Well if we are in agreement that only allowing syncing a subset of prefs is a acceptable mitigation,

Sorry Paul, I should have been clearer. I'm not really in agreement about this - many people find this useful and there are many articles on the internet explaining how to customize this so any pref you choose can be synced.

That said though, I understand we want to keep users safe. So I guess I'm asking whether we feel the risks here outweigh the benefits this facility offers to a small-but-vocal subset of our users? Should some of the "dangerous" prefs mentioned in bug 1538008 be unavailable in the release channel instead, thereby also avoiding social-engineering attacks?

We need to do something in the short term. I think that the only way that we could justify NOT doing something in this bug, is if we have a short term solution for the either the "move privileged content to a trusted process" (bug 1538008) , or the "prompt the user when a sync change is triggered from web content (bug 1538024).

Note the solution I was proposing in comment 15 is something where power users could still disable the whitelist if they REALLY want to sync obscure prefs. They might still complain as would need to remove the whitelist manually (since we wouldn't sync services.sync.engine.pref.whitelist or whatever we call it) but they technically still COULD.

Implementing a blacklist I guess is better that nothing (like blocking security.*, for example) but I'm sure there are many other vectors. If you know of pref branches that people like to set, then perhaps we could expand the whitelist from comment 2, but I think it needs to be a whitelist to be at all effective as a control.

Comment 18

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS it might be worth taking the question back up to 1538008, since the choice of doing this bug or not depends on the other solutions somewhat. Though ultimately, I really feel that pissing off a few vocal users is worth protecting our wider userbase.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Paul Theriault [:pauljt] from comment #17)

Note the solution I was proposing in comment 15 is something where power users could still disable the whitelist if they REALLY want to sync obscure prefs. They might still complain as would need to remove the whitelist manually (since we wouldn't sync services.sync.engine.pref.whitelist or whatever we call it) but they technically still COULD.

Right, I missed that. I agree that's reasonable, thanks.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1538015 (part 1) - test only change to tighten up the checks being made.

This is a test-only change to make the existing synced prefs tests more
explicit about values being checked and moves a couple of prefs around to make
things a bit clearer, which should make the test changes in part 2 clearer.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1538015 (part 2) - restrict when incoming synced prefs are applied.

This also removes a few preferences from the set we sync by default based
due to potential harm which can be caused syncing inappropriate values.

Depends on D29774

Comment 22

[Mark Hammond [:markh] [:mhammond]]

I've got a patch which I just put up for initial feedback, but thought I should discuss the approach here too.

It combines the allowlist-in-a-pref from Paul and the general approach of Gijs. Specifically:

• There's a "blocklist" which is hard-coded in javascript, so can't be changed by the user.
• There's a "allowlist" which is stored in a preference, so can be changed by the user. However, this preference itself is not synced, so the user must make adjustments to this preference manually on each device.
• The blocklist is consulted first, and the allowlist next.
• The default for the allowlist is such that all preferences we setup to sync by default are allowed.

So what's in these lists?

• The blocklist has the allowlist preference, plus "browser.safebrowsing." and "browser.remote." - I added the latter because Gijs mentioned it in comment 2, although I couldn't find any "browser.remote." preferences?

Note also that this means removing the following preferences:

pref("services.sync.prefs.sync.browser.safebrowsing.phishing.enabled", true);
pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);
pref("services.sync.prefs.sync.browser.safebrowsing.downloads.enabled", true);
pref("services.sync.prefs.sync.browser.safebrowsing.passwords.enabled", true);

so not only are these prefs no longer synced by default, they simply can not be synced. Some sites recommend setting these prefs to false for privacy reasons. However, I think it's easy to make a case that they should be blocklisted.

• While creating the allowlist I ended up deleting a few preference which don't seem to be used these days, along with xpinstall.whitelist.required due to potential harm. These deletions are:

pref("services.sync.prefs.sync.xpinstall.whitelist.required", true);
pref("services.sync.prefs.sync.addons.ignoreUserEnabledChanges", true);
pref("services.sync.prefs.sync.extensions.personas.current", true);
pref("services.sync.prefs.sync.pref.advanced.images.disable_button.view_image", true);
pref("services.sync.prefs.sync.pref.advanced.javascript.disable_button.advanced", true);

which results in the following allowlist:

accessibility.
browser.
dom.disable media.
dom.event.contextmenu.
layout.spellcheckDefault
network.cookie.
extensions.update.enabled
intl.accept_languages
permissions.default.image
pref.downloads.disable_button.
pref.privacy.disable_button.
privacy.
security.default_personal_cert
security.OCSP.
security.tls.version.
services.sync.syncedTabs.
signon.rememberSignons
spellchecker.

which is a bit longer than ideal and what Gijs offered, but reducing it any further means also preventing the syncing of preferences which we sync by default now, which I'm a little reluctant to do without concrete harm being identified so we avoid annoying our users any more than we must. However, it's all obviously on the table for discussion.

Could also do with a follow-up that allows syncing privacy.resistFingerprinting.* to turn it on, but doesn't allow syncing to turn it off, though how we'd implement that, I dunno.

TBH I'm really not that bothered by that - it doesn't seem likely someone would attempt to use what we are fixing here simply to enable fingerprinting.

Note that there's a few synced-by-default things that I'm deliberately not including (some security. , xpinstall.whitelist.required, for one)

As above, I left the existing security prefs, but didn't allowlist the entire security. branch.

The patch is a little larger than it needs to be because I chose to console.warn() when there's a pref that would have been synced before this patch but no longer is - users who haven't enabled custom prefs will never see this warning and users who have might appreciate an indication of what's going on.

Comment 23

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #22)

• The blocklist has the allowlist preference, plus "browser.safebrowsing." and "browser.remote." - I added the latter because Gijs mentioned it in comment 2, although I couldn't find any "browser.remote." preferences?

I meant browser.tabs.remote, apologies for the confusion.

Comment 24

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So Ive had a quick look through the prefs we allow here - there are some semi-dangerous ones I think, but we a never going to be fully safe here. One thing to note however is that Tom has been able to develop the patch to isolate accounts.firefox.com into a separate process. See https://bugzilla.mozilla.org/show_bug.cgi?id=1539595#c9

But in light of that, we should have a think about whether we should still do this - since this was always considered a stopgap measure. On one hand, I would prefer we did this, as an additional control for this type of issue (assuming that it doesn't break use cases). On the other hand, maybe its not worth the added engineering complexity that this change adds.

Or maybe we might just implement a less severe version of the block/allow list.

Comment 25

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just a note: Ive been chatting with Mark on irc and I realised that the current patch misses some of the "dangerous" prefs from the original PoC in 1538008 comment 0

Comment 26

[Mark Hammond [:markh] [:mhammond]]

In that chat, we came up with a far simpler proposal.

• A new preference, say "services.sync.prefs.dangerously_allow_arbitrary" has a default value of false.
• If that preference is false, we treat the "services.sync.prefs.sync." branch as a whitelist - if that pref doesn't already exist locally, we refuse to take the preference. This is different to how it works now - we now happily take new incoming "control" prefs.
• If that new preference is true, we work as we work now - trust anything, block nothing.
• There's no blacklist - we don't even need to blacklist that new preference, we just need to ensure we never enable it by default or create a "control" pref for it (although if we want to guard against accidents we could treat it as a 1-item blocklist)

This means that by default, we only sync prefs that we've shipped as being OK to sync (ie, these prefs)

Most users will see no change and the tiny fraction of users who leverage this have 2 options:

1. Manually create control preferences for the prefs they want to sync on both devices. For users who have many custom prefs, this will be a PITA and effectively kills the feature for them. However, it would allow (say) enterprises to expand that list fairly easily in a safe way.

2. Manually flip "services.sync.prefs.dangerously_allow_arbitrary" on each of their devices. Once flipped, things work for them exactly like they do now.

We'd recommend people use (1) and strongly advise against using (2) (and hopefully the pref name kinda speaks for itself)

Thoughts?

Comment 27

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Would it be possible to make the behavior of dangerously_allow_arbitrary to create services.sync.prefs.sync.foo entries for incoming prefs? This would allow users to turn it on, sync, and then turn it off without having to create custom prefs.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tom Ritter [:tjr] from comment #27)

Would it be possible to make the behavior of dangerously_allow_arbitrary to create services.sync.prefs.sync.foo entries for incoming prefs? This would allow users to turn it on, sync, and then turn it off without having to create custom prefs.

Yep, that's what would happen in the proposal above.

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Mark Hammond [:markh] from comment #26)

In that chat, we came up with a far simpler proposal.

• A new preference, say "services.sync.prefs.dangerously_allow_arbitrary" has a default value of false.
• If that preference is false, we treat the "services.sync.prefs.sync." branch as a whitelist - if that pref doesn't already exist locally, we refuse to take the preference. This is different to how it works now - we now happily take new incoming "control" prefs.
• If that new preference is true, we work as we work now - trust anything, block nothing.
• There's no blacklist - we don't even need to blacklist that new preference, we just need to ensure we never enable it by default or create a "control" pref for it (although if we want to guard against accidents we could treat it as a 1-item blocklist)

This means that by default, we only sync prefs that we've shipped as being OK to sync (ie, these prefs)

Most users will see no change and the tiny fraction of users who leverage this have 2 options:

1. Manually create control preferences for the prefs they want to sync on both devices. For users who have many custom prefs, this will be a PITA and effectively kills the feature for them. However, it would allow (say) enterprises to expand that list fairly easily in a safe way.

2. Manually flip "services.sync.prefs.dangerously_allow_arbitrary" on each of their devices. Once flipped, things work for them exactly like they do now.

We'd recommend people use (1) and strongly advise against using (2) (and hopefully the pref name kinda speaks for itself)

Thoughts?

Sorry for the delay Mark, lost this bug when traveling (Im behind in bugmail). As discussed I think this is a nice solution - basically it just means an even stricter whitelist for regular users. I've done a quick review of the list of prefs and I can't see any obvious attack vectors - some avenue for inconvenience, but if you have a compromised sync account, you are already in a world of inconvenience. So sounds like a good plan to me. Some things to keep in mind is how we prevent people from adding dangerous prefs to this list (but I expect thats just a part of regular review process). Let do it.

Comment 30

[Thom Chiovoloni [:tcsc] (ex-moco)]

Manually flip "services.sync.prefs.dangerously_allow_arbitrary" on each of their devices. Once flipped, things work for them exactly like they do now.

This would be a pretty substantial change to how the current system works, and at a minimum would deserve mentioning in the release notes.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Release Note Request (optional, but appreciated)
[Why is this notable]: This is a change in behaviour for advanced users of sync. The old behaviour is documented on various sites which we don't control.
[Affects Firefox for Android]: No
[Suggested wording]: The way non-default preferences are synced has changed. Please see [sumo link] for more.
[Links (documentation, blog post, etc)]: I'm currently working with sumo to get a link, but thought I'd get this flag set asap.

Comment 32

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/mozilla-central/rev/643214222e38498bbd795d70890f0e6b9ac581b0
https://hg.mozilla.org/mozilla-central/rev/e36730a6d5898a03b25439a75340e46c8240848c

Comment 33

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #31)

Release Note Request (optional, but appreciated)
[Why is this notable]: This is a change in behaviour for advanced users of sync. The old behaviour is documented on various sites which we don't control.
[Affects Firefox for Android]: No
[Suggested wording]: The way non-default preferences are synced has changed. Please see [sumo link] for more.
[Links (documentation, blog post, etc)]: I'm currently working with sumo to get a link, but thought I'd get this flag set asap.

Sumo link is https://support.mozilla.org/kb/sync-custom-preferences - not yet live but that's where it will be.

Comment 34

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 9062381 [details]
Bug 1538015 (part 2) - restrict when incoming synced prefs are applied.

Beta/Release Uplift Approval Request

• User impact if declined: Mitigation for sandbox escape will not exist.

It's my understanding that this patch is desired in esr68, so I'm requesting beta uplift now. If the risk is too great, then maybe this could be considered an esr68 request? (Apologies for not being sure about the correct process here)

• 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: Ideally some QA to ensure preferences syncing still works correctly
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Risk is limited to preferences syncing.
• String changes made/needed:

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 9062380 [details]
Bug 1538015 (part 1) - test only change to tighten up the checks being made.

If we're going to want this on esr68 then might as well get it in now since it's a behaviour change. Approved for 68.0b14

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cb236393456baab3546acbe92f397cb48648cd4f
https://hg.mozilla.org/releases/mozilla-beta/rev/61169eb427180be0b3b8fbbd9c2ef2e7f185a2ae

Comment 37

[Mark Hammond [:markh] [:mhammond]]

https://support.mozilla.org/en-US/kb/sync-custom-preferences is now live.

Comment 38

[ovidiu boca[:Ovidiu]]

I verified this on Mac OS X 10.14, Ubuntu 16.04, Windows 10 with FF Nightly 69.0a1(2019-06-29) and FF Beta 68.0b14 and I can confirm the fact that the sync between the prefs works as expected.

Comment 39

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Julien: why was this wontfixed for esr-60? this is one of the ways to break/hinder one of the pwn2own sandbox-escapes that ZDI is going to blog about. Resetting tracking flags to see if there was more behind this or if it was just a reflexive response to "not sec-high".

Comment 40

[Julien Cristau [:jcristau]]

It was a response to "not sec-high, behaviour change that might break stuff for people, late in the esr60 lifecycle". If we want to take it anyway because it's going to go public, that's fine.

Comment 41

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: rebased vs ESR60: 0001-Bug-1538015-part-1-test-only-change-to-tighten-up-th.patch

At request from :gijs, I've rebased these patches against bookmarks/esr60. 1/2 because I'm unsure if there's an easy way for me to attach multiple patches at once (My bugzilla-fu is rather rusty 😓)

Comment 42

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: rebased vs ESR60: 0002-Bug-1538015-part-2-restrict-when-incoming-synced-pre.patch

Rebased patch 2/2

Comment 43

[:Gijs (he/him)]

Comment on attachment 9075457 [details] [diff] [review]
rebased vs ESR60: 0002-Bug-1538015-part-2-restrict-when-incoming-synced-pre.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: mitigating sec-high/crit sandbox escaping paths
• User impact if declined: see above
• Fix Landed on Version: 69, uplift to 68
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): I mean, it's late in the cycle, yada yada, but at least there's some automated testing, it won't affect anyone except people who use sync, and even there I wouldn't expect it to affect anyone unless they rely on syncing unknown prefs, and there the impact is pretty much what we want it to be - or so we hope. It's had a good amount of baking on nightly/beta and I'm not aware of any issues. Plus, there are easy workarounds (ie pref flips) that we could deploy quickly if things really did go pearshaped. Last but not least, green trypush!
• String or UUID changes made by this patch: nope

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9075456 [details] [diff] [review]
rebased vs ESR60: 0001-Bug-1538015-part-1-test-only-change-to-tighten-up-th.patch

Helps mitigate a sandbox escape. Approved for 60.8esr.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9d066208192089152b2f58379e6acd3e5360c242
https://hg.mozilla.org/releases/mozilla-esr60/rev/cbdd37ea216336ed423014d5607a8a26dfece5a8

Comment 46

[ovidiu boca[:Ovidiu]]

I verified this on Firefox ESR 60.8.0 with Mac OS X 10.14, Windows 10 and Ubuntu 16.04 and I can confirm the fix.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

NI Julien for the relnote request since this is shipping in 68.

Comment 48

[Julien Cristau [:jcristau]]

Added to the draft release notes for 68. I'll also include it for 60.8.0esr.