470876 - anti-phishing support not in fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

If I visit: 
http://www.mozilla.com/firefox/its-a-trap.html

I do not get an anti-phishing warning message.  I think this is scheduled for a later time, but we don't seem to have any bugs filed to indicate this.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I think we're going to have a hard time with the memory requirements (and maybe even disk space requirements) of the current anti-phishing/malware database, so this will probably be a bit more involved than just porting the Firefox code.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Are we not going to have this in the product?  If so, lets close this bug out.

Comment 3

[Aakash Desai [:aakashd]]

We have test cases on litmus that are disabled right now. If this is ever resolved as fixed/wontfix, then please mark the in-litmus flag as +/- and enable/delete those testcases from the fennec test run.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

anti-phishing isn't in the roadmap for 2.0

Comment 5

[Mehdi Mulani [:mmm] (I don't check this)]

FWIW, we're considering using a smaller in memory data structure in place of the the safe-browsing sqlite DB. The target memory usage is around 1.5MB.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> FWIW, we're considering using a smaller in memory data structure in place of
> the the safe-browsing sqlite DB. The target memory usage is around 1.5MB.

That would be great news!

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch adds the necessary code to activate SafeBrowsing (phishing and malware protection). I pulled nearly all of this code from Firefox, but the XPCOM code was simplified into a single JS class and not many separate JS files. We also needed to deal with multi-process issues. A brief rundown of the patch:

* Adds the MOZ_SAFE_BROWSING=1 bit to confvars so toolkit builds the support
* Adds the required prefs to mobile.js
* Adds SafeBrowsing.js XPCOM object. This object controls the toolkit URLClassifier components. It does double duty, controlling the phishing and malware activities. It simply monitors the prefs and updates the URL classifier list manager.
* Adds blockedSite.xhtml, the underlying impl for about:blocked which is used by the toolkit when we find a phishing or malware site. phishing.dtd has the strings.
* Added code to content.js and browser.js to handle interacting with the about:blocked page.
* Makes sure we add the new files to the package manifest

The code mainly comes from http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/

We will need to make about:blocked mobile-friendly, but that is a different bug.

I just realized that I should add #ifdef MOZ_SAFE_BROWSING in various places so we can turn this feature off from confvars.sh if we need to. I'll make a followup patch for that.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot

UI looks like this

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 527473 [details] [diff] [review]
patch

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js
>   /**
>+   * Handle blocked site event bubbling up from content.
>+   */

The comment seems wrong since we're not handling an event but a message.

>+  _handleBlockedSite: function _handleBlockedSite(aMessage) {
>+    let formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+    let json = aMessage.json;
>+    if (json.action == "leave") {
>+      // Get the start page from the *default* pref branch, not the user's
>+      let url = Browser.getHomePage({ useDefault: true });
>+      this.loadURI(url);
>+    } else if (json.action == "report-malware") {
>+      // Get the stop badware "why is this blocked" report url, append the current url, and go there.
>+      try {
>+        let reportURL = formatter.formatURLPref("browser.safebrowsing.malware.reportURL");
>+        reportURL += json.url;
>+        this.loadURI(reportURL);
>+      } catch (e) {
>+        Cu.reportError("Couldn't get malware report URL: " + e);
>+      }
>+    } else { // It's a phishing site, not malware
>+      try {
>+        let reportURL = formatter.formatURLPref("browser.safebrowsing.warning.infoURL");
>+        this.loadURI(reportURL);
>+      } catch (e) {
>+        Cu.reportError("Couldn't get phishing info URL: " + e);
>+      }
>+    }
>+  },

When there is more than 2 cases that are handle by a if/else checking the same condition I usually prefer to use a 'switch', but I guess it can be a matter of preference.

>diff --git a/mobile/chrome/content/content.js b/mobile/chrome/content/content.js
>--- a/mobile/chrome/content/content.js
>+++ b/mobile/chrome/content/content.js
>@@ -270,17 +270,17 @@ let Content = {
>     addMessageListener("Browser:SetCharset", this);
>     addMessageListener("Browser:ContextCommand", this);
>     addMessageListener("Browser:CanUnload", this);
> 
>     if (Util.isParentProcess())
>       addEventListener("DOMActivate", this, true);
> 
>     addEventListener("MozApplicationManifest", this, false);
>-    addEventListener("command", this, false);
>+    addEventListener("click", this, false);

Why this change? Does it mean the code won't fire if the user navigate with an hardware keyboard and hit return?


>-        else if (/^about:neterror\?e=netOffline/.test(errorDoc.documentURI)) {
>+        } else if (/^about:neterror\?e=netOffline/.test(errorDoc.documentURI)) {
>           if (ot == errorDoc.getElementById("errorTryAgain")) {
>             // Make sure we're online before attempting to load
>             Util.forceOnline();
>           }
>+        } else if (/^about:blocked/.test(errorDoc.documentURI)) {
>+          dump("---- command from blocked site\n")

Left over from debugging?

>+            if (isMalware)
>+              sendAsyncMessage("Browser:BlockedSite", { url: errorDoc.location.href, action: "report-malware" });
>+            else
>+              sendAsyncMessage("Browser:BlockedSite", { url: errorDoc.location.href, action: "report-phishing" });

Can we change the above by:

let action = isMalware ? "report-malware" : "report-phising";
sendAsyncMessage("Browser:BlockedSite", { url: errorDoc.location.href, action: action });

>+          } else if (ot == errorDoc.getElementById("ignoreWarningButton")) {
>+            // Allow users to override and continue through to the site,
>+            // but add a notify bar as a reminder, so that they don't lose
>+            // track after, e.g., tab switching.
>+            let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
>+            webNav.loadURI(content.location, Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CLASSIFIER, null, null, null);
>+            
>+            // TODO: We'll need to impl notifications in the parent process and use the preference code found here:
>+            //       http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2672
>+            //       http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/content/globalstore.js
>+          }

Nit: could you make the link point to a specific revision, just in case the code change there?

>diff --git a/mobile/installer/package-manifest.in b/mobile/installer/package-manifest.in
> ; Safe Browsing
>-@BINPATH@/components/nsSafebrowsingApplication.manifest
>-@BINPATH@/components/nsSafebrowsingApplication.js
>+;@BINPATH@/components/nsSafebrowsingApplication.manifest
>+;@BINPATH@/components/nsSafebrowsingApplication.js

Why are those components commented?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

> The comment seems wrong since we're not handling an event but a message.

Changed it and also the comment for handleCert

> When there is more than 2 cases that are handle by a if/else checking the same
> condition I usually prefer to use a 'switch', but I guess it can be a matter of
> preference.

It is nicer. Changed

> Why this change? Does it mean the code won't fire if the user navigate with an
> hardware keyboard and hit return?

Desktop made the same change. See:
http://hg.mozilla.org/mozilla-central/file/855e5cd3c884/browser/base/content/browser.js#l4759

I added the same comment to Fennec

> Left over from debugging?

Removed
 
> Can we change the above by:
> 
> let action = isMalware ? "report-malware" : "report-phising";
> sendAsyncMessage("Browser:BlockedSite", { url: errorDoc.location.href, action:
> action });

Done

> Nit: could you make the link point to a specific revision, just in case the
> code change there?

Done

> >-@BINPATH@/components/nsSafebrowsingApplication.manifest
> >-@BINPATH@/components/nsSafebrowsingApplication.js
> >+;@BINPATH@/components/nsSafebrowsingApplication.manifest
> >+;@BINPATH@/components/nsSafebrowsingApplication.js
> 
> Why are those components commented?

I removed them now. They are desktop Firefox components

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

* Made Vivien's review changes
* Added MOZ_SAFE_BROWSING checks in enough places to safely turn this feature off if we need to

Comment 12

[Doug Turner (:dougt)]

this is going to cost 10s of mbs of space and probably lot of ram too.  How much does this cost -- both for first run and for a few days/weeks of deltas?

I think there is a better approach -- using a filter and network lookup on failure.  that is what we should build, i think.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)
> this is going to cost 10s of mbs of space and probably lot of ram too.  How
> much does this cost -- both for first run and for a few days/weeks of deltas?
> 
> I think there is a better approach -- using a filter and network lookup on
> failure.  that is what we should build, i think.

I am totally cool with someone re-writing the back end to do something different. The front-end would likely be the same.

What bug are we using to track the new fangled approach? Who is assigned?

Yes, we need to get moving on whatever platform changes need to happen ASAP.

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #12)
> I think there is a better approach -- using a filter and network lookup on
> failure.  that is what we should build, i think.

Chromium has also been optimizing for the mobile use case and this is precisely their current approach. (it might be worth it to try using their implementation if you're not tied to any APIs yet)(In reply to comment #13)
They are moving to either a bloom filter or prefix set. From their notes, the bloom filter took up ~1.9M while the prefix set takes up ~1.2M for the same data.

Since the bloom filter is probabilistic, in the case of any positive they did a network lookup (which is exactly as you describe).

> (In reply to comment #13)
> I am totally cool with someone re-writing the back end to do something
> different. The front-end would likely be the same.
> 
> What bug are we using to track the new fangled approach? Who is assigned?
> 
> Yes, we need to get moving on whatever platform changes need to happen ASAP.

The effort to rewrite the back-end of safe browsing is in bug 572463 and I'm currently working on that. Our goal is to rewrite it in JS and at the same time revamp it. A tall order that might be better served by moving to chromium code for the meantime (especially as you don't have to deal with dirty profiles for mobile).

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed, but defaulted to off:
http://hg.mozilla.org/mozilla-central/rev/cc24b61149ed

the code is in place and ready to be switched on. We need better platform support for a smaller url classifier data file before we can turn this on.

I don't know if we have a good "use a bloom filter to make a smaller URL classifier data file" bug opened yet.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I would like to work with you guys on making the data storage more efficient on disk and over the network because on the I have a very similar requirement regarding certificate revocation data and we might be able to share some infrastructure.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

We'll file a bug to turn this on when the backend if smaller/faster

Comment 18

[Martijn Wargers (dead)]

(In reply to comment #15)
> pushed, but defaulted to off:
> http://hg.mozilla.org/mozilla-central/rev/cc24b61149ed
> 
> the code is in place and ready to be switched on. We need better platform
> support for a smaller url classifier data file before we can turn this on.

I guess bug 669407 was filed for this.

Comment 19

[Andreea Pod]

Going to http://www.mozilla.com/firefox/its-a-trap.html I do not get the "Reported Web Forgery!" message like on desktop, it takes me to a page telling me "It's a Trap!", is this the expected result?

Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110804 Firefox/7.0a2 Fennec/7.0a2
Device: LG Opyimus 2X

Comment 20

[Matt Brubeck (:mbrubeck)]

This code was checked in but will not actually be turned on until bug 669407 is fixed.  We'll file a separate bug to enable the feature.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

I'm now testing this, running with the patches from bug 673470 plus "MOZ_SAFE_BROWSING=1" and the feature is broken in Fennec. 

The test URLs do work, but those get their data from a (very simple) direct update in the SafeBrowsing.js script. Updating from the real Google SB servers (handling timeouts, parsing the protocol) doesn't work on Fennec. So the test URLs are really the *only* ones that work. 

From a bit of looking, I suspect some functionality is simply missing, such as the stuff in (for example) browser/components/safebrowsing/content/globalstore.js which doesn't seem to have an equivalent in /mobile.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Re-closing this old bug and moving future work to bug 741808