1483342 - Add basic GeckoRuntime Safe Browsing API

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Eugen Sawin [:esawin]]

In bug 1462000 we've discussed a multi-step approach for the implementation of the safe browsing API for GeckoView, see https://bugzilla.mozilla.org/show_bug.cgi?id=1462000#c18.

As described, safe browsing is currently coincidentally enabled when tracking protection is enabled without an option to control or disable it.

We should fix this state asap, before continuing with the extended implementation of the safe browsing API.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1483342-1.0-Add-GeckoRuntime-Safe-Browsing-API.-.patch

With this patch we expose the "browser.safebrowsing.{phishing, malware}.enabled" prefs via GeckoRuntimeSettings to allow minimal control over safe browsing state.

This is an implementation for step #1 and partly #3 from https://bugzilla.mozilla.org/show_bug.cgi?id=1462000#c18.

Eventually, we might want to consolidate the tracking protection and safe browsing APIs since they share a lot of characteristics, but that will ideally require step #2 which is up for discussion yet.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1483342-2.0-Initialize-SafeBrowsing-module-by-de.patch

For now, we should initialize the SafeBrowsing module asap to make sure the initial load is properly classified.

In future, we might have to split the module's responsibilities (TP and SB) and think how it could function as a Google Play Services (step #4) fallback option.

Comment 3

[Chris Peterson [:cpeterson]]

[geckoview:klar:p2] because the Focus team wants Safe Browsing soon (Focus+WebView has Safe Browsing on newer Android versions), but says it is not a release blocker.

Comment 4

[François Marier [:francois]]

Comment on attachment 9000060 [details] [diff] [review]
0001-Bug-1483342-1.0-Add-GeckoRuntime-Safe-Browsing-API.-.patch

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

There's another control you could offer: enable protection from potentially unwanted software.

It's bundled with the malware blocking, so that needs to be enabled for unwanted software to get blocked. But if you want to block only malware, you can remove goog-unwanted-proto and test-unwanted-simple from urlclassifier.malwareTable.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to François Marier [:francois] from comment #4)
> There's another control you could offer: enable protection from potentially
> unwanted software.
> 
> It's bundled with the malware blocking, so that needs to be enabled for
> unwanted software to get blocked. But if you want to block only malware, you
> can remove goog-unwanted-proto and test-unwanted-simple from
> urlclassifier.malwareTable.

Thanks, I've wondered whether it would be feasible to unbundle unwanted and harmful from malware. I think we should discuss this in more detail for step #3.

