Closed Bug 1029148 Opened 5 years ago Closed 5 years ago

store "current search engine" configuration outside of prefs

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2

People

(Reporter: Gavin, Assigned: florian)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

A common way to hijack searches is to override the browser.search.selecedEngine pref (e.g. by writing to the user's pref file, or dropping in a user.js file in the profile, or shipping an addon with a different value for that pref).

Since this is a high-value configuration option, it makes more sense to store this information outside of prefs (which are designed to be modular and overridden).

The requirements here are:
- remove all uses of browser.search.selectedEngine pref in Firefox. This probably involves adjusting the engineMetadataService to allow setting "global" metadata not tied to a particular engine, and then using that mechanism to store a selectedEngine attribute. 
- on migration to Firefox 33, migrate the value of that pref to the new storage (users should experience no change in selected engine as a result of this change)
Flags: firefox-backlog+
Some questions:
1. whatever storage we use here, it will still be some sort of simple file in the profile folder (let's say for example we store a json in searchplugins folder). How does this mitigate the hijacking issue, since one can just replace/modify this file instead of user.js? This us going to break the already existing methods, but new methods are basically the same with some minor changes.
2. We will have to add specific code for enterprises using ESR to manage their preferred default engine, prefs were giving this for free
3. We will need to add code to Sync to synchronize the choice, it currently just synchronizes prefs (among which browser.search.selectedEngine).

The costs so far look higher than the small benefit of breaking existing methods.
QA Whiteboard: [qa+]
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Points: --- → 8
(In reply to Marco Bonardo [:mak] from comment #1)
> 1. whatever storage we use here, it will still be some sort of simple file
> in the profile folder (let's say for example we store a json in
> searchplugins folder). How does this mitigate the hijacking issue, since one
> can just replace/modify this file instead of user.js? This us going to break
> the already existing methods, but new methods are basically the same with
> some minor changes.

The goal is to mitigate in the short term, not address hijacking once and for all. We are working on various other efforts to tackle the broader problem, this is just a part of it.

> 2. We will have to add specific code for enterprises using ESR to manage
> their preferred default engine, prefs were giving this for free

We're not removing "defaultenginename", which is used to set the default engine for new profiles, so this shouldn't be a problem.

> 3. We will need to add code to Sync to synchronize the choice, it currently
> just synchronizes prefs (among which browser.search.selectedEngine).

Yes, this will be complicated. I discussed this with Florian, I might be Ok with losing this functionality.
Status: ASSIGNED → NEW
Iteration: 33.2 → ---
Points: 8 → ---
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Points: --- → 8
Here is a summary of (what I took away from) the conversations I had with Gavin on this topic:

Goal: we want to make it as difficult as possible to hijack the search engine by dropping a file or editing prefs.js in the profile folder.

Non-goal: attempting to mitigate hijacking done by touching the application folder, or by installing an add-on (and then using the nsIBrowserSearchService API).

Current state:
Since bug 860560 setting a user value to either of the browser.search.selectedEngine or browser.search.defaultenginename preferences changes the engine.

Proposed changes:
- completely stop using browser.search.selectedEngine (except for a one time migration),
- ignore user values of browser.search.defaultenginename (we still need to respect the default value, because that's the way the default search engine is customized for different locales, or for ESR users).
- Store the current search engine in the search-metadata.json file (this means the engineMetadataService object in nsSearchService.js may need some tweaking).
- No longer sync the selected search engine.
- Include in the search-metadata.json file a verification hash that should be unique to the current profile. This is to prevent hijacking through an installer copying the same search-metadata.json file to the profile of all users.
So basically generate a random key, store it in search-metadata.json and somewhere else (preferences, again?) and check that it is correct? Or use a hash from the current profile name?
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> So basically generate a random key, store it in search-metadata.json and
> somewhere else (preferences, again?) and check that it is correct? Or use a
> hash from the current profile name?

Indeed, using the profile name was what I had in mind. Saving something else in the preferences wouldn't be a real improvement.
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> > So basically generate a random key, store it in search-metadata.json and
> > somewhere else (preferences, again?) and check that it is correct? Or use a
> > hash from the current profile name?
> 
> Indeed, using the profile name was what I had in mind. Saving something else
> in the preferences wouldn't be a real improvement.

You mean the random directory path? Because the profile name itself is always 'default', by, err, default...
Also, using that path/ID will mean that this will invalidate the search engine pref/hash when using Firefox Reset, or when you move profile data to a different machine... which might be OK, but is something we should keep in mind.
I mean the name of the folder that contains the profile.

When I move a profile from a machine to another I usually keep the whole folder, so the name would be kept. I guess others could just take the content of the folder, in which case yes, we would lose the search engine tweaks that the user made.

Not sure about the expected behavior when using Firefox Reset. Thanks for pointing it out.
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> - No longer sync the selected search engine.

Hrm, that would be a sad side-effect, as it's pretty likely that if I set DuckDuckGo as my primary search engine, I'd like to have that on all devices.
Attached patch WIP (obsolete) — Splinter Review
This patch implements the changes proposed in comment 3 and seems to work, but I need to look closer at the rest of the code to check that I'm not breaking things, and think about testing.
Challenging project, good progress.

Would it make sense to attempt to enumerate all the downstream user experiences that currently rely on search's "preffiness", to allow mitigating impact?

Among these:

- about:support, SUMO
- Reset Firefox
- FHR data collection and segmentation
- Test Pilot search related work
- profile migration (automatic and manual)
- Sync (for settings)

All of these rely in exposing *something*.  Some of them rely on having "settable" things.
From a user study I've been working on, there is some inconsistency out in the wild between the browser.search.defaultenginename value and the user's actual default set in the searchbox.

More detail: I've been conducting a user study on search hijacking specifically recruiting users who have been hijacked. The most effective way to screen hijacked users is to view the value set in about:config in browser.search.defaultenginename. However, we have received quite a few false positives where a participant's value for browser.search.defaultenginename will be a hijacking search engine, but their default searchbox is set to google. I've followed up with some participants to ask them if they recall switching back to google as their search engine (or whatever their default) and no one can recall how it happened.

I'm not sure how this setting mismatch happens in practice. Perhaps an anti-virus program?

I will attach a screenshot of an instance of this mismatch.
(In reply to Bill Selman from comment #12)

If the hijacking happened a long time ago (before Firefox 24), the mismatch could be something that has been fixed by bug 860560.
Attached patch Patch v2 (obsolete) — Splinter Review
I'm unsure about how to test this. I would need to re-init the search service after writing to the profile folder in the test. The only idea I have for now is to add a wrappedJSObject getter on the service and use it to reset some internal values so that the initialization code can run again. I'm not really happy with this idea though.
Attachment #8448081 - Attachment is obsolete: true
Attachment #8451756 - Flags: review?(MattN+bmo)
Attachment #8451756 - Flags: feedback?(dolske)
Iteration: 33.2 → 33.3
(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> Created attachment 8451756 [details] [diff] [review]
> Patch v2
> 
> I'm unsure about how to test this. I would need to re-init the search
> service after writing to the profile folder in the test.

A slightly cleaner path than wrappedJSObject may be to use the fact ss is an nsIObserver and call .observe() with your "test-do-something" topic, and there you might do whatever internal work you need. This would restrict the internals exposure to specific actions you need.
hm, and well, if you just put each single test in a separate xpcshell-test there should not even be a problem, just do whatever setup you need before initing ss.
Our current blocklisting system allows us to revert preference changes when a blocked add-on is detected. That currently allows us to reset the selected search engine for add-ons which change it, but that will no longer be possible after these change.

We need to either add a new mechanism to reset search changes when malicious add-ons are detected, or magically reset it when we receive a request to reset 'browser.search.defaultenginename'. I'm weakly in favor of the latter approach.
We'll have to address this in bug 1031441
Blocks: 1031441
Comment on attachment 8451756 [details] [diff] [review]
Patch v2

Review of attachment 8451756 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. It seems like this may makes it harder for us to change the default search engine for profiles which haven't explicitly indicated a preference (but I haven't fully thought through it). I'm not sure if this is how we would do such a migration if needed anyways. (i.e. only change new profiles or also change existing profiles with no user-set selectedEngine).

This definitely needs a test before landing and Marco's suggestion seems reasonable.
Attachment #8451756 - Flags: feedback+
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #20)
> Comment on attachment 8451756 [details] [diff] [review]

> this may makes it harder for us to change
> the default search engine for profiles which haven't explicitly indicated a
> preference

I don't think it will. If there's no user-set search engine in the profile, after the patch we still use the value of the browser.search.defaultenginename preference as the default search engine.
Comment on attachment 8451756 [details] [diff] [review]
Patch v2

I like Marco's suggestion in comment 16.
Attachment #8451756 - Flags: feedback?(dolske)
Comment on attachment 8451756 [details] [diff] [review]
Patch v2

Review of attachment 8451756 [details] [diff] [review]:
-----------------------------------------------------------------

I can't help but think we have too much logic differing between defaultEngine and currentEngine which should be the same at all times. I'm not sure why the code wasn't simplified more when the two were made to be the same. Are there cases where they aren't the same? i.e. Can't defaultEngine just be an alias for currentEngine?

For posterity, why was the metadata service chosen for this (with the fake global engine) rather than putting this in search.json which already supports global properties and I believe already exists for all users? I realize it's more of a cache but it seems to fit this use. This patch may be adding extra I/O for a large proportion of our users since search-metadata.json currently doesn't exist for users who haven't customized their search engines AFAIK.

r- for lack of tests (which I figured would come before my review).

::: browser/components/nsBrowserGlue.js
@@ +1615,5 @@
>      }
>  
> +    if (currentUIVersion < 23) {
> +      const kSelectedEnginePref = "browser.search.selectedEngine";
> +      if (Services.prefs.prefHasUserValue(kSelectedEnginePref)) {

If I'm not mistaken, this didn't create the hash upon migration if the engine is the default. This means that if I have the original default engine of "Google" as my current engine, I will still be susceptible to hijacking after this patch.

I think we may want to be more defensive for this migration to ensure malware doesn't trigger it again and cause the currentEngine to be overwritten by modifying the migration version in prefs.js to a value lower than 23. I verified that this hijacking attempt is possible with this patch.

::: toolkit/components/search/nsSearchService.js
@@ +3697,5 @@
> +    // Data is an array of bytes.
> +    let data = converter.convertToByteArray(str, {});
> +    let hasher = Cc["@mozilla.org/security/hash;1"]
> +                   .createInstance(Ci.nsICryptoHash);
> +    hasher.init(hasher.SHA256);

Nit: Ci.nsICryptoHash.SHA256 is a little more clear/standard IMO.

@@ +4057,5 @@
>      if (!this._currentEngine) {
> +      let name = engineMetadataService.getGlobalAttr("current");
> +      if (engineMetadataService.getGlobalAttr("hash") == this._getVerificationHash(name)) {
> +        this._currentEngine = this.getEngineByName(name);
> +      }

I think an else block here that logs a message when the hash doesn't match would be useful for debugging tests and with bug reports.

@@ -4169,5 @@
>    _addObservers: function SRCH_SVC_addObservers() {
>      Services.obs.addObserver(this, SEARCH_ENGINE_TOPIC, false);
>      Services.obs.addObserver(this, QUIT_APPLICATION_TOPIC, false);
>      Services.prefs.addObserver(BROWSER_SEARCH_PREF + "defaultenginename", this, false);
> -    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "selectedEngine", this, false);

This will break extensions who set the selectedEngine from their code instead of setting it through the service. If we're fine with that (assuming I'm correct), why don't we also remove the defaultenginename pref observer too to simplify the code?
Attachment #8451756 - Flags: review?(MattN+bmo) → review-
(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #23)
> I can't help but think we have too much logic differing between
> defaultEngine and currentEngine which should be the same at all times. I'm
> not sure why the code wasn't simplified more when the two were made to be
> the same. Are there cases where they aren't the same? i.e. Can't
> defaultEngine just be an alias for currentEngine?

There's history in bug 738818 and bug 860560. I think the short answer is "API compatibility concerns" (both for other apps using the search service and add-ons). Not a great choice in retrospect but not worth revisiting now unfortunately :( 

> For posterity, why was the metadata service chosen for this (with the fake
> global engine) rather than putting this in search.json which already
> supports global properties and I believe already exists for all users? I
> realize it's more of a cache but it seems to fit this use. This patch may be
> adding extra I/O for a large proportion of our users since
> search-metadata.json currently doesn't exist for users who haven't
> customized their search engines AFAIK.

search.json is a cache, and it gets blown away in various cases (e.g. when build ID changes). The selected engine can't get blown away. Don't see how we'd resolve that conflict without larger changes.

> I think an else block here that logs a message when the hash doesn't match
> would be useful for debugging tests and with bug reports.

Telemetry!
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #24)
> (In reply to Matthew N. [:MattN] (behind on bugmail) from comment #23)
> > I can't help but think we have too much logic differing between
> > defaultEngine and currentEngine which should be the same at all times. I'm
> > not sure why the code wasn't simplified more when the two were made to be
> > the same. Are there cases where they aren't the same? i.e. Can't
> > defaultEngine just be an alias for currentEngine?
> 
> There's history in bug 738818 and bug 860560. I think the short answer is
> "API compatibility concerns" (both for other apps using the search service
> and add-ons). Not a great choice in retrospect but not worth revisiting now
> unfortunately :( 

I skimmed them quickly the other day but didn't see an answer. I guess bug 738818 comment 105 is one.

> > For posterity, why was the metadata service chosen for this (with the fake
> > global engine) rather than putting this in search.json which already
> > supports global properties and I believe already exists for all users? I
> > realize it's more of a cache but it seems to fit this use. This patch may be
> > adding extra I/O for a large proportion of our users since
> > search-metadata.json currently doesn't exist for users who haven't
> > customized their search engines AFAIK.
> 
> search.json is a cache, and it gets blown away in various cases (e.g. when
> build ID changes). The selected engine can't get blown away. Don't see how
> we'd resolve that conflict without larger changes.

I was hoping that in the cases where it gets blown away we had already read the value into memory and could write it out again while recreating search.json.

> > I think an else block here that logs a message when the hash doesn't match
> > would be useful for debugging tests and with bug reports.
> 
> Telemetry!

Good idea.
Iteration: 33.3 → 34.1
Attached patch Patch v3 (obsolete) — Splinter Review
Now with a test, following the approach in comment 16. Bug 1018240 has conveniently added a way to re-init the search service from an observer notification since I wrote attachment 8451756 [details] [diff] [review].

I removed the defaultenginename pref observer, as suggested in comment 23, but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that breaks browser/base/content/test/general/browser_aboutHome.js so either we should revert this change, or I'll look into fixing that test.

I'm not completely sure what the expected use case of Services.search.defaultEngine is at this point; it looks like we could as well remove it. But that's probably for another bug.
Attachment #8451756 - Attachment is obsolete: true
Attachment #8461223 - Flags: feedback?(MattN+bmo)
QA Contact: camelia.badau
Comment on attachment 8461223 [details] [diff] [review]
Patch v3

Review of attachment 8461223 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> I removed the defaultenginename pref observer, as suggested in comment 23,
> but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> breaks browser/base/content/test/general/browser_aboutHome.js so either we
> should revert this change, or I'll look into fixing that test.

I suspect the test should change but without knowing what it's doing it's hard to say.

::: toolkit/components/search/nsSearchService.js
@@ +3199,5 @@
>      this.__sortedEngines = null;
> +    this._currentEngine = null;
> +    this._defaultEngine = null;
> +
> +    // Clear the meta data service.

Nit: "metadata"

::: toolkit/components/search/tests/xpcshell/test_prefSync.js
@@ -83,5 @@
> -
> -  // Test that setting the currentEngine to the original default engine clears
> -  // the selectedEngine pref, rather than setting it. To do this we need to
> -  // set the value of defaultenginename on the default branch.
> -  let defaultBranch = Services.prefs.getDefaultBranch("");

A test like this should be preserved (in test_selectedEngine.js) to make sure we don't regress my concern about changing the default in the future.

::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
@@ +44,5 @@
> +function asyncReInit() {
> +  let promise = waitForNotification("reinit-complete");
> +
> +  Services.search.QueryInterface(Ci.nsIObserver)
> +          .observe(null, "nsPref:changed", "general.useragent.locale");

I don't think this is used.

@@ +93,5 @@
> +  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
> +});
> +
> +// An engine set without a valid hash should be ignored.
> +add_task(function* test_persistAcrossRestarts() {

duplicate/inaccurate function name

@@ +116,5 @@
> +  do_check_eq(Services.search.currentEngine.name, getDefaultEngineName());
> +});
> +
> +
> +function run_test() {

A sanity check that the hash is truthy wouldn't hurt. Perhaps testing the the hash function (verifying it takes the profile path salt into account) would be good.
Attachment #8461223 - Flags: feedback?(MattN+bmo) → feedback+
Comment on attachment 8461223 [details] [diff] [review]
Patch v3

Review of attachment 8461223 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
@@ +44,5 @@
> +function asyncReInit() {
> +  let promise = waitForNotification("reinit-complete");
> +
> +  Services.search.QueryInterface(Ci.nsIObserver)
> +          .observe(null, "nsPref:changed", "general.useragent.locale");

It is since http://hg.mozilla.org/mozilla-central/rev/0234ad055a7b (or I don't understand your comment).
(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> Comment on attachment 8461223 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8461223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/search/tests/xpcshell/test_selectedEngine.js
> @@ +44,5 @@
> > +function asyncReInit() {
> > +  let promise = waitForNotification("reinit-complete");
> > +
> > +  Services.search.QueryInterface(Ci.nsIObserver)
> > +          .observe(null, "nsPref:changed", "general.useragent.locale");
> 
> It is since http://hg.mozilla.org/mozilla-central/rev/0234ad055a7b (or I
> don't understand your comment).

Ah, sorry, I misread this as adding a listener. Nevermind this. Note bug 1043627 though.
(In reply to Bill Selman from comment #12) 
> More detail: I've been conducting a user study on search hijacking
> specifically recruiting users who have been hijacked. The most effective way
> to screen hijacked users is to view the value set in about:config in
> browser.search.defaultenginename. However, we have received quite a few
> false positives where a participant's value for
> browser.search.defaultenginename will be a hijacking search engine, but
> their default searchbox is set to google. I've followed up with some
> participants to ask them if they recall switching back to google as their
> search engine (or whatever their default) and no one can recall how it
> happened.

I think the issue with this screenshot is that the value of the preference is not a valid search engine name, it's a URL. Unlike the old 'keyword.URL' preference, 'browser.search.defaultenginename' only makes sense if it contains the name of an installed search engine. If it's not valid, the search service just defaults to (in this case) Google.
I think that, since we're relying on a salted hash here, we should require that the salt include a string along the lines of:

"By modifying this file, I agree that I am doing so only within $appName itself, using official, user-driven search engine selection processes, and in a way which does not circumvent user consent. I acknowledge that any attempt to change this file from outside of $appName is a malicious act, and will be responded to accordingly."

I also think that we should try to make sure that we're implementing this in such a way that we're certain it would trigger the anti-circumvention provisions of the DMCA if modified from the outside.
Attached patch Patch v4Splinter Review
(In reply to Matthew N. [:MattN] from comment #27)

> (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > I removed the defaultenginename pref observer, as suggested in comment 23,
> > but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> > breaks browser/base/content/test/general/browser_aboutHome.js so either we
> > should revert this change, or I'll look into fixing that test.
> 
> I suspect the test should change but without knowing what it's doing it's
> hard to say.

I can't reproduce locally, so we will see what will happen on try (https://tbpl.mozilla.org/?tree=Try&rev=e69066ab8d84) with this new version of the patch, maybe someone fixed it somehow since last week.

> ::: toolkit/components/search/tests/xpcshell/test_prefSync.js
> @@ -83,5 @@
> > -
> > -  // Test that setting the currentEngine to the original default engine clears
> > -  // the selectedEngine pref, rather than setting it. To do this we need to
> > -  // set the value of defaultenginename on the default branch.
> > -  let defaultBranch = Services.prefs.getDefaultBranch("");
> 
> A test like this should be preserved (in test_selectedEngine.js) to make
> sure we don't regress my concern about changing the default in the future.

I'm glad you asked me to test this. That's how I discovered that the current engineMetadataService._commit implementation always saves the same data (the 'store' variable was kept in a closure).

I removed the aStore parameter that didn't seem used anywhere (but maybe try will tell me I was wrong on that).

> A sanity check that the hash is truthy wouldn't hurt. Perhaps testing the
> the hash function (verifying it takes the profile path salt into account)
> would be good.

I added a check for the length of the hash (which should always be the same). I don't see a good way to actually test the hash function without duplicating most of its code in the test; which would end up not testing much.


About bug 1043627, Gavin said there that he was OK with us keeping that reinitialization code pref'ed off for Firefox desktop. After looking at this again, I think removing the ifdef only in the observe method but not around the add/removeObserver calls is even easier and doesn't expose us to more risk than a pref would.
Attachment #8461223 - Attachment is obsolete: true
Attachment #8466378 - Flags: review?(MattN+bmo)
(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> Created attachment 8466378 [details] [diff] [review]
> Patch v4
> 
> (In reply to Matthew N. [:MattN] from comment #27)
> 
> > (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > I removed the defaultenginename pref observer, as suggested in comment 23,
> > > but it seems (https://tbpl.mozilla.org/?tree=Try&rev=dca69e0f09b6) that
> > > breaks browser/base/content/test/general/browser_aboutHome.js so either we
> > > should revert this change, or I'll look into fixing that test.
> > 
> > I suspect the test should change but without knowing what it's doing it's
> > hard to say.
> 
> I can't reproduce locally, so we will see what will happen on try
> (https://tbpl.mozilla.org/?tree=Try&rev=e69066ab8d84) with this new version
> of the patch, maybe someone fixed it somehow since last week.

Still not fixed :(.
Comment on attachment 8466378 [details] [diff] [review]
Patch v4

Review of attachment 8466378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me (ignoring the test issue)
Attachment #8466378 - Flags: review?(MattN+bmo) → review+
Depends on: 1048375
(In reply to Florian Quèze [:florian] [:flo] from comment #32)

> > ::: toolkit/components/search/tests/xpcshell/test_prefSync.js
> > @@ -83,5 @@
> > > -
> > > -  // Test that setting the currentEngine to the original default engine clears
> > > -  // the selectedEngine pref, rather than setting it. To do this we need to
> > > -  // set the value of defaultenginename on the default branch.
> > > -  let defaultBranch = Services.prefs.getDefaultBranch("");
> > 
> > A test like this should be preserved (in test_selectedEngine.js) to make
> > sure we don't regress my concern about changing the default in the future.
> 
> I'm glad you asked me to test this. That's how I discovered that the current
> engineMetadataService._commit implementation always saves the same data (the
> 'store' variable was kept in a closure).

Gavin asked me for details about this so I looked into it again. It only became a problem because when reinitializing the search service we call engineMetadataService.init again and that creates a new object in engineMetadataService._store.
Iteration: 34.1 → 34.2
I'm going to ask again, please do not land this without adding a disclaimer about outside modifications to the hash input. As the person who is most often responsible for dealing with violators in these cases, I can tell you that it is much more difficult to do so when it's not crystal clear that they're blatantly and knowingly in the wrong.

Adding a disclaimer to the hash input is a trivial change now and will be much more difficult once it lands.
(In reply to Kris Maglione [:kmag] from comment #31)
> I also think that we should try to make sure that we're implementing this in
> such a way that we're certain it would trigger the anti-circumvention
> provisions of the DMCA if modified from the outside.

Presumably this would require talking with legal and could block this bug for weeks so feel free to start that discussion with legal now. If this actually will help legally then I don't have an objection. If not, it's not going to do anything for malicious authors.

Gavin, what are your thoughts on comment 31 and comment 36?
Flags: needinfo?(gavin.sharp)
Adding that info about intent to the salt is a reasonable request, regardless of its possible legal implications or uses. Florian said he was going to whip up a patch.
Flags: needinfo?(gavin.sharp)
Attached patch Add a disclaimer in the salt (obsolete) — Splinter Review
Attachment #8467363 - Flags: review?(gavin.sharp)
(In reply to Matthew N. [:MattN] from comment #37)
> Presumably this would require talking with legal and could block this bug
> for weeks so feel free to start that discussion with legal now.

I don't think there's any need to block on legal, but I do think it's worth checking in case, e.g., using a cryptographic signature gives us a stronger legal position.

(In reply to Florian Quèze [:florian] [:flo] from comment #39)
> Add a disclaimer in the salt

I have one nit. I'd prefer that the disclaimer comes after the profile path, so that no-one can claim that they used a pre-computed seed rather than the disclaimer string itself.
(In reply to Kris Maglione [:kmag] from comment #40)

> I have one nit. I'd prefer that the disclaimer comes after the profile path,
> so that no-one can claim that they used a pre-computed seed rather than the
> disclaimer string itself.

Ok.
Attachment #8467363 - Attachment is obsolete: true
Attachment #8467363 - Flags: review?(gavin.sharp)
Attachment #8467375 - Flags: review?(gavin.sharp)
Comment on attachment 8467375 [details] [diff] [review]
Add a disclaimer in the salt v2

You could use fancy new template strings for this (which aren't documented that well yet, but see http://tc39wiki.calculist.org/es6/template-strings/ and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_strings), but that's not upliftable.
Attachment #8467375 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/eae149b23424
https://hg.mozilla.org/mozilla-central/rev/10f1f952ff73
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Contact: camelia.badau → petruta.rasa
This is probably not the ideal place to discuss this, but should we also store the homepage setting outside of preferences in the same way? A significant amount of our search traffic comes from about:home, and a lot of search hijackers hijack the homepage at the same time.
Sure - want to file that bug?
Verified using latest Nightly 2014-08-10 under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.9.4 all the specific points from comment 3.

It seems that now, the selected search engine is not remembered when opening another Firefox version with the same profile as on Nightly (where I changed the default search engine), even if the user is not connected to sync. Is this expected or should I file a new bug for it?

Is there any other verification needed? Thank you!
Flags: needinfo?(florian)
(In reply to Petruta Rasa [QA] [:petruta] from comment #48)

> It seems that now, the selected search engine is not remembered when opening
> another Firefox version with the same profile as on Nightly (where I changed
> the default search engine), even if the user is not connected to sync. Is
> this expected or should I file a new bug for it?

If you mean that you set the search engine using Nightly, and then use the same profile with an older Firefox and it doesn't see the change you made using Nightly, I think that's expected.
Flags: needinfo?(florian)
Thanks, marking as verified based on above comments.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
(In reply to Gregg Lind (User Research - Test Pilot) from comment #11)
> Challenging project, good progress.
> 
> Would it make sense to attempt to enumerate all the downstream user
> experiences that currently rely on search's "preffiness", to allow
> mitigating impact?
> 
> Among these:
> 
> - about:support, SUMO
> - Reset Firefox
> - FHR data collection and segmentation
> - Test Pilot search related work
> - profile migration (automatic and manual)
> - Sync (for settings)
> 
> All of these rely in exposing *something*.  Some of them rely on having
> "settable" things.

Did we file bugs to address this list? I'm thinking sync in particular; we should expect backlash if we did break this, and I thought sync was able to sync other things than prefs?
Flags: needinfo?(florian)
(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #51)
> (In reply to Gregg Lind (User Research - Test Pilot) from comment #11)

> > - about:support, SUMO
> > - Reset Firefox
> > - FHR data collection and segmentation
> > - Test Pilot search related work
> > - profile migration (automatic and manual)
> > - Sync (for settings)
> > 
> > All of these rely in exposing *something*.  Some of them rely on having
> > "settable" things.
> 
> Did we file bugs to address this list? I'm thinking sync in particular; we
> should expect backlash if we did break this, and I thought sync was able to
> sync other things than prefs?

I didn't. Gavin said in comment 2 that he might be OK with breaking sync. I think I did some limited testing and didn't notice anything broken in the other things mentioned in this list. Might be worth having people who actually understand these areas double check that everything is fine though.
Flags: needinfo?(florian)
Fixing Sync shouldn't be too hard, can you file a followup?
Flags: needinfo?(florian)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #53)
> Fixing Sync shouldn't be too hard, can you file a followup?

Filed bug 1054960.
Depends on: 1054960
Flags: needinfo?(florian)
Depends on: 1083961
Depends on: 1200714
Duplicate of this bug: 1030982
Depends on: 1424343
You need to log in before you can comment on or make changes to this bug.