SUMO feedback shows that users falling victim to "search hijacking" (keyword.url being taken over by an unwanted search engine) is a common problem. Some of these cases are triggered by aggressive malware that wants to steal search referrals, but other cases can be resolved by us simply resetting the pref to its default value once: - poorly written add-ons that legitimately change the pref, but programmatically such that removing the add-on doesn't revert the change - sketchy software installations might do a one-time change of the pref value in the user's profile - users may have unintentionally change the pref value, or may have been encouraged to modify it by a "tweaks" site, without realizing that this leaves them susceptible to brokenness in the future (google.com/firefox is no longer being maintained, and might go away at some point) There are two cases we can detect: - pref is changed, and the hostname is different (e.g. switched from the default of Google to SuperAwesomeWebDealsSearch.com) - pref is changed, and only parameters are different (e.g. user customized to a specific type of Google search, or referral codes where added) In either case, we could prompt the user and offer to reset the keyword.URL value to the default after they trigger a search from the location bar (with perhaps a slightly different string for each).

Yes, I agree that we should prompt the user for this, as it is most likely hijacking or something they are unaware of. * We should have a list of the values we ship (including Google, Bing, Yandex, Yahoo, etc) and not ask them if it's been changed to one of these. * We should ask after startup; if we try to ask when they are doing a search, we're less likely to get their attention, since we'll just be in the way. * It's OK to be a bit "in your face" about this, since it's a one-time question, and it won't affect most of our audience (which have the default value). Text suggestion for the dialog box: You — or software you installed — has changed the search provider in Firefox to be: evilsearch.com Is this what you wanted, or should we restore the standard search provider? [Restore Google as my search] [Keep the new search provider] The "Restore" button should be the selected and default choice. I usually don't like using modal dialog boxes for this, but in this case, I think it's warranted.

And, if Yandex or something else is our default search for that locale, show that instead of Google, of course.

How do you prevent the malware from storing that the user already has answered the question with "Keep the new search provider"? Or is the intention to annoy the user each and every startup, 50 times per day if necessarily?

(In reply to Thomas Ahlblom from comment #3) > How do you prevent the malware from storing that the user already has > answered the question with "Keep the new search provider"? We don't. We're not trying to get into a malware arms race, we're just trying to solve this problem for some number of users who got into this state accidentally, and can easily get out of it.

Gavin, I am working on getting closure on the wording of the text. What do you need from Limi (or UI perspective) to go ahead and add this in per your proposal.

Gavin, are we still planning on getting this done to check in for 13? I am assigning it to you - let me know if that's the wrong thing to do.

I finally took the time to work on this today, sorry for the delay. I think this patch is functionally complete, but I also wanted to do was telemetrize the response buttons, and I also need to write some tests. Will provide more info to help iron out the exact strings and the interaction etc. tomorrow.

So the behavior implemented by the patch is that when a search is triggered using the pref value of keyword.URL, and keyword.url has a user-set value (i.e. was manually changed by the user or by a third party program/addon) we run the following steps: 1) If there is a notification currently being displayed, or if the user has previously said "no thanks", do nothing 2) if the keyword.URL value's "base domain" (e.g. "google.com" for "www.google.com" or "search.google.com") matches the base domain of the default search plugin, do nothing. This means that users who have a custom google.com keyword.URL value won't see this prompt. 3) Show a notification bar that says: "Firefox is using 'evilsearch.com' for searches from the location bar. Would you like to use Google instead?" where "evilsearch.com" is the hostname extracted from the custom keyword.URL value, and "Google" is the name of the default engine (which we use for keyword searches by default). The button options are "Yes, use Google" or "No, continue using evilsearch.com". 4) If the user selects "Yes, use Google", we: a) reset keyword.URL b) if they are currently viewing a page whose base domain matches the base domain of keyword.URL, then we navigate the current tab to the Google search form (essentially google.com) 5) If the user selects "No", we record their decision (to avoid reprompting in the future), and do nothing. The notification bar persists for three page loads, to avoid redirects from loading keyword.URL causing it to disappear prematurely. The notification bar has no close button. There is a mechanism in place to easily allow us to force the notification to be re-shown, even if it has been previously dismissed. We might consider doing this if we later decide to re-word the prompt, or something like that.