My main concern (for this bug) is that SB is currently in this weird bundled state with TP without any API control options.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9000060 [details] [diff] [review]
0001-Bug-1483342-1.0-Add-GeckoRuntime-Safe-Browsing-API.-.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +264,5 @@
> +         * @param enabled A flag determining whether or not to block malware
> +         *                sites.
> +         * @return The builder instance.
> +         */
> +        public @NonNull Builder blockMalware(boolean enabled) {

Are malware and phishing the only categories? I thought we'd have something similar to tracking protection where you can OR together flags. I'd prefer that to separate settings like this, but not a big deal.

Comment 7

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> Are malware and phishing the only categories? I thought we'd have something
> similar to tracking protection where you can OR together flags. I'd prefer
> that to separate settings like this, but not a big deal.

Currently, malware comprises unwanted and harmful (per Gecko-pref). Exposing the 1:1 setting-to-pref mapping was the only realistic API I could offer given the time frame (going on leave next week).
But I'll happily pass the baton to someone else, if we want to land something more complex initially.

I've been thinking about consolidating the TP and SB APIs into a more general "Blocking API" with one single flag-based setting  and one callback (either rename onTrackerBlocked or reuse onLoadError with additional error categories).
There are a couple of details that would need discussion to make this API work, e.g., how GPS integration support would look like given the generic API and how blocklist/whitelist mechanics could work, should we decide to expose more control over that aspect.

Comment 8

[François Marier [:francois]]

(In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #5)
> My main concern (for this bug) is that SB is currently in this weird bundled
> state with TP without any API control options.

I'm not sure exactly what you mean by that. When you're back from PTO, let's have a meeting about the kinds of API control you'd want/need?

Comment 9

[François Marier [:francois]]

(In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #7)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> > Are malware and phishing the only categories?
> 
> Currently, malware comprises unwanted and harmful (per Gecko-pref).

This is the full list of categories that we get from Google:

- phishing ("goog-phish-proto")
- malware ("goog-malware-proto") -- desktop-only
- potentially harmful software ("goog-harmful-proto") -- Android-only
- unwanted software ("goog-unwanted-proto")

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9000060 [details] [diff] [review]
0001-Bug-1483342-1.0-Add-GeckoRuntime-Safe-Browsing-API.-.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +260,5 @@
> +         * Note: For each blocked site, {@link NavigationDelegate#onLoadError}
> +         * with error category {@link NavigationDelegate#ERROR_CATEGORY_SAFEBROWSING}
> +         * is called.
> +         *
> +         * @param enabled A flag determining whether or not to block malware

I prefer "True to determine whether..."

@@ +357,5 @@
>  
>      private final Pref<?>[] mPrefs = new Pref<?>[] {
>          mCookieBehavior, mCookieLifetime, mCookieLifetimeDays, mConsoleOutput,
> +        mJavaScript, mRemoteDebugging, mTrackingProtection, mWebFonts,
> +        mSafebrowsingMalware, mSafebrowsingPhishing,

Alphabetize

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 9000061 [details] [diff] [review]
0002-Bug-1483342-2.0-Initialize-SafeBrowsing-module-by-de.patch

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

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm
@@ +37,5 @@
>    onInit() {
>      this._useTrackingProtection = false;
>      this._useDesktopMode = false;
> +    // Required for safe browsing and tracking protection.
> +    SafeBrowsing.init();

Technically this should depend on the pref values right?

Comment 12

[Eugen Sawin [:esawin]]

(In reply to François Marier [:francois] from comment #8)
> (In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #5)
> > My main concern (for this bug) is that SB is currently in this weird bundled
> > state with TP without any API control options.
> 
> I'm not sure exactly what you mean by that. When you're back from PTO, let's
> have a meeting about the kinds of API control you'd want/need?

I mean the current state in GV. To enable SB in GV, you need to enable TP, which coincidentally enables SB by calling SafeBrowsing.init(). At this point, there is no way to disable SB in GV, even after disabling TP.
I'm just trying to get this in a sane state before we dive deeper into the actual SB API design.

Comment 13

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #11)
> Technically this should depend on the pref values right?

Correct, although I would prefer it to load as soon as possible during startup because SB and TP are likely going to be enabled (on by default). I also fear there is a race for the initial blocklist download (looking at kbrosnan's test results) in Gecko.

Long-term, the module will probably need refactoring to decouple TP from SB and for the intergration of GPS.

Comment 14

[François Marier [:francois]]

(In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #12)
> I mean the current state in GV. To enable SB in GV, you need to enable TP,
> which coincidentally enables SB by calling SafeBrowsing.init().

I think there's some confusion here.

The URL classifier is the component that does classification both for Safe Browsing (i.e. phishing/malware protection) and for TP. You can't disable the URL classifier unless you disable all of the features that use URL classification.

SafeBrowsing.jsm is one of the core parts of the URL classifier and isn't actually just for Safe Browsing. Yes, the naming in that whole module sucks for lots of legacy reasons I won't go into. If it were written today, it would most likely be called UrlClassifier.jsm.

So if you want Safe Browsing, you need to enable the URL classifier, but not TP. If you want TP, you also need to enable the URL classifier, but not Safe Browsing.

> At this point, there is no way to disable SB in GV, even after disabling TP.

If you want to disable all of Google Safe Browsing, then you need to disable these:

browser.safebrowsing.downloads.enabled = false
browser.safebrowsing.malware.enabled = false
browser.safebrowsing.passwords.enabled = false
browser.safebrowsing.phishing.enabled = false

If you want to disable the URL classifier entirely, then you also need to disable these features that rely on it and pull lists from our own server:

browser.safebrowsing.blockedURIs.enabled = false
plugins.flashBlock.enabled = false
plugins.show_infobar = false
privacy.trackingprotection.annotate_channels = false
privacy.trackingprotection.enabled = false
privacy.trackingprotection.pbmode.enabled = false

Hopefully it's a little clearer.

Comment 15

[Eugen Sawin [:esawin]]

(In reply to François Marier [:francois] from comment #14)
> So if you want Safe Browsing, you need to enable the URL classifier, but not
> TP. If you want TP, you also need to enable the URL classifier, but not Safe
> Browsing.

There is no GV API to enable SB (this is what this bug is for).
Enabling TP in GV also enables SB (by SafeBrowsing.init()).
There is no GV API to disable SB once enabled through TP (again, this bug).
 
> If you want to disable all of Google Safe Browsing, then you need to disable
> these:
> 
> browser.safebrowsing.downloads.enabled = false

That's disabled by mobile.js by default.

> browser.safebrowsing.malware.enabled = false

This bug.

> browser.safebrowsing.passwords.enabled = false

Do we actually want to allow disabling this?

> browser.safebrowsing.phishing.enabled = false

This bug.

> If you want to disable the URL classifier entirely, then you also need to
> disable these features that rely on it and pull lists from our own server:

I don't think we're too concerned with disabling the classifier, we just want to expose an API to give the app control over the blocking mechanics.

> Hopefully it's a little clearer.

Sorry for being too GV-centric in my statements which seems to be the root cause for the confusion.

Comment 16

[François Marier [:francois]]

(In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #15)
> There is no GV API to enable SB (this is what this bug is for).
> Enabling TP in GV also enables SB (by SafeBrowsing.init()).

That function, just like the .jsm file I referred to in comment 14, is badly named. It should really be named UrlClassifier.init() since it's not loading Safe Browsing; it's loading the URL classifier.

The URL classifier will need to be initialized via SafeBrowsing.init() if you enable either one of TP or SB.

> There is no GV API to disable SB once enabled through TP (again, this bug).

TP won't actually enable Safe Browsing, so I don't think there's anything to disable in GV.

> > browser.safebrowsing.downloads.enabled = false
> 
> That's disabled by mobile.js by default.

Yes, the UI was never implemented in Fennec (bug 1239094).

> > browser.safebrowsing.passwords.enabled = false
> 
> Do we actually want to allow disabling this?

No. It's disabled by default everywhere because the implementation is not finished.

> Sorry for being too GV-centric in my statements which seems to be the root
> cause for the confusion.

From what I understand of your requirements with respect to Safe Browsing, what you want is simply to have options to enable/disable the categories offered by Google on Android (comment 9):

- phishing
- potentially harmful software
- unwanted software

You've got the first one via browser.safebrowsing.phishing.enabled and you're bundling the last two together via browser.safebrowsing.malware.enabled (comment 4).

Comment 17

[Eugen Sawin [:esawin]]

(In reply to François Marier [:francois] from comment #16)
> The URL classifier will need to be initialized via SafeBrowsing.init() if
> you enable either one of TP or SB.

Yes, we do that currently when enabling TP, with patch 2 from this bug we're going to init it by default.
 
> TP won't actually enable Safe Browsing, so I don't think there's anything to
> disable in GV.

Enabling TP via the GV setting enables SB for GV by calling SafeBrowsing.init() while having the SB prefs (phishing, malware) enabled (and no API to control them).

> From what I understand of your requirements with respect to Safe Browsing,
> what you want is simply to have options to enable/disable the categories
> offered by Google on Android (comment 9):
> 
> - phishing
> - potentially harmful software
> - unwanted software
> 
> You've got the first one via browser.safebrowsing.phishing.enabled and
> you're bundling the last two together via
> browser.safebrowsing.malware.enabled (comment 4).

That's correct, that way the app using GV will have some control over the blocking mechanics of SB.
We can extend on this, but that's the basic pref-to-GV-setting API that we can offer without requiring deep dive discussions which will be required going forward.

Comment 18

[François Marier [:francois]]

(In reply to Eugen Sawin [:esawin] on leave from 20 August from comment #17)
> > TP won't actually enable Safe Browsing, so I don't think there's anything to
> > disable in GV.
> 
> Enabling TP via the GV setting enables SB for GV by calling
> SafeBrowsing.init() while having the SB prefs (phishing, malware) enabled
> (and no API to control them).

Ah, I finally see what you mean now. You're describing the _GV_ behavior _prior_ to this patch landing. Sorry it took me a while.

> That's correct, that way the app using GV will have some control over the
> blocking mechanics of SB.
> We can extend on this, but that's the basic pref-to-GV-setting API that we
> can offer without requiring deep dive discussions which will be required
> going forward.

I agree. Your patch is the simplest way to let GV disable the downloading of the Google lists if you don't care about that but would still want to take advantage of TP.

Comment 19

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #10)
> I prefer "True to determine whether..."

I'm keeping the current form for consistency with all the other boolean runtime settings for now, we can mass-rename them, if we want to.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e290808d8e
[1.1] Add GeckoRuntime Safe Browsing API. r=snorp,jchen,francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb971f169811
[2.1] Initialize SafeBrowsing module by default to enable safe browsing. r=jchen

Comment 21

[Chris Peterson [:cpeterson]]

We don't need to uplift Safe Browsing to GV 62 Beta.

Comment 22

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/47e290808d8e
https://hg.mozilla.org/mozilla-central/rev/fb971f169811

Comment 23

[Chris Peterson [:cpeterson]]

We need to uplift this API to the GV 62 relbranch so Focus 7.0 can disable Safe Browsing.

https://github.com/mozilla-mobile/focus-android/issues/3236

Comment 24

[Chris Peterson [:cpeterson]]

Comment on attachment 9000060 [details] [diff] [review]
0001-Bug-1483342-1.0-Add-GeckoRuntime-Safe-Browsing-API.-.patch

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration:

User impact if declined: Focus wants to disable Safe Browsing by default.

Fix Landed on Version: 63 Nightly

Risk to taking this patch (and alternatives if risky): Low risk because this patch just adds a new GeckoView API and landing the patch on GECKOVIEW_62_RELBRANCH doesn't risk destabilizing Firefox or Fennec 62.

String or UUID changes made by this patch: No

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment 25

[Chris Peterson [:cpeterson]]

Comment on attachment 9000061 [details] [diff] [review]
0002-Bug-1483342-2.0-Initialize-SafeBrowsing-module-by-de.patch

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration:

User impact if declined: Focus wants to disable Safe Browsing by default.

Fix Landed on Version: 63 Nightly

Risk to taking this patch (and alternatives if risky): Low risk because it's a small fix and landing the patch on GECKOVIEW_62_RELBRANCH doesn't risk destabilizing Firefox or Fennec 62.

String or UUID changes made by this patch: No

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9000061 [details] [diff] [review]
0002-Bug-1483342-2.0-Initialize-SafeBrowsing-module-by-de.patch

GV62+

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-release/rev/54454c859a682c36bd05cf4e6ff71ffac7a3d05a
https://hg.mozilla.org/releases/mozilla-release/rev/e5c09855a9ab461af3ccec1e4fb566050b1504f0