783047 - Update SafeBrowsing to use HTTPS

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Sid Stamm [:geekboy or :sstamm]]

The SafeBrowsing API has supported HTTPS for a while now and we should change our implementation to use it.  Chrome shipped it and it works well:

http://src.chromium.org/viewvc/chrome?view=rev&revision=132456

This change would remove the need for MACing.

Summary of work (via Brian Ryner):
- All http://safebrowsing.clients.google.com/ URLs should change to https://safebrowsing.google.com/  (note that the .clients part is gone)
- /newkey requests are no longer necessary.
- sending the wrapped key (wrkey parameter) in requests is no longer necessary.
- handling of a rekey response is no longer necessary
- requesting the MAC (appending ;mac to the listname) is no longer necessary
- handling of MAC data appended to redirect URLs ("u:url,mac") in the response is no longer necessary.

More detail in the SafeBrowsing developer guide:
https://developers.google.com/safe-browsing/developers_guide_v2

Comment 1

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

This is one of the last parts of the browser's built-in functionality, other than non-default search engines, using non-HTTPS connections. (See all the bugs blocking bug 771788 that have been fixed recently.) It would be great to get this change done. This change seems to be quite a non-trivial privacy win (leaking your browsing history + safebrowsing tracking cookie to only Google instead of Google + people on the network).

There may be some performance implications here, regarding the added latency of SSL. I filed bug 877766 about one potential way of mitigating that.

Raymond, are you interested in taking this on too? I see you have done many of the other HTTP -> HTTPS conversions. From comment 0, it looks like the changes (besides bug 877766) are mostly deleting code that is no longer needed.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