So, things I'm looking for feedback on from UX: - The notification/button strings - behavior of 1), 2) and 4b) from comment 8

UX guys, can we get some answers to Gavin's questions? Thanks.

I think there may need to be some indication that Google is the default and some wording for when the search engine changes. "Firefox is using "evilsearch.com" for searches from the location bar would you like to restore the default search? [Yes, use Google] [No, use evilsearch.com]" And if we detect that the search was subsequently changed to newsearch.com: "Something has changed Firefox to use "newsearch.com" for searches from the location bar. Would you like to restore "evilsearch.com"? [Yes, use evilsearch.com][No, use newsearch.com]

Comment on attachment 600146 [details] screenshot of notification Looks good to me.

I'm a tad concerned about the mismatch between ShortName and the domain, in particular if malicious attackers would try to spoof google or something. Apart from that, we're in a "oh my f'ing god" situation, and the UI is in a separate notification box, so I'm less concerned about string changes. We should alert the l10n newsgroup, though.

bz, can you review the docshell change?

(In reply to Axel Hecht [:Pike] from comment #13) > I'm a tad concerned about the mismatch between ShortName and the domain, in > particular if malicious attackers would try to spoof google or something. You mean by setting the pref to a domain that "looks like" google.com, that they control? That seems unlikely to be much of a problem in practice - Google-squatting probably isn't that commonly used for real search hijacking, and even if someone is using "g0ogle.com", the fact that we're prompting at all should raise enough doubt to have at least a decent chance of users clicking "Yes, use Google". We don't need to be perfect. > Apart from that, we're in a "oh my f'ing god" situation, and the UI is in a > separate notification box, so I'm less concerned about string changes. We > should alert the l10n newsgroup, though. I'm not sure what you mean by this - I wasn't planning to land this on Aurora/Beta, if that's what you're thinking?

> 2) if the keyword.URL value's "base domain" (e.g. "google.com" for > "www.google.com" or "search.google.com") matches the base domain of the default > search plugin, do nothing. This means that users who have a custom google.com > keyword.URL value won't see this prompt. Sounds reasonable. Google will close their open redirects if they become a problem ;) > The notification bar persists for three page loads, to avoid redirects from > loading keyword.URL causing it to disappear prematurely. The notification > bar has no close button. The notification bar should go away iff the user has interacted with the page before leaving. Persisting based on the number of redirects isn't ideal for usability (it persists after leaving a legitimate search page) or for security (attackers can insert additional redirects through data: URLs). > browser.promptedToResetKeywordURL Maybe call this browser.browserPromptedToResetKeywordURL to make it extra clear that an add-on or piece of third-party software that tweaks this value is lying. Also, let's add this pref name to the AMO pre-review scanner.

I think it's great that the prompt will only be shown to users who actually use the keyword.URL feature. Of users who have a modified keyword.URL, how many also have a strange engine (or Google with a strange affiliate tag) selected in their search bar?

