Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Description • 12 years ago

The initial patch in bug 470876 did not fully enable setting up the URL classifier (list manager) so we don't get access to the DB of real malware/phishing data. We only have access to the manually added test sites.

The "listManager" defined in SafeBrowsing.js :
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/components/SafeBrowsing.js#100

Probably needs to be initialized like it is here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/safebrowsing/content/globalstore.js#125

To test this, you'll need to build with MOZ_SAFE_BROWSING=1 in confvars.sh

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 1 • 12 years ago

Gian-Carlo mentioned using http://www.phishtank.com/ to get potential, real malware sites to help test.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 2 • 12 years ago

Should not enable this by default unless bug 727370 is solved, you'll get scr***d on non-rooted phones otherwise.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 3 • 12 years ago

Here's a bunch of fixes to get updates working in Native Fennec. It's missing the stuff to get the actual error screens, as I have no real idea what to do with those or how to plug them into native. (This will cause a crash when you hit a phishing/malware site). It also has all relevant logging/debugging enabled.

With this, we initialize the store correctly, do a working update with the test DB, and start updating from Google. Unfortunately right now Google gives a 503 to our first update request. I'm not seeing anything malformed, though, and according to the documentation this more or less means an overloaded server?! Debugging futher...

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 4 • 12 years ago

Soo, the problem is this:

  updateProviderURLs: function() {
#ifdef USE_HISTORIC_SAFEBROWSING_ID
    let clientID = "navclient-auto-ffox";
#else
    let clientID = Services.appinfo.name;    
#endif

This services.appinfo.name is "Fennec" in Fennec. Google doesn't have this listed as a client that is allowed to use the big database, so they refuse to send us updates for the goog-* tables (I presume the googpub-* tables would work, but of course we don't want those). If I change this to "Firefox", it will work.

dolske, do you have any suggestions where to pick a name so this would read "Firefox" in "Fennec", but presumably still do the right thing wrt SeaMonkey, Iceweasel and others?

FWIW, the .pset files for the test tables that are written out are too large according to the filesystem:
-rw-r--r-- app_46   app_46       4096 2012-08-24 18:44 test-malware-simple.pset
And this causes them to be rejected on loading. The larger (200k-800k) tables seem to work correctly. Taras, I'm suspecting something may be broken with mozilla::fallocate on Android. (Didn't verify yet, just a hunch!)

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 5 • 12 years ago

Notes from mfinkle:

http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/content.js#362
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content
/browser.js#926

needs to go into:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3926

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 6 • 12 years ago

- This defaults to preffed off. We can enable the pref if we have the UX for doing so, and after I fix bug 727370.

- Safebrowsing.jsm is the jdolske rewrite, but changed to use MOZ_APP_UA_NAME instead of Services.appinfo.name. Necessary because "Fennec" isn't known to Google as a client that can use the full lists.

- Fixes our doorhanger code to display correctly when a doorhanger has no buttons.

- Fixes the display of about:blocked in several areas. Allow the user to override the warning, but remind him with a doorhanger that the site is dangerous each time (s)he switches to it.

- Plumbing to make it all work.

After enabling the pref, I correctly get SafeBrowsing updates, and sites in Phishtank that are blocked on desktop correctly get the warning on mobile.

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 7 • 12 years ago

