Skip to content

Commit

Permalink
Bug 1538015 (part 2) - restrict when incoming synced prefs are applie…
Browse files Browse the repository at this point in the history 
…d. r=tcsc,pauljt,Gijs, a=RyanVM

Specifically, a "control pref" for a pref must already exist locally, or
a new preference, `services.sync.prefs.dangerously_allow_arbitrary` must
be set to true.

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.

Differential Revision: https://phabricator.services.mozilla.com/D29775

--HG--
extra : histedit_source : 40454bf4a75a018e29e1a70f930128a43576594d
  • Loading branch information
@mhammond
mhammond committed Jun 25, 2019
1 parent e759d40 commit 4b78820
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 21 deletions.
18 changes: 6 additions & 12 deletions browser/app/profile/firefox.js
View file
Expand Up @@ -1171,10 +1171,6 @@ pref("services.sync.prefs.sync.browser.link.open_newwindow", true);
pref("services.sync.prefs.sync.browser.newtabpage.enabled", true);
pref("services.sync.prefs.sync.browser.newtabpage.pinned", true);
pref("services.sync.prefs.sync.browser.offline-apps.notify", true);
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);
pref("services.sync.prefs.sync.browser.search.update", true);
pref("services.sync.prefs.sync.browser.sessionstore.restore_on_demand", true);
pref("services.sync.prefs.sync.browser.startup.homepage", true);
Expand All @@ -1194,7 +1190,6 @@ pref("services.sync.prefs.sync.dom.disable_open_during_load", true);
pref("services.sync.prefs.sync.dom.disable_window_flip", true);
pref("services.sync.prefs.sync.dom.disable_window_move_resize", true);
pref("services.sync.prefs.sync.dom.event.contextmenu.enabled", true);
pref("services.sync.prefs.sync.extensions.personas.current", true);
pref("services.sync.prefs.sync.extensions.update.enabled", true);
pref("services.sync.prefs.sync.intl.accept_languages", true);
pref("services.sync.prefs.sync.layout.spellcheckDefault", true);
Expand All @@ -1205,8 +1200,6 @@ pref("services.sync.prefs.sync.network.cookie.lifetimePolicy", true);
pref("services.sync.prefs.sync.network.cookie.lifetime.days", true);
pref("services.sync.prefs.sync.network.cookie.thirdparty.sessionOnly", true);
pref("services.sync.prefs.sync.permissions.default.image", 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);
pref("services.sync.prefs.sync.pref.downloads.disable_button.edit_actions", true);
pref("services.sync.prefs.sync.pref.privacy.disable_button.cookie_exceptions", true);
pref("services.sync.prefs.sync.privacy.clearOnShutdown.cache", true);
Expand All @@ -1225,15 +1218,16 @@ pref("services.sync.prefs.sync.privacy.resistFingerprinting", true);
pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true);
pref("services.sync.prefs.sync.privacy.resistFingerprinting.reduceTimerPrecision.microseconds", true);
pref("services.sync.prefs.sync.privacy.resistFingerprinting.reduceTimerPrecision.jitter", true);
pref("services.sync.prefs.sync.security.OCSP.enabled", true);
pref("services.sync.prefs.sync.security.OCSP.require", true);
pref("services.sync.prefs.sync.security.default_personal_cert", true);
pref("services.sync.prefs.sync.security.tls.version.min", true);
pref("services.sync.prefs.sync.security.tls.version.max", true);
pref("services.sync.prefs.sync.services.sync.syncedTabs.showRemoteIcons", true);
pref("services.sync.prefs.sync.signon.rememberSignons", true);
pref("services.sync.prefs.sync.spellchecker.dictionary", true);
pref("services.sync.prefs.sync.xpinstall.whitelist.required", true);

// A preference which, if false, means sync will only apply incoming preference
// changes if there's already a local services.sync.prefs.sync.* control pref.
// If true, all incoming preferences will be applied and the local "control
// pref" updated accordingly.
pref("services.sync.prefs.dangerously_allow_arbitrary", false);