Comment on attachment 602185 [details] [diff] [review] patch r=me on the docshell bits

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15) > (In reply to Axel Hecht [:Pike] from comment #13) > > > Apart from that, we're in a "oh my f'ing god" situation, and the UI is in a > > separate notification box, so I'm less concerned about string changes. We > > should alert the l10n newsgroup, though. > > I'm not sure what you mean by this - I wasn't planning to land this on > Aurora/Beta, if that's what you're thinking? Oops, sorry. Sheila alerted me in mail, and I thought that had to be string freeze break, but it was re 13, so obviously not. Maybe I should read before I write :-).

(In reply to Jesse Ruderman from comment #16) > The notification bar should go away iff the user has interacted with the > page before leaving. This is rather complicated to implement (there's no good way to determine "interacted with the page"). > Persisting based on the number of redirects isn't > ideal for usability (it persists after leaving a legitimate search page) This isn't a big deal - in theory users may still want to change their mind even after interacting with the "EvilSearch" site. > for security (attackers can insert additional redirects through data: URLs). > Also, let's add this pref name to the AMO pre-review scanner. I'm not too worried about people explicitly trying to get around this prompt - I don't really want to get into an arms race, the main goal here is to provide relief against slightly sleazy/confusing actors, not malware. (In reply to Jesse Ruderman from comment #17) > Of users who have a modified keyword.URL, how many also have a strange > engine (or Google with a strange affiliate tag) selected in their search bar? We could go further with this prompt and have it reset the default search engine, as well. I'd like to try and evaluate that in a followup (we should get more telemetry for search bar stuff).

(In reply to [:Cww] from comment #11) > I think there may need to be some indication that Google is the default and > some wording for when the search engine changes. This is a good suggestion. > And if we detect that the search was subsequently changed to newsearch.com: Distinguishing initial change from subsequent changes is more complicated, and I'm not sure there's much additional benefit (hopefully users aren't suffering from constant changes, but even if they are the generic message seems suitable).

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20) > (In reply to Jesse Ruderman from comment #16) > > The notification bar should go away iff the user has interacted with the > > page before leaving. > > This is rather complicated to implement (there's no good way to determine > "interacted with the page"). Can you piggyback on the popup blocker heuristic?

Yes, sorry Axel I just wanted to let you know something was coming down the pipe. This is on the trunk and localizers will pick it up like all the other changes. I was just letting you know, well..to be nice :-). Apologies if it created confusion.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21) > Distinguishing initial change from subsequent changes is more complicated, > and I'm not sure there's much additional benefit (hopefully users aren't > suffering from constant changes, but even if they are the generic message > seems suitable). How about storing the user accepted user engine in the newly introduced browser.promptedToResetKeywordURL option (and possibly rename it to something like browser.userAcceptetKeywordURLChange) and check against that if it exist.

Another idea, it could also have some hash tacked on to it to make it harder to spoof.

Comment on attachment 602185 [details] [diff] [review] patch Frank, can you review the browser/ parts of this?

Comment on attachment 602185 [details] [diff] [review] patch Review of attachment 602185 [details] [diff] [review]: ----------------------------------------------------------------- It looks good. :) Just a few small questions/notes: ::: browser/modules/KeywordURLResetPrompter.jsm @@ +36,5 @@ > + let defaultURI; > + try { > + defaultURI = defaultEngine.getSubmission("foo").uri; > + } catch (ex) { > + // If this somehow fails, our defaults are broken... Why are we worried about the defaults being broken? We don't check for this elsewhere, e.g. https://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#878 @@ +46,5 @@ > + let keywordBaseDomain; > + try { > + keywordBaseDomain = Services.eTLD.getBaseDomain(keywordURI); > + if (keywordBaseDomain == Services.eTLD.getBaseDomain(defaultURI)) > + return; Setting keyword.URL to something like http://www.google.com/search?q=site:evil.com+ slips through this, but that seems like an edge case. @@ +82,5 @@ > + [keywordBaseDomain]), > + accessKey: browserBundle.getString("keywordPrompt.noButton.accessKey"), > + popup: null, > + callback: function(aNotificationBar, aButton) { > + Services.prefs.setIntPref("browser.promptedToResetKeywordURL", KEYWORD_PROMPT_REV); If the user selects no, and later keyword.URL gets hijacked again to a different value, the user will not be prompted. Are we okay with that?

(In reply to Frank Yan (:fryn) from comment #28) > Why are we worried about the defaults being broken? In theory it's possible for a search engine to mess with the return value of originalDefaultEngine getter by changing the defaultenginename pref (I've seen some toolbars that do this). Ideally we want to retrieve the build's actual original default engine, so we might need to make some adjustments for that. > Setting keyword.URL to something like http://www.google.com/search > ?q=site:evil.com+ slips through this, but that seems like an edge case. Yeah, I suspect that's pretty uncommon. Adding in/switching Google referral parameters may be more likely, but that's less user-hostile, so not something we should worry about now, I think. > If the user selects no, and later keyword.URL gets hijacked again to a > different value, the user will not be prompted. Are we okay with that? Yeah, this is related to khagaroth's suggestion. I'm not sure that we need to address this in this revision.

Comment on attachment 602185 [details] [diff] [review] patch (In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29) > In theory it's possible for a search engine to mess with the return value of > originalDefaultEngine getter by changing the defaultenginename pref (I've > seen some toolbars that do this). Ideally we want to retrieve the build's > actual original default engine, so we might need to make some adjustments > for that. It'd be great if originalDefaultEngine were more resilient to being hijacked, but that's for another bug. Also, third-party code can now mess with the pref browser.promptedToResetKeywordURL, but I suppose we could blacklist them for doing things like that if necessary. Thanks for the answers. :)

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29) > Ideally we want to retrieve the build's > actual original default engine, so we might need to make some adjustments > for that. There should be ways to read the default branch of prefs (i.e. the ones the build has, and not the state after user/profile modifications), right? We should be able to get the original default from there.

Gavin, how are we doing on this. Anything else we need before checking this in?

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #31) > There should be ways to read the default branch of prefs (i.e. the ones the > build has, and not the state after user/profile modifications), right? We > should be able to get the original default from there. Yes, I am aware of the existence of the default pref branch :) The current originalDefaultEngine API doesn't use it, and reworking it to use it involves larger changes that I'm not going to tackle in this bug.

A few minor tweaks: - "Would you like to use Google instead?" -> "Would you like to restore the default search (Google)?" per Cheng's comment - tweaked the pref name from browser.promptedToResetKeywordURL to browser.keywordURLPromptDeclined - worked around the originalDefaultEngine bug by just reading the pref myself - added a !keywordBaseDomain check to better handle failure cases with getBaseDomain

The reset button says "Yes, use Google". So it should also load the Google search result page for the users query.

(In reply to Siddhartha Dugar [:sdrocking] from comment #37) > The reset button says "Yes, use Google". So it should also load the Google > search result page for the users query. As mentioned in comment 8, we do load the engine's "searchform" URL if you click "Yes" and the base domain of the current page matches the base domain of the keyword.URL value. Extracting the users query from the keyword URI post-fixup is more involved, and it wasn't something I wanted to get into as part of the first revision.

I filed bug bug 738804 to back this patch out while we investigate additional changes to simplify our search prefs.

No longer tracking this for Firefox 13, since it was backed out in bug 738804.

It's been five months since this patch was backed out so that investigation could happen. What is the result of that investigation? 50% of release users have the keyword.url pref set to something other than default which can only be done by editing about:config or by an external application. I think it's safe to assume these are being changed by external applications like software and add-on installs. Historically this is the largest issue reported to support. I think we should land this existing patch and then iterate on it unless there was some new insight that was learned in the last five months.

Hey Michael, Bug 738818 is tracking the larger goal that we identified when we first looked into this. No real progress has been made there since we haven't dedicated any resources to tackling this problem specifically. Sounds like you want to push for that, and maybe have us revisit a shorter-term fix like the one we originally landed here. I agree with you that we should do something, but this bug isn't the best place to track it - can you file another one, and CC me?

Per the discussion in bug 784189 and elsewhere I've gone ahead and unbitrotted this and landed it. https://hg.mozilla.org/integration/mozilla-inbound/rev/07ee3508f530

I'm glancing at this patch, and wonder: If a user rejected to get his default search reset, 'cause they like it, and then get hijacked to a different keyword.URL, should we unset browser.keywordURLPromptDeclined? We wouldn't be able to reset them to what they initially wanted, but still get them off `evilsearch.com`

(In reply to Axel Hecht [:Pike] from comment #46) > If a user rejected to get his default search reset, 'cause they like it, and > then get hijacked to a different keyword.URL, should we unset > browser.keywordURLPromptDeclined? That scenario was considered unlikely enough that I didn't bother handling it. We can "bump" the keywordURLPromptDeclined pref at some later point to re-prompt users who had previously dismissed it, though.

keywordPrompt.yesButton = Yes, use %1$S keywordPrompt.noButton = No, continue using '%1$S' Not sure about consistency here (one with quotes, the other one without, used in the same context).

(In reply to Francesco Lodolo [:flod] from comment #49) > keywordPrompt.yesButton = Yes, use %1$S > keywordPrompt.noButton = No, continue using '%1$S' > > Not sure about consistency here (one with quotes, the other one without, > used in the same context). Since the argument in the yesButton string is the default provider name, I decided quotes were unnecessary (we won't ship a default provider whose name is confusing). The argument in the noButton string is a host name from the user's prefs, so I couldn't really rely on it being something that necessarily made sense.

I woud suggest giving the "never ask me again(for this domain)" option regardless if whether they choose yes or no. 2 of the worst, well intentioned, culprits are mcafee and avg with there own safe-search alternatives and if I recall correctly,they both reset the value repeatedly(on startup/windows startup/virus updates or something)

That's a good idea - do you want to file a new bug about that, and CC me? I can file it if you don't want to.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #52) > That's a good idea - do you want to file a new bug about that, and CC me? I > can file it if you don't want to. Yes, please file it(and cc me)

Hello everyone, How can I test this feature? Is there a blacklist/whitelist with addons related to this ? I'm not seeing any reset notification by installing for eg. yandex bar (http://bar.yandex.ru) or manually changing the keyword.URL to evilsearch.com.

(In reply to Paul Silaghi [QA] from comment #54) > How can I test this feature? > Is there a blacklist/whitelist with addons related to this ? > I'm not seeing any reset notification by installing for eg. yandex bar > (http://bar.yandex.ru) or manually changing the keyword.URL to > evilsearch.com. There is no blacklist/whitelist. Manually changing keyword.URL and then actually performing a keyword search (e.g. entering "foo bar" in the location bar and pressing enter) should result in the notification appearing.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #55) > Manually changing keyword.URL and then > actually performing a keyword search (e.g. entering "foo bar" in the > location bar and pressing enter) should result in the notification appearing. This looks good. But I've noticed something else: 1. Installing http://us.toolbar.yahoo.com/ and check "Set Yahoo! as default home page and default search" installs the toolbar but without setting the homepage and the default search engine. Also no sign of reset notification. This works fine on a nightly before the fix. 2. Installing http://bar.yandex.com/ is setting the homepage and the default search engine, but still no reset notification appears Are this possible related bugs?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #55) > Manually changing keyword.URL and then > actually performing a keyword search (e.g. entering "foo bar" in the > location bar and pressing enter) should result in the notification appearing. 3. Also no sign of reset notification if keyword.URL is set to an invalid link (e.g. "xxxxx")

(In reply to Paul Silaghi [QA] from comment #56) > 1. Installing http://us.toolbar.yahoo.com/ and check "Set Yahoo! as default > home page and default search" installs the toolbar but without setting the > homepage and the default search engine. Also no sign of reset notification. > This works fine on a nightly before the fix. What do you mean by "without setting the default search engine"? This patch should not have affected what happens when you install Yahoo toolbar, so if you're seeing something like that, please file a new bug. > 2. Installing http://bar.yandex.com/ is setting the homepage and the default > search engine, but still no reset notification appears Does the yandex toolbar change keyword.URL? That's the only situation that this patch covers.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #58) > What do you mean by "without setting the default search engine"? This patch > should not have affected what happens when you install Yahoo toolbar, so if > you're seeing something like that, please file a new bug. bug 825239 filed > Does the yandex toolbar change keyword.URL? That's the only situation that > this patch covers. Indeed keyword.url is not changed, so it's not a bug.

After doing exploratory testing around this feature, I've found the next issue which concern me: 1) After manually changing the pref with "http://www.google.com" or "http://www.search.google.com" , the notification appears, when it shouldn't, in my opinion. Is this behavior expected? 2) After manually changing the pref with something like "evil.com" or" www.evil.com ", without "http://" , or with something invalid, the notification doesn't appear. I am not sure, is this behavior expected?

(disabled for Firefox 19 in bug 838864)

We've decided to abandon this plan in favor of bug 738818. Disabled for Firefox 21 and 22 in bug 838864.