Comment on attachment 662657 [details] [diff] [review]
Patch 1. Make using the UrlClassifier possible in Fennec.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+        } else if (/^about:blocked/.test(errorDoc.documentURI)) {
>+          let secHistogram = Cc["@mozilla.org/base/telemetry;1"].
>+            getService(Ci.nsITelemetry).
>+            getHistogramById("SECURITY_UI");

nit: I try to encourage no line wrapping. 

>+          let bucketName = isMalware ? "WARNING_MALWARE_PAGE_":"WARNING_PHISHING_PAGE_";

nit: spaces around the ":"

>+            // ....but add a notify bar as a reminder, so that they don't lose
>+            // track after, e.g., tab switching.
>+            NativeWindow.doorhanger.show(Strings.browser.GetStringFromName("safeBrowsingDoorhanger"),
>+                                         "safebrowsing-warning", [],
>+                                         BrowserApp.selectedTab.id);

nit: line wrapping
also, does this doorhnager never go away unless the user dismisses it? I would have thought you needed to pass { persistence: -1 } in for the options.

>diff --git a/mobile/android/chrome/jar.mn b/mobile/android/chrome/jar.mn

>+  content/blockedSite.xhtml            (content/blockedSite.xhtml)
> * content/aboutApps.xhtml              (content/aboutApps.xhtml)
>   content/aboutApps.js                 (content/aboutApps.js)
>   content/blockedSite.xhtml            (content/blockedSite.xhtml)

it's already in here. no need to re-add it.

>diff --git a/mobile/android/components/SafeBrowsing.jsm b/mobile/android/components/SafeBrowsing.jsm

I assume we'll just use the toolkit version of this file at some point?

r+, but fix the nits

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 8 • 12 years ago

>also, does this doorhnager never go away unless the user dismisses it? I would 
>have thought you needed to pass { persistence: -1 } in for the options.

Persistence:-1 lets it persist even when other pages are loaded. The default makes the doorhanger go away whenever the user loads a new page, but will pop it back if (s)he only switches tabs. This is what we want: if the new page is unsafe too, we'll pop up a new warning. If the new page is safe, there's no need to keep the warning around.

>I assume we'll just use the toolkit version of this file at some point?

Bug 778609 which is blocked on bug 778608, which is stalled due to unexplainable test failures.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 9 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2cd2911c28

Let's keep it open and add a second patch to actually enable once the dependent bugs are fixed.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 10 • 12 years ago

The 2 second delay added in bug 778855 isn't enough to prevent a startup regression on mobile:

Regression  Ts increase 5.53% on Android 2.2 (Native) Mozilla-Inbound
-----------------------------------------------------------------------
    Previous: avg 3127.200 stddev 66.728 of 30 runs up to revision 85d6cbd01d39
    New     : avg 3300.052 stddev 32.591 of 5 runs since revision fe5b75aa2b16
    Change  : +172.852 (5.53% / z=2.590)
    Graph   : http://mzl.la/PC4s2o

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 11 • 12 years ago

(In reply to Gian-Carlo Pascutto (:gcp) from comment #10)
> The 2 second delay added in bug 778855 isn't enough to prevent a startup
> regression on mobile:

In XUL Fennec, we would also wait for the initial page to load before starting a delayed loading of some components. Might be worth trying that. That means the very first page loaded might not be checked for phishing/malware though.

	Ed Morley [:emorley]
	Comment 12 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4a2cd2911c28

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 13 • 12 years ago

(In reply to Mark Finkle (:mfinkle) from comment #11)
> That means the very first page loaded might not be checked for
> phishing/malware though.

I would imagine that a lot (or most) of the phishing links people click on will be coming from other apps like email/twitter. That means if FF isn't already running, that will be the first page loaded and so missing the check on that seems like a bad idea.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 14 • 12 years ago

>That means the very first page loaded might not be checked for phishing/malware 
>though.

Bug 672284.

jdolske put the suspicion for the slowdown to the initial update we do to initialize the test tables (which causes disk IO). Delaying that update doesn't compromise our security, and there might be ways to do that faster or get rid of it in many situations, anyway.

If a significant part of the slowdown is just due to the extra JS loading, I don't think we can avoid taking the hit, though.

I might just bump it from 2 to 5 seconds to get rid of the seeming regression here? There's already bugs in place for all the other approaches.

	Justin Dolske [:Dolske]
	Comment 15 • 12 years ago

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> Soo, the problem is this:
> 
>   updateProviderURLs: function() {
> #ifdef USE_HISTORIC_SAFEBROWSING_ID
>     let clientID = "navclient-auto-ffox";
> #else
>     let clientID = Services.appinfo.name;    
> #endif
> 
> This services.appinfo.name is "Fennec" in Fennec. Google doesn't have this
> listed as a client that is allowed to use the big database, so they refuse
> to send us updates for the goog-* tables (I presume the googpub-* tables
> would work, but of course we don't want those). If I change this to
> "Firefox", it will work.

Interesting, I assumed the client got the same data no matter what this ID was. Do we know what the difference is exactly, or is it just more downloaded data?

> dolske, do you have any suggestions where to pick a name so this would read
> "Firefox" in "Fennec", but presumably still do the right thing wrt
> SeaMonkey, Iceweasel and others?

It's possible we'll need to talk with Google about making "Fennec" work.

But otherwise I'd say change the Makefile so that USE_HISTORIC_SAFEBROWSING_ID gets defined when either MOZ_PHOENIX or... uhmmm... whatever's defined when Mobile is built... is set. (Along with OFFICIAL_BUILD)

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 16 • 12 years ago

>Interesting, I assumed the client got the same data no matter what this ID was. Do 
>we know what the difference is exactly, or is it just more downloaded data?

With some IDs you won't get anything unless you use the googpub-* lists. The data is the same regardless of the client. The contents of the lists differs (differed?), due to agreements Google has with the providers of the data for the lists, see for example bug 557752.

>But otherwise I'd say change the Makefile so that USE_HISTORIC_SAFEBROWSING_ID 
>gets defined when either MOZ_PHOENIX or... uhmmm... whatever's defined when 
>Mobile is built... is set. (Along with OFFICIAL_BUILD)

After a discussion with mfinkle about the alternatives, the current code ended up using MOZ_APP_UA_NAME, which is "Firefox" in Fennec.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SafeBrowsing.jsm#93

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 17 • 12 years ago

I spent some time making and testing alternatives for the SafeBrowsing loading that didn't have the ts regression while being slightly cleaner in startup, for example by only delaying the initial "test-tables" update until after startup. (This update was our main suspect for the ts regression in bug 778855 and bug 779008)

https://tbpl.mozilla.org/?tree=Try&rev=faf78e4b17dc
https://tbpl.mozilla.org/?tree=Try&rev=e86e477abf66
https://tbpl.mozilla.org/?tree=Try&rev=96b3db2a59f4

Ts didn't improve, on the contrary.

Then I spent some time checking if the delayed-initialization didn't have any problems when Firefox is launched with a "bad" URL. I failed to bypass SafeBrowsing even when it starts with a big delay when doing this. When analyzing further, the following seems to happen:

When Firefox is started with a bad URL, and SafeBrowsing isn't initialized, the url-classifier is still called, and will see in the default prefs that it should run checks. It will instantiate Classifier, which will check which tables it has, and check against all of them. If there is a bad URL at this point, it will be flagged and a completion (to Google's server, to check for false positives) will be queued. The completion can't run because it doesn't have the server URL yet. The channel blocks as long as there is no return from the completion. At the moment the SafeBrowsing initialization runs, the URL will be set, the completion will fire, return a positive, and stop the channel. At that point, the error page (which was just set in the initialization) will be displayed.

So, this whole goo of code actually does the right thing. Who'd have thought! Note that there is no delay in this process if you do not hit a dangerous site.

Based on this, the above ts test, and the observation that postponing the initial test-tables update doesn't actually help anything, I've (only) adjusted jdolske's tweak to delay to initialization from 2 seconds to 5 seconds.

And I flipped the pref to on.

	Mark Finkle (:mfinkle) (use needinfo?) Reporter
	Comment 18 • 12 years ago

Comment on attachment 667914 [details] [diff] [review]
Patch 2. Enable SafeBrowsing.

Excellent research.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 19 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b006a1c946fd

	Ed Morley [:emorley]
	Comment 20 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/b006a1c946fd