>This change seems to be quite a non-trivial privacy win (leaking your browsing history

SafeBrowsing does not leak your browsing history. Not to Google, not to anyone else.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Brian, Gian-Carlo is right. We are already implementing version 2 of the spec, which downloads the entire safebrowsing database in chunks and looks up URLs in downloaded database. We are *not* using the Safebrowsing Lookup API that sends every URL to the service.

The only thing this bug will solve is letting us get rid of a bunch of MACs intended to detect database corruption, which will be redundant if we get it over SSL.

Comment 4

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

(In reply to Monica Chew [:mmc] (please use ?needinfo) from comment #3)
> Brian, Gian-Carlo is right. We are already implementing version 2 of the
> spec, which downloads the entire safebrowsing database in chunks and looks
> up URLs in downloaded database. We are *not* using the Safebrowsing Lookup
> API that sends every URL to the service.

Right. It was my mistake to mix up the browsing history aspect with the tracking cookie aspect. But, the tracking cookie is still problematic, because it allows your browsing to be linked across network connections (e.g. one Wifi hotspot to another, or 3G <-> wifi). Obviously, almost any non-SSL website will cause the same issue, but I think it is worthwhile to ensure that the built-in functionality of the browser avoids such issues, especially when doing so is easy.

> The only thing this bug will solve is letting us get rid of a bunch of MACs
> intended to detect database corruption, which will be redundant if we get it
> over SSL.

Besides the tracking cookie issue, switching to HTTPS has performance consequences for the cases where we need to do the check. The performance could either be improved due to SPDY connection coalescing if you already have an open connection to another Google site, or bad if you don't already have an open connection to Google. That was/is being discussed in bug 877766.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

In that case, you might be interested in this old bug: https://bugzilla.mozilla.org/show_bug.cgi?id=368255

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>This change would remove the need for MACing.
>- requesting the MAC (appending ;mac to the listname) is no longer necessary
>- handling of MAC data appended to redirect URLs ("u:url,mac") in the response is no longer necessary.

FWIW, I just did a quick check here by switching all our URLs to HTTPS and seeing if things still work. They do. However, the update links we get are still of the form:

http://safebrowsing-cache.google.com/safebrowsing/rd/ChNnb29nLW1hbHdhcmUtc2hhdmFyEAAYwYMIIICGCDItwQECAP_____7_________7___-_r3_3___3_____f_7___________v7__8A

Which obviously aren't safe without the MAC. I'm going to make a guess here that Google sends us HTTP links if we request MAC usage. I'll try hacking away the MACs and see what happens.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

>I'm going to make a guess here that Google sends us HTTP links if we request MAC usage.

Whoops, we do that ourselves. The code that does it even points to this bug :(

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Remove MAC support.

Do we need to play the UUID dance with the changed interfaces?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Download forwards via HTTPS

I hope this catches all the tests...

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 3. Remove all MAC-related tests.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

Comment 13

[Robert Kaiser]

I heard that back when this was implemented, Google didn't want us to use https for performance or load reasons, have we cleared with them that it's OK now? (I guess with the new technologies Google and us implemented in that area, those connections might have become lighter anyhow.)

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Yes, see comment 0. The input is from Brian Ryner, which deals with SafeBrowsing at Google. Chrome already switched.

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8358438 [details] [diff] [review]
Patch 1. Remove MAC support.

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

Looks good.

::: toolkit/components/url-classifier/nsIUrlClassifierDBService.idl
@@ +31,1 @@
>  interface nsIUrlClassifierUpdateObserver : nsISupports {

This requires a uuid change (./mach uuid) right above this line.

::: toolkit/components/url-classifier/nsIUrlClassifierHashCompleter.idl
@@ +16,1 @@
>     * nsIUrlClassifierCompleter::complete() call.

uuid change

::: toolkit/components/url-classifier/nsIUrlClassifierStreamUpdater.idl
@@ +22,1 @@
>    /**

uuid change

::: toolkit/components/url-classifier/nsIUrlListManager.idl
@@ +22,1 @@
>  interface nsIUrlListManager : nsISupports

Same here

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -266,5 @@
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP
> -nsUrlClassifierStreamUpdater::RekeyRequested()

The rekeying tests from netwerk/test/unit/test_cookiejars_safebrowsing.js can also be removed.

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -389,5 @@
> -  nsAutoCString base64(key);
> -  UnUrlsafeBase64(base64);
> -
> -  // PL_Base64Decode doesn't null-terminate unless we let it allocate,
> -  // so we need to calculate the length ourselves.

Wow. Thank you for eliminating this.

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8358440 [details] [diff] [review]
Patch 3. Remove all MAC-related tests.

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

I don't understand how this interacts with the competer changes in test_partial. Other than that, looks good.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

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

Thanks very much for taking this bug. How does try look?

Please fix the uuid issues and either file a bug for removing the rekey testing from netwerk/test/unit/test_cookiejars_safebrowsing.js or include it in this patch.

::: b2g/app/b2g.js
@@ +317,1 @@
>  #ifdef MOZ_SAFE_BROWSING

This is not enabled on b2g, right?

::: browser/metro/profile/metro.js
@@ +571,1 @@
>  #ifdef MOZ_SAFE_BROWSING

Is this enabled on metro?

::: testing/profiles/prefs_general.js
@@ +67,5 @@
>  user_pref("camino.warn_when_closing", false); // Camino-only, harmless to others
>  
>  // Make url-classifier updates so rare that they won't affect tests
>  user_pref("urlclassifier.updateinterval", 172800);
>  // Point the url-classifier to the local testing server for fast failures

Can you include a note that we can't use https for localhost?

Comment 18

[Gian-Carlo Pascutto [:gcp]]

>I don't understand how this interacts with the competer changes in test_partial. Other than that, looks good.

Excellent question! This comment should answer it:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsIUrlClassifierHashCompleter.idl#24

Apparently this was needed for cases where we get a hit, need to do a completion, but don't have a valid key.

>This is not enabled on b2g, right?
>Is this enabled on metro?

It's not enabled on those. I've just changed those URLs already so it's good to go whenever we activate it.

>How does try look?

I have only verified that this actually (seems to) work, not that it passes tests :) It passes xpcshell locally, though. I'll fix up cookiejar and give it a spin.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 5. Remove cookie jar rekeying

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 6. Update application reputation for api changes

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Addressed review comments, fixed one URL that I missed:
https://tbpl.mozilla.org/?tree=Try&rev=a9f871d1c2d4

Seems to still work on Android too.

If gavin or dougt give the thumbs up, and dcamp can take a look, this is good to go (I'm assuming that try there ^^^ will go green).

Comment 22

[Doug Turner (:dougt)]

i am all for using https everywhere we can.  if you have a specific code question, or a question about our agreement with google, please ask (or send me mail).

Comment 23

[Gian-Carlo Pascutto [:gcp]]

The whiteboard says this needs sr=dougt|gavin. I take it your comment means sr: dougt+ then?

Comment 24

[[:mmc] Monica Chew (no longer reading bugmail)]

Philip, I'm not sure if this requires seamonkey changes.

Comment 25

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

Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

Please get explicit acknowledgement from an appropriate Google contact that we're going to be making this change.

Comment 26

[Philip Chee]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #24)
> Philip, I'm not sure if this requires seamonkey changes.
Anything in /browser/ would need corresponding changes. Thanks for the heads up. I'm currently tracking this in SeaMonkey Bug 920951. By the way does s/apikey/key/ apply only to Firefox? Or to everyone using this API?

Comment 27

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Philip Chee from comment #26)
> (In reply to Monica Chew [:mmc] (please use needinfo) from comment #24)
> > Philip, I'm not sure if this requires seamonkey changes.
> Anything in /browser/ would need corresponding changes. Thanks for the heads
> up. I'm currently tracking this in SeaMonkey Bug 920951. By the way does
> s/apikey/key/ apply only to Firefox? Or to everyone using this API?

It applies to everyone using this API.

Comment 28

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/661680041755
https://hg.mozilla.org/integration/mozilla-inbound/rev/e20b9bc867ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/db7475f5aecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f48e08e09d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df7eda20b61
https://hg.mozilla.org/integration/mozilla-inbound/rev/913a1c5086c5

Comment 29

[Reed Loden [:reed]]

Comment on attachment 8358441 [details] [diff] [review]
Patch 4. Update settings for all Firefox versions (includes apikey "key' add)

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

::: browser/app/profile/firefox.js
@@ +795,1 @@
>  pref("browser.safebrowsing.reportURL", "http://safebrowsing.clients.google.com/safebrowsing/report?");

You missed updating this one.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

The actual commit should be fine? I noticed it before landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f48e08e09d#l3.18

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/661680041755
https://hg.mozilla.org/mozilla-central/rev/e20b9bc867ab
https://hg.mozilla.org/mozilla-central/rev/db7475f5aecb
https://hg.mozilla.org/mozilla-central/rev/a4f48e08e09d
https://hg.mozilla.org/mozilla-central/rev/8df7eda20b61
https://hg.mozilla.org/mozilla-central/rev/913a1c5086c5

Comment 32

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ebf1b43421ac1e4792d07b2297459a99b7fd1997
Bug 783047 - Update SafeBrowsing URLs to use HTTPS. r=mmc,dcamp sr=gavin