// A preference that controls whether we should show the icon for a remote tab.
// This pref has no UI but exists because some people may be concerned that
Expand Down
90 changes: 86 additions & 4 deletions services/sync/modules/engines/prefs.js
View file
Expand Up @@ -21,6 +21,45 @@ ChromeUtils.defineModuleGetter(this, "LightweightThemeManager",
XPCOMUtils.defineLazyGetter(this, "PREFS_GUID",
() => CommonUtils.encodeBase64URL(Services.appinfo.ID));

// In bug 1538015, we decided that it isn't always safe to allow all "incoming"
// preferences to be applied locally. So we have introduced another preference,
// which if false (the default) will ignore all incoming preferences which don't
// already have the "control" preference locally set. If this new
// preference is set to true, then we continue our old behavior of allowing all
// preferences to be updated, even those which don't already have a local
// "control" pref.
const PREF_SYNC_PREFS_ARBITRARY = "services.sync.prefs.dangerously_allow_arbitrary";

XPCOMUtils.defineLazyPreferenceGetter(this, "ALLOW_ARBITRARY", PREF_SYNC_PREFS_ARBITRARY);

// The SUMO supplied URL we log with more information about how custom prefs can
// continue to be synced. SUMO have told us that this URL will remain "stable".
const PREFS_DOC_URL_TEMPLATE = "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/sync-custom-preferences";
XPCOMUtils.defineLazyGetter(this, "PREFS_DOC_URL",
() => Services.urlFormatter.formatURL(PREFS_DOC_URL_TEMPLATE));


// Check for a local control pref or PREF_SYNC_PREFS_ARBITRARY
this.isAllowedPrefName = function(prefName) {
if (prefName == PREF_SYNC_PREFS_ARBITRARY) {
return false; // never allow this.
}
if (ALLOW_ARBITRARY) {
// user has set the "dangerous" pref, so everything is allowed.
return true;
}
// The pref must already have a control pref set, although it doesn't matter
// here whether that value is true or false. We can't use prefHasUserValue
// here because we also want to check prefs still with default values.
try {
Services.prefs.getBoolPref(PREF_SYNC_PREFS_PREFIX + prefName);
// pref exists!
return true;
} catch (_) {
return false;
}
};

function PrefRec(collection, id) {
CryptoWrapper.call(this, collection, id);
}
Expand Down Expand Up @@ -101,15 +140,26 @@ PrefStore.prototype = {
_getSyncPrefs() {
let syncPrefs = Services.prefs.getBranch(PREF_SYNC_PREFS_PREFIX)
.getChildList("", {})
.filter(pref => !isUnsyncableURLPref(pref));
.filter(pref => isAllowedPrefName(pref) && !isUnsyncableURLPref(pref));
// Also sync preferences that determine which prefs get synced.
let controlPrefs = syncPrefs.map(pref => PREF_SYNC_PREFS_PREFIX + pref);
return controlPrefs.concat(syncPrefs);
},

_isSynced(pref) {
return pref.startsWith(PREF_SYNC_PREFS_PREFIX) ||
this._prefs.get(PREF_SYNC_PREFS_PREFIX + pref, false);
if (pref.startsWith(PREF_SYNC_PREFS_PREFIX)) {
// this is an incoming control pref, which is ignored if there's not already
// a local control pref for the preference.
let controlledPref = pref.slice(PREF_SYNC_PREFS_PREFIX.length);
return isAllowedPrefName(controlledPref);
}

// This is the pref itself - it must be both allowed, and have a control
// pref which is true.
if (!this._prefs.get(PREF_SYNC_PREFS_PREFIX + pref, false)) {
return false;
}
return isAllowedPrefName(pref);
},

_getAllPrefs() {
Expand Down Expand Up @@ -143,11 +193,43 @@ PrefStore.prototype = {
// _isSynced returns false when 'foo.pref' doesn't exist (e.g., on a new device).
let prefs = Object.keys(values).sort(a => -a.indexOf(PREF_SYNC_PREFS_PREFIX));
for (let pref of prefs) {
let value = values[pref];
if (!this._isSynced(pref)) {
// An extra complication just so we can warn when we decline to sync a
// preference due to no local control pref.
if (!pref.startsWith(PREF_SYNC_PREFS_PREFIX)) {
// this is an incoming pref - if the incoming value is not null and
// there's no local control pref, then it means we would have previously
// applied a value, but now will decline to.
// We need to check this here rather than in _isSynced because the
// default list of prefs we sync has changed, so we don't want to report
// this message when we wouldn't have actually applied a value.
// We should probably remove all of this in ~ Firefox 80.
if (value !== null) { // null means "use the default value"
let controlPref = PREF_SYNC_PREFS_PREFIX + pref;
let controlPrefExists;
try {
Services.prefs.getBoolPref(controlPref);
controlPrefExists = true;
} catch (ex) {
controlPrefExists = false;
}
if (!controlPrefExists) {
// This is a long message and written to both the sync log and the
// console, but note that users who have not customized the control
// prefs will never see this.
let msg = `Not syncing the preference '${pref}' because it has no local ` +
`control preference (${PREF_SYNC_PREFS_PREFIX}${pref}) and ` +
`the preference ${PREF_SYNC_PREFS_ARBITRARY} isn't true. ` +
`See ${PREFS_DOC_URL} for more information`;
console.warn(msg);
this._log.warn(msg);
}
}
}
continue;
}

let value = values[pref];
if (typeof value == "string" && UNSYNCABLE_URL_REGEXP.test(value)) {
this._log.trace(`Skipping incoming unsyncable url for pref: ${pref}`);
continue;
Expand Down
4 changes: 3 additions & 1 deletion services/sync/tests/unit/test_engine_changes_during_sync.js
View file
Expand Up @@ -146,7 +146,9 @@ add_task(async function test_passwords_change_during_sync() {
add_task(async function test_prefs_change_during_sync() {
_("Ensure that we don't bump the score when applying prefs.");

const TEST_PREF = "services.sync.prefs.sync.test.duringSync";
const TEST_PREF = "test.duringSync";
// create a "control pref" for the pref we sync.
Services.prefs.setBoolPref("services.sync.prefs.sync.test.duringSync", true);

enableValidationPrefs();

Expand Down
58 changes: 54 additions & 4 deletions services/sync/tests/unit/test_prefs_store.js
View file
Expand Up @@ -106,20 +106,24 @@ add_task(async function run_test() {
"testing.synced.url": "blob:ebeb707a-502e-40c6-97a5-dd4bda901463",
// Make sure we can replace the unsynced URL with a valid URL.
"testing.unsynced.url": "https://www.example.com/2",
// Make sure our "master control pref" is ignored.
"services.sync.prefs.dangerously_allow_arbitrary": true,
"services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary": true,
};
await store.update(record);
// Note that 'prefs' here is looking at the *control* prefs only.
Assert.strictEqual(prefs.get("testing.int"), 42);
Assert.strictEqual(prefs.get("testing.string"), "im in ur prefs");
Assert.strictEqual(prefs.get("testing.bool"), false);
Assert.strictEqual(prefs.get("testing.deleted-without-control-pref"), "I'm deleted-without-control-pref");
Assert.strictEqual(prefs.get("testing.deleted-with-local-control-pref"), undefined);
Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), undefined);
Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), "I'm deleted-with-incoming-control-pref");
Assert.strictEqual(prefs.get("testing.dont.change"), "Please don't change me.");
Assert.strictEqual(prefs.get("testing.somepref"), "im a new pref from other device");
Assert.strictEqual(prefs.get("testing.somepref"), undefined);
Assert.strictEqual(prefs.get("testing.synced.url"), "https://www.example.com");
Assert.strictEqual(prefs.get("testing.unsynced.url"), "https://www.example.com/2");
Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), true);
Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), undefined);
Assert.strictEqual(prefs.get("services.sync.prefs.dangerously_allow_arbitrary"), false);
Assert.strictEqual(prefs.get("services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary"), undefined);

_("Enable persona");
// Ensure we don't go to the network to fetch personas and end up leaking
Expand Down Expand Up @@ -192,3 +196,49 @@ add_task(async function run_test() {
prefs.resetBranch("");
}
});

add_task(async function test_dangerously_allow() {
_("services.sync.prefs.dangerously_allow_arbitrary");
// read our custom prefs file before doing anything.
Services.prefs.readDefaultPrefsFromFile(do_get_file("prefs_test_prefs_store.js"));
// configure so that arbitrary prefs are synced.
Services.prefs.setBoolPref("services.sync.prefs.dangerously_allow_arbitrary", true);

let engine = Service.engineManager.get("prefs");
let store = engine._store;
let prefs = new Preferences();
try {
_("Update some prefs");
// This pref is not going to be reset or deleted as there's no "control pref"
// in either the incoming record or locally.
prefs.set("testing.deleted-without-control-pref", "I'm deleted-without-control-pref");
// Another pref with only a local control pref.
prefs.set("testing.deleted-with-local-control-pref", "I'm deleted-with-local-control-pref");
prefs.set("services.sync.prefs.sync.testing.deleted-with-local-control-pref", true);
// And a pref without a local control pref but one that's incoming.
prefs.set("testing.deleted-with-incoming-control-pref", "I'm deleted-with-incoming-control-pref");
let record = new PrefRec("prefs", PREFS_GUID);
record.value = {
"testing.deleted-without-control-pref": null,
"testing.deleted-with-local-control-pref": null,
"testing.deleted-with-incoming-control-pref": null,
"services.sync.prefs.sync.testing.deleted-with-incoming-control-pref": true,
"testing.somepref": "im a new pref from other device",
"services.sync.prefs.sync.testing.somepref": true,
// Make sure our "master control pref" is ignored, even when it's already set.
"services.sync.prefs.dangerously_allow_arbitrary": false,
"services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary": true,

};
await store.update(record);
Assert.strictEqual(prefs.get("testing.deleted-without-control-pref"), "I'm deleted-without-control-pref");
Assert.strictEqual(prefs.get("testing.deleted-with-local-control-pref"), undefined);
Assert.strictEqual(prefs.get("testing.deleted-with-incoming-control-pref"), undefined);
Assert.strictEqual(prefs.get("testing.somepref"), "im a new pref from other device");
Assert.strictEqual(Svc.Prefs.get("prefs.sync.testing.somepref"), true);
Assert.strictEqual(prefs.get("services.sync.prefs.dangerously_allow_arbitrary"), true);
Assert.strictEqual(prefs.get("services.sync.prefs.sync.services.sync.prefs.dangerously_allow_arbitrary"), undefined);
} finally {
prefs.resetBranch("");
}
});

0 comments on commit 4b78820

Please sign in to